Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(react-button): apply hover styles only on devices that support it #31700

Closed

Conversation

mainframev
Copy link
Contributor

@mainframev mainframev commented Jun 13, 2024

We have a "sticky hover" issue on pointer devices due to the lack of differentiation between pointer and hover devices. This problem also exists in v8.

I approached it with the help of media-query any-hover. Hover effect is wrapped into that query, left hover:active outside, so there will be a visual indication after tap on mobile devices.

Browser support

Support is solid

Devices

While the any-hover media query generally helps distinguish between pointer and hover devices, we must be aware of devices with multiple input methods (e.g., pointer devices connected to keyboards and mouse, or laptops with touchscreens). The any-hover: hover query targets any available input method capable of hover, whereas hover: hover applies only to the primary input.

Previous similar issues I found

Previous Behavior

Screen.Recording.2024-06-13.at.13.06.45.mov

New Behavior

Screen.Recording.2024-06-13.at.13.06.10.mov
RPReplay_Final1718277613.MP4

Related Issue(s)

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 13, 2024

Perf Analysis (@fluentui/react-components)

Scenario Render type Master Ticks PR Ticks Iterations Status
FluentProviderWithTheme virtual-rerender 36 33 10 Possible regression
All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 639 626 5000
Button mount 305 308 5000
Field mount 1130 1159 5000
FluentProvider mount 722 720 5000
FluentProviderWithTheme mount 80 87 10
FluentProviderWithTheme virtual-rerender 36 33 10 Possible regression
FluentProviderWithTheme virtual-rerender-with-unmount 73 69 10
MakeStyles mount 857 869 50000
Persona mount 1782 1718 5000
SpinButton mount 1457 1444 5000
SwatchPicker mount 1620 1636 5000

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 13, 2024

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-breadcrumb
@fluentui/react-breadcrumb - package
113.657 kB
31.532 kB
115.873 kB
31.731 kB
2.216 kB
199 B
react-button
Button
37.105 kB
10.785 kB
39.321 kB
10.914 kB
2.216 kB
129 B
react-button
CompoundButton
43.516 kB
12.076 kB
46.084 kB
12.248 kB
2.568 kB
172 B
react-button
MenuButton
41.886 kB
12.133 kB
44.149 kB
12.314 kB
2.263 kB
181 B
react-button
SplitButton
49.898 kB
13.725 kB
52.409 kB
13.922 kB
2.511 kB
197 B
react-button
ToggleButton
53.032 kB
12.545 kB
56.711 kB
12.733 kB
3.679 kB
188 B
react-components
react-components: Button, FluentProvider & webLightTheme
69.141 kB
20.157 kB
71.357 kB
20.336 kB
2.216 kB
179 B
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
211.741 kB
60.957 kB
213.957 kB
61.16 kB
2.216 kB
203 B
react-components
react-components: entire library
1.093 MB
270.372 kB
1.097 MB
270.801 kB
4.326 kB
429 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-card
Card - All
100.845 kB
28.873 kB
react-card
Card
93.629 kB
27.077 kB
react-card
CardFooter
14.356 kB
5.798 kB
react-card
CardHeader
16.879 kB
6.664 kB
react-card
CardPreview
14.42 kB
5.934 kB
react-components
react-components: FluentProvider & webLightTheme
44.442 kB
14.607 kB
react-dialog
Dialog (including children components)
98.908 kB
29.807 kB
react-message-bar
MessageBar (all components)
24.413 kB
9.117 kB
react-portal-compat
PortalCompatProvider
8.39 kB
2.64 kB
react-tag-picker
@fluentui/react-tag-picker - package
180.139 kB
54.231 kB
react-timepicker-compat
TimePicker
103.427 kB
34.598 kB
🤖 This report was generated against b059abe27750129616c2324b9e567ee5772dcb7c

@@ -0,0 +1,7 @@
{
Copy link
Collaborator

@fabricteam fabricteam Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕵 fluentuiv9 Open the Visual Regressions report to inspect the affected screenshots

Card Converged 2 screenshots
Image Name Diff(in Pixels) Image Type
Card Converged.appearance - High Contrast.normal.chromium.png 0 Removed
Card Converged.appearance.normal.chromium.png 0 Removed
Card Converged - Interactive 9 screenshots
Image Name Diff(in Pixels) Image Type
Card Converged - Interactive.appearance interactive - Filled - High Contrast.click.chromium.png 0 Added
Card Converged - Interactive.appearance interactive - Filled - High Contrast.hover.chromium.png 0 Added
Card Converged - Interactive.appearance interactive - Filled - High Contrast.normal.chromium.png 0 Added
Card Converged - Interactive.appearance interactive - Filled Alternative - High Contrast.click.chromium.png 0 Added
Card Converged - Interactive.appearance interactive - Filled Alternative - High Contrast.hover.chromium.png 0 Added
Card Converged - Interactive.appearance interactive - Filled Alternative - High Contrast.normal.chromium.png 0 Added
Card Converged - Interactive.appearance interactive - Outline - Dark Mode.click.chromium.png 0 Removed
Card Converged - Interactive.appearance interactive - Outline - Dark Mode.hover.chromium.png 0 Removed
Card Converged - Interactive.appearance interactive - Outline - Dark Mode.normal.chromium.png 0 Removed
Card Converged - Selectable 8 screenshots
Image Name Diff(in Pixels) Image Type
Card Converged - Selectable.appearance selectable - Subtle - Dark Mode.click.chromium.png 0 Removed
Card Converged - Selectable.appearance selectable - Subtle - Dark Mode.hover.chromium.png 0 Removed
Card Converged - Selectable.appearance selectable - Subtle - Dark Mode.normal.chromium.png 0 Removed
Card Converged - Selectable.appearance selectable - Subtle - Dark Mode.selected.chromium.png 0 Removed
Card Converged - Selectable.appearance selectable - Subtle - High Contrast.click.chromium.png 0 Removed
Card Converged - Selectable.appearance selectable - Subtle - High Contrast.hover.chromium.png 0 Removed
Card Converged - Selectable.appearance selectable - Subtle - High Contrast.normal.chromium.png 0 Removed
Card Converged - Selectable.appearance selectable - Subtle - High Contrast.selected.chromium.png 0 Removed
SwatchPicker Converged 1 screenshots
Image Name Diff(in Pixels) Image Type
SwatchPicker Converged.Shape.default.chromium.png 0 Added

@mainframev mainframev marked this pull request as ready for review June 13, 2024 12:07
@mainframev mainframev requested review from khmakoto and a team as code owners June 13, 2024 12:07
@mainframev mainframev force-pushed the fix/button-hover-mobile branch from 5f0ae01 to b5037aa Compare June 13, 2024 12:52
Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's have a discussion about this on Tech sync first.

For example, I have following concerns:

  • this PR adds a bundle size hit
  • consumers' overrides for :hover selector will stop to work as a new selector will be @media (any-hover: hover)

@spmonahan
Copy link
Contributor

Let's have a discussion about this on Tech sync first.

For example, I have following concerns:

  • this PR adds a bundle size hit
  • consumers' overrides for :hover selector will stop to work as a new selector will be @media (any-hover: hover)

+1 this. While I really like these newer selectors that, in theory, allow us to better support multiple input modes my past experience has been that they are not uniformly implemented and need lots of testing. As part of further discussion I'd propose we have a device support/testing matrix for features like this.

@mainframev
Copy link
Contributor Author

@layershifter @spmonahan thank you guys! As suggested, let's discuss more in detail on tech sync 👍🏻

@mainframev
Copy link
Contributor Author

Closing in favor of feature request #31858.

@mainframev mainframev closed this Jun 28, 2024
@mainframev mainframev self-assigned this Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Sticky hover effect on mobile
4 participants