Better recognition of touch devices #1653

dmtrKovalenko · 2020-04-13T10:01:06Z

This PR closes #1649
Also closes #1664

Description

Use media (pointer: fine) (https://developer.mozilla.org/en-US/docs/Web/CSS/@media/pointer) which is the best way to determine touch devices. So use it to render mobile wrapper and hide focus outline.

P.S. It is not supported in IE but it is fine because we are falling back to desktop mode :)

…pper

vercel · 2020-04-13T10:01:11Z

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/mui-org/material-ui-pickers/ei6e89ngg
✅ Preview: https://material-ui-pickers-git-bugfix-desktop-responsive-breakpoint.mui-org.now.sh

cypress · 2020-04-13T10:03:35Z

Test summary

73 • 0 • 1 • 0

Run details


Project	material-ui-pickers
Status	Passed
Commit	`7c61217`
Started	Apr 21, 2020 11:09 AM
Ended	Apr 21, 2020 11:10 AM
Duration	01:12 💡
OS	Linux Debian - 9.11
Browser	Chrome 78

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

codecov · 2020-04-13T10:04:57Z

Codecov Report

Merging #1653 into next will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             next    #1653   +/-   ##
=======================================
  Coverage   89.76%   89.77%           
=======================================
  Files          73       73           
  Lines        2315     2317    +2     
  Branches      402      420   +18     
=======================================
+ Hits         2078     2080    +2     
  Misses        190      190           
  Partials       47       47

