[Slider] Increase hover target size #18074
Conversation
eps1lon
added
new feature
Details of bundle changes.Comparing: e049ad8...4a8de83
|
eps1lon
force-pushed
the
feat/Slider/bigger-hitbox
branch
from
ca7ceb9
to
4a8de83
Compare
|
Given it's the first time (I believe?) we apply this approach to improve the touch zone area on mobile, do you know if we have different components to handle? If we do, does the support scale to these other components? For instance, the Slider's rail padding was a result of a tradeoff to allow compact display while maintaining a not too bad interaction zone. As we handle the thumb in this pull request, should we do something for the rail zone? |
|
It is a regression from the previous implementation. What should we do for the rail? |
|
@eps1lon Benchmarking alternative implementations, I have found the following:
I would propose to increase the whole slider interactive zone, only on "mobile". What do you think about something like this?: diff --git a/packages/material-ui/src/Slider/Slider.js b/packages/material-ui/src/Slider/Slider.js
index 01d73ad0e..c0a303b48 100644
--- a/packages/material-ui/src/Slider/Slider.js
+++ b/packages/material-ui/src/Slider/Slider.js
@@ -154,6 +154,18 @@ export const styles = theme => ({
height: '100%',
padding: '0 11px',
},
+ // The primary input mechanism of the device includes a pointing device of limited accuracy.
+ '@media (pointer: coarse)': {
+ '&::after': {
+ position: 'absolute',
+ content: '""',
+ // Reach 42px touch target, about ~8mm on screen.
+ left: -9,
+ top: -9,
+ right: -9,
+ bottom: -9,
+ },
+ },
},With 9/48*42 = 7.875mm, we should be within the Material Design recommended range:
|
@eps1lon Thanks for looking at this regression! I hadn't realized it. |
We don't branch in the core for other components. Why start specifically here?
I have no idea where these numbers come from. Why start deviating here but not in the other components? If you have a general issue with touch hit size please open an issue. I tried to follow the general core approach. If I didn't please point to the instances where we do it differently. Otherwise we can address this across the core in a separate PR. Same applies to the rail. The primary concern is fixing a regression. |
Sorry, in https://material.io/design/usability/accessibility.html#layout-typography, the spec mention that 48px on screens translate, on average to 9mm. From this correspondence, we can estimate the size on the screen of 42px.
It's a great question!
It seems that the rail is an integral part of the regression, at least, if we compare it with v3.9.3. Should we include it?
|
|
I'm wondering about something with the |
I have tested it, it prevents the interaction with the other elements. Having a different behavior based on the precision of the pointer device might create unpredictable interaction zone overlaps. So I don't think that my proposed approach is great. Maybe something like this instead to avoid conflicts? --- a/packages/material-ui/src/Slider/Slider.js
+++ b/packages/material-ui/src/Slider/Slider.js
@@ -154,6 +154,18 @@ export const styles = theme => ({
height: '100%',
padding: '0 11px',
},
+ // The primary input mechanism of the device includes a pointing device of limited accuracy.
+ '@media (pointer: coarse)': {
+ // Reach 42px touch target, about ~8mm on screen.
+ padding: '20px 0',
+ '&$vertical': {
+ padding: '0 20px',
+ },
+ },
}, |
Please include this test. |
|
@eps1lon Sure: https://codesandbox.io/s/material-demo-u9y9b would be my only concern with the change. |
In general this UI should be discouraged (and is so by Material guidelines) since the space between interactive elements is very small.
-- https://material.io/design/layout/spacing-methods.html#touch-targets Beyond that I can't find an issue with your demo (using latest) or my fork (using this PR: https://codesandbox.io/s/material-demo-vwilb) |
codesandbox-ci to the rescue 👌! Right, it doesn't reproduce, I had to inverse the order to make it noticeable:
Yeah, there are some points to consider in this land:
What do you think of reducing the interaction zone width and height from 48px to 24px? It's 12px on master, we would still x4 the interaction surface with 24px. |
Use touch target size of 48px from Material guidelines (https://material.io/design/usability/accessibility.html#layout-typography)
The actual touch target size was already good since we use the full rail. But we could get some toggle flicker when hovering from label to thumb
mastervs.
branch