Impacted Files	Coverage Δ
lib/src/DateRangePicker/DateRangePicker.tsx	`97.95% <100.00%> (ø)`
lib/src/constants/dimensions.ts	`100.00% <100.00%> (ø)`
lib/src/wrappers/DesktopPopperWrapper.tsx	`84.90% <100.00%> (+0.29%)`	⬆️
lib/src/wrappers/DesktopWrapper.tsx	`100.00% <100.00%> (ø)`
lib/src/wrappers/ResponsiveWrapper.tsx	`96.29% <100.00%> (-0.14%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2323a4b...7c61217. Read the comment docs.

oliviertassinari

I wonder about a potential inconsistency this prop can bring. In the CSS we will use sm, with no way to have it configured. Do we need to expose a desktopModeBreakpoint prop? What if it was hard coded?

lib/src/wrappers/ResponsiveWrapper.tsx

dmtrKovalenko · 2020-04-13T10:10:54Z

What do you mean by? For example somebody can decide to show the mobile picker for tablets

In the CSS we will use sm, with no way to have it configured

oliviertassinari · 2020-04-13T10:25:56Z

Ok, so we have two different concerns:

Logic consistency. Would it be weird to have a mobile version with small day cells or a desktop version with large day cells? Should we make sure that the CSS media query match the version of the component?
Threshold. What's the tablet's screen width? What version should we display for a tablet?

It seems that it's should be the touch vs mouse pointer support that should determine what version to display. Maybe we are no looking at the right media query. What if we were using @media (hover: none) instead of the screen width? The more I think about it, the more I like this approach. However, I have seen cases (in Galaxy phones where it doesn't work, there is an issue in the core). So I think that we would also need to add a condition for small screens.

dmtrKovalenko · 2020-04-16T16:15:04Z

@oliviertassinari really, it is the best way to recognize touch devices. It works really nice for our case. Thanks for this idea! 💪

dmtrKovalenko · 2020-04-16T16:57:56Z

Todo figure out visual regression

oliviertassinari · 2020-04-16T18:30:50Z

Regarding the limitation with Samsung phones, here is it: mui/material-ui#15000. You can use BrowserStack to try that out.
I believe something like this @media (hover: none), (max-width: 600px) would allow to cover most of the cases.

dmtrKovalenko · 2020-04-17T09:21:09Z

@oliviertassinari also I want to handle the case when the user is on desktop, but the window is small (e.g. iframes or just resized window)

Maybe we can make a hook isTouchDevice and place it in @materail-ui/core/utils with all the required logic to recognize touch device?

oliviertassinari · 2020-04-17T18:35:08Z

Maybe we can make a hook isTouchDevice and place it in @materail-ui/core/utils with all the required logic to recognize touch device?

@dmtrKovalenko Given that we only have a use case, for now, in the date picker, I think that we should keep the solution scoped to the date picker. Once 1. we find a solution, 2. that we stress test with the users of the date picker, 3. that we see a need outside the date picker, then, yes, definitely, I think that it would make a great addition to the /core or /utils package.

oliviertassinari · 2020-04-17T19:22:31Z

This approach seems to do it.

   const ResponsiveWrapper: React.FC<ResponsiveWrapperProps> = ({
-    desktopModeMediaQuery = '(hover: hover)',
+    desktopModeMediaQuery = '@media (pointer: fine)',
     okLabel,
     cancelLabel,
     clearLabel,

I have included @media to ease pattern search in the repository (between JS and CSS)

The global document listener approach would be challenging if we need to wait for a first pointer interaction: https://stackoverflow.com/questions/23885255/how-to-remove-ignore-hover-css-style-on-touch-devices

dmtrKovalenko · 2020-04-21T11:04:12Z

Confirmed for galaxy s* (pointer: fine) is working just fine

oliviertassinari

Awesome. I'm excited, I can't wait to try it out in real life (publish) and the implication for updating the styles for desktop #1648 :)

oliviertassinari · 2020-04-21T11:11:27Z

lib/src/wrappers/ResponsiveWrapper.tsx

+  /** Css media query when `Mobile` mode will be changed to `Desktop`
+   * @default "@media (pointer: fine)"
+   * @example "@media (min-width: 720px)" or theme.breakpoints.up("sm")


What do you think of this variation to better match the style of the mono-repository for the description of the props?

Suggested change

/** Css media query when `Mobile` mode will be changed to `Desktop`

* @default "@media (pointer: fine)"

* @example "@media (min-width: 720px)" or theme.breakpoints.up("sm")

/**

* CSS media query when "mobile" mode will be changed to "desktop".

*

* @default "@media (pointer: fine)"

* @example "@media (min-width: 720px)" or theme.breakpoints.up("sm")

Mobile and Desktop are used as prefixes for specific components (e.g. MobileDatePicker) so it should be more clear for users. Isn't it?

Using Mobile feels like it's a piece of code that can be used on its own. What about 1. MobileDatePicker or 2. "mobile", I think that we would lose value if we try to compromise between these two options.

oliviertassinari · 2020-04-21T11:12:02Z

lib/src/wrappers/ResponsiveWrapper.tsx

   */
-  desktopModeBreakpoint?: Breakpoint;
+  desktopModeMediaQuery?: string;


Have you considered a shorter name for the prop?

Suggested change

desktopModeMediaQuery?: string;

desktopMediaQuery?: string;

I think the shorter variant is less expressive. What "desktop" means?

It could be interpreted as "a media query for desktop", definitely not as good as the information we can provide in a description. Adding mode definitely provide extra context. I don't know. I was wondering about this tradeoff of information vs length for the props, e.g. openPickerIcon, vs pickerIcon.

oliviertassinari · 2020-04-21T11:14:29Z

Also closes #1664

Are you sure about this one? Yes, it solves the issue on mobile, but what about desktop?

dmtrKovalenko · 2020-04-21T12:00:19Z

Yeah, because it needs for desktop a11y. We need to render outline for the focused element. (This is a problem of trap focus actually😮)

And it is not a problem for desktop, because we are not doing missclicks thanks to mouse. On desktop you will probably never see it if not navigating through the Tab. But on mobile it is appearing all the time.

oliviertassinari · 2020-04-21T12:03:21Z

I have added an extra description on #1664 to describe what I have observed.

spider9375 · 2024-08-08T16:14:17Z

@oliviertassinari sorry for bringing this up, but i'm confused. The documentation states:

fine
The primary input mechanism includes an accurate pointing device, such as a mouse.

This means that this media query matches desktop, and not mobile devices.

And at the same time the name of the constant is
IS_TOUCH_DEVICE_MEDIA which when true means that it is in fact not touch device and instead one with a mouse? Or am i understanding this wrong?

Change breakpoint when desktop mode will be enabled for ResponsiveWra…

ab5c99b

…pper

dmtrKovalenko added the PR: ready to ship label Apr 13, 2020

oliviertassinari reviewed Apr 13, 2020

View reviewed changes

lib/src/wrappers/ResponsiveWrapper.tsx Outdated Show resolved Hide resolved

oliviertassinari changed the title ~~Change breakpoint when desktop mode will be enabled for ResponsiveWra…~~ Reduce responsive breakpoint from md to sm Apr 13, 2020

oliviertassinari removed the PR: ready to ship label Apr 13, 2020

oliviertassinari mentioned this pull request Apr 13, 2020

[DatePicker] Update to match the specification (desktop) #1648

Closed

Use media hover to better determine touch devices

77ff855

vercel bot deployed to Preview April 16, 2020 16:11 View deployment

dmtrKovalenko changed the title ~~Reduce responsive breakpoint from md to sm~~ Better recognition of touch devices Apr 16, 2020

dmtrKovalenko requested a review from oliviertassinari April 16, 2020 16:15

Fix incorrect type

ee842a5

vercel bot deployed to Preview April 16, 2020 16:57 View deployment

Use real touch device for pickers visual regression

b2c78b6

vercel bot deployed to Preview April 16, 2020 17:00 View deployment

This comment has been minimized.

Sign in to view

Change touch device media to (hover: fine)

b09ae11

vercel bot deployed to Preview April 21, 2020 10:58 View deployment

Remove unused import

7c61217

vercel bot deployed to Preview April 21, 2020 11:05 View deployment

oliviertassinari reviewed Apr 21, 2020

View reviewed changes

oliviertassinari approved these changes Apr 21, 2020

View reviewed changes

dmtrKovalenko merged commit 512c17e into next Apr 21, 2020

dmtrKovalenko deleted the bugfix/desktop-responsive-breakpoint branch April 24, 2020 09:30

oliviertassinari mentioned this pull request May 16, 2020

[Autocomplete] Requires two clicks to open on mobile ios after state is set mui/material-ui#21049

Closed

2 tasks

oliviertassinari mentioned this pull request May 30, 2020

@media (hover: none) issue on older mobile devices mui/material-ui#15736

Closed

oliviertassinari mentioned this pull request Oct 16, 2020

[Tooltip] Refactor touch handling mui/material-ui#23088

Closed

Angelk90 mentioned this pull request Nov 15, 2020

[DatePicker] Strange black border in the calendar mui/material-ui#23532

Closed

2 tasks

oliviertassinari mentioned this pull request Aug 16, 2023

[pickers] Some mobile phones render desktop pickers (i.e. Google Pixel 7) mui/mui-x#10039

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better recognition of touch devices #1653

Better recognition of touch devices #1653

dmtrKovalenko commented Apr 13, 2020 •

edited

Loading

vercel bot commented Apr 13, 2020 •

edited

Loading

cypress bot commented Apr 13, 2020 •

edited

Loading

codecov bot commented Apr 13, 2020 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

dmtrKovalenko commented Apr 13, 2020

oliviertassinari commented Apr 13, 2020 •

edited

Loading

dmtrKovalenko commented Apr 16, 2020

dmtrKovalenko commented Apr 16, 2020

oliviertassinari commented Apr 16, 2020

dmtrKovalenko commented Apr 17, 2020

oliviertassinari commented Apr 17, 2020 •

edited

Loading

oliviertassinari commented Apr 17, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

dmtrKovalenko commented Apr 21, 2020

oliviertassinari left a comment

oliviertassinari Apr 21, 2020

dmtrKovalenko Apr 21, 2020

dmtrKovalenko Apr 21, 2020

oliviertassinari Apr 21, 2020 •

edited

Loading

oliviertassinari Apr 21, 2020

dmtrKovalenko Apr 21, 2020

oliviertassinari Apr 21, 2020

oliviertassinari commented Apr 21, 2020

dmtrKovalenko commented Apr 21, 2020

oliviertassinari commented Apr 21, 2020

spider9375 commented Aug 8, 2024

Better recognition of touch devices #1653

Better recognition of touch devices #1653

Conversation

dmtrKovalenko commented Apr 13, 2020 • edited Loading

Description

vercel bot commented Apr 13, 2020 • edited Loading

cypress bot commented Apr 13, 2020 • edited Loading

Test summary

Run details

codecov bot commented Apr 13, 2020 • edited Loading

Codecov Report

oliviertassinari left a comment • edited Loading

Choose a reason for hiding this comment

dmtrKovalenko commented Apr 13, 2020

oliviertassinari commented Apr 13, 2020 • edited Loading

dmtrKovalenko commented Apr 16, 2020

dmtrKovalenko commented Apr 16, 2020

oliviertassinari commented Apr 16, 2020

dmtrKovalenko commented Apr 17, 2020

oliviertassinari commented Apr 17, 2020 • edited Loading

oliviertassinari commented Apr 17, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

dmtrKovalenko commented Apr 21, 2020

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari Apr 21, 2020

Choose a reason for hiding this comment

dmtrKovalenko Apr 21, 2020

Choose a reason for hiding this comment

dmtrKovalenko Apr 21, 2020

Choose a reason for hiding this comment

oliviertassinari Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Apr 21, 2020

Choose a reason for hiding this comment

dmtrKovalenko Apr 21, 2020

Choose a reason for hiding this comment

oliviertassinari Apr 21, 2020

Choose a reason for hiding this comment

oliviertassinari commented Apr 21, 2020

dmtrKovalenko commented Apr 21, 2020

oliviertassinari commented Apr 21, 2020

spider9375 commented Aug 8, 2024

dmtrKovalenko commented Apr 13, 2020 •

edited

Loading

vercel bot commented Apr 13, 2020 •

edited

Loading

cypress bot commented Apr 13, 2020 •

edited

Loading

codecov bot commented Apr 13, 2020 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

oliviertassinari commented Apr 13, 2020 •

edited

Loading

oliviertassinari commented Apr 17, 2020 •

edited

Loading

oliviertassinari commented Apr 17, 2020 •

edited

Loading

oliviertassinari Apr 21, 2020 •

edited

Loading