[Slider] Improve touch passive event handling #22269

mikhalev-im · 2020-08-18T18:48:03Z

Fixes #13623

Hope I followed all the guides and didn't miss anything codewise, as this is my first-time contribution

I have followed (at least) the PR section of the contributing guide.

mui-pr-bot · 2020-08-18T18:57:03Z

Details of bundle changes

Generated by 🚫 dangerJS against d32384d

oliviertassinari · 2020-08-19T10:24:52Z

Solve the issue:

oliviertassinari · 2020-08-19T10:32:45Z

The rule from lighthouse: https://web.dev/uses-passive-event-listeners/. For those who wonder why
https://github.com/mui-org/material-ui/blob/776c80b47987d5180362ea613813c0e182e6868e/packages/material-ui/src/Tooltip/Tooltip.js#L427 or https://github.com/mui-org/material-ui/blob/776c80b47987d5180362ea613813c0e182e6868e/packages/material-ui/src/SwipeableDrawer/SwipeableDrawer.js#L495
don't trigger this warning, it's because lighthouse only look at touch start events inside of the document (with synthetic events we benefit from delegation)

We push the discussion further in #20990. At least relying more on passive events is a step in the right direction.

oliviertassinari · 2020-08-19T10:38:48Z

With React 17 change of event delegation, I would expect this problem to surface again facebook/react#6436

oliviertassinari · 2020-08-19T10:42:19Z

Bingo #22252

eps1lon

If I understand this correctly we use a pure CSS solution by default where touchAction: none means scrolling is disabled. However, in iOS this feature isn't supported so we have to fallback to a non-passive scroll listener that calls preventDefault()?

If that is true then I'd make two suggestions:

use feature detection instead of user-agent sniffing
That way new versions of iOS automatically use the better implementation (no JS) once it supports it. And we don't have to rely on compatibility tables which are only useful as an approximation. I know of at least 2 examples in the past year where they were outdated. I'll try to come up with an example for this. Need to find Safari 12 on iOS first.
Don't check the feature state in the event handler. Seems to me iOS === true implies that the event is cancelable

eps1lon · 2020-08-19T14:17:20Z

Tested with firefox 51 which does not support touch-action: none according to caniuse. Used a production deploy of https://codesandbox.io/s/zen-fermat-1o2wt?file=/src/App.js available under https://csb-1o2wt-o0qacglln.vercel.app/

let cachedSupportsTouchActionNone;
function doesSupportTouchActionNone() {
  if (cachedSupportsTouchActionNone === undefined) {
    const element = document.createElement("div");
    element.style.touchAction = "none";
    document.body.appendChild(element);
    cachedSupportsTouchActionNone =
      window.getComputedStyle(element).touchAction === "none";
    element.parentElement.removeChild(element);
  }
  return cachedSupportsTouchActionNone;
}

It's a bit more involved but also way more robust. Considering the latest safari on iOS does support touch-action: none and passive event listeners are quite the perf boost I'd say it's worth it.

oliviertassinari · 2020-08-19T14:42:41Z

@eps1lon Your assumptions are correct. To answer your points:

I don't know how to feature detect the support of such a feature. I doubt it's possible has it's at the CSS level. Considering that touch action is supported by iOS 13 and that this version has above 80% of market share on iOS, we could drop the iOS check https://www.statista.com/statistics/1118925/mobile-apple-ios-version-share-worldwide/ in the near future.
Checking for .canceable wasn't enough. We get a warning in Chrome if we don't do so. To me, it doesn't make sense.

I have updated the logic with a new commit, to make it cleaner.

oliviertassinari · 2020-08-19T14:50:33Z

are quite the perf boost I'd say it's worth it

I don't follow how you come to this conclusion. From what I understand, there are two major elements that reduce its performance win potential.

the area of the screen: only where the slider is, a fraction on the area of the screen.
the number of events, only one, the touch start.

eps1lon · 2020-08-19T14:55:21Z

packages/material-ui/src/Slider/Slider.js

@@ -618,8 +624,7 @@ const Slider = React.forwardRef(function Slider(props, ref) {
  });

  const handleTouchStart = useEventCallback((event) => {
-    if (event.cancelable && iOS) {
-      // Workaround as Safari has partial support for touchAction: 'none'.
+    if (!touchActionSupport) {


Why not just check event.cancelable? That way we don't accidentally leave this untouch if we change when we add passive listeners or not.

We get a warning in Chrome if we don't do so.

document this please

And we should investigate it further. This doesn't seem right.

And we should investigate it further. This doesn't seem right.

Agree, it doesn't seem right, but I don't think that we should care either. Will it make a difference?

eps1lon · 2020-08-19T14:56:07Z

I don't know how to feature detect the support of such a feature. I doubt it's possible has it's at the CSS level.

I said that I verified it with Firefox 51. Do you have an example where it fails?

oliviertassinari · 2020-08-19T15:32:30Z

I said that I verified it with Firefox 51. Do you have an example where it fails?

Thanks for the example, I haven't tested, it's ingenious. Honestly, I think that we can keep the iOS check, it sounds good enough for the problem. I believe there are no performance concern #22269 (comment), it's more a matter of preparing the drop of Safari < 13 support.

concerns should have been resolved

eps1lon · 2020-08-22T07:00:16Z

packages/material-ui/src/Slider/Slider.js

@@ -126,6 +126,23 @@ const axisProps = {

 const Identity = (x) => x;

+// TODO: remove support for Safari < 13.


Aside: I really need to get an implementation working with pointer events. With those we don't need touchAction: none if I remember correctly.

Would we need to drop the support of older phones if we move to pointer events?

I don't know what phones we're supporting. But even our current browserslist would match

packages/material-ui/src/Slider/Slider.js

oliviertassinari · 2020-08-22T07:35:06Z

@mikhalev-im Thanks for the help

[Slider] Improve touch passive event handling (mui#13623)

761d3ac

mikhalev-im force-pushed the slider-passive-events branch from f046241 to 761d3ac Compare August 18, 2020 19:12

oliviertassinari added the component: slider This is the name of the generic UI component, not the React module! label Aug 18, 2020

avoid warning console

bc0f645

oliviertassinari added the new feature New feature or request label Aug 18, 2020

oliviertassinari approved these changes Aug 18, 2020

View reviewed changes

oliviertassinari changed the title ~~[Slider] Improve touch passive event handling (#13623)~~ [Slider] Improve touch passive event handling Aug 18, 2020

oliviertassinari approved these changes Aug 18, 2020

View reviewed changes

oliviertassinari added 2 commits August 19, 2020 12:05

minimize diff

8d1db3a

not relevant?

5d58d25

oliviertassinari requested a review from eps1lon August 19, 2020 10:38

oliviertassinari mentioned this pull request Aug 19, 2020

React v17 lighthouse passive events #22252

Closed

2 tasks

eps1lon previously requested changes Aug 19, 2020

View reviewed changes

Sebastian review

1d113db

eps1lon reviewed Aug 19, 2020

View reviewed changes

sebatian review

bc397e5

oliviertassinari force-pushed the slider-passive-events branch from ac0af5d to bc397e5 Compare August 21, 2020 19:22

oliviertassinari requested a review from eps1lon August 21, 2020 19:24

github-actions bot mentioned this pull request Aug 21, 2020

Update snapshots for pr/22269 eps1lon/mui-scripts-incubator#763

Closed

yarn proptypes

3661517

eps1lon reviewed Aug 22, 2020

View reviewed changes

eps1lon requested changes Aug 22, 2020

View reviewed changes

packages/material-ui/src/Slider/Slider.js Outdated Show resolved Hide resolved

Update packages/material-ui/src/Slider/Slider.js

d32384d

eps1lon approved these changes Aug 22, 2020

View reviewed changes

oliviertassinari merged commit 2592a60 into mui:next Aug 22, 2020

oliviertassinari mentioned this pull request Apr 14, 2021

[ClickAwayListener] Don't call onClickAway in the event cycle that mounted it #25741

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Slider] Improve touch passive event handling #22269

[Slider] Improve touch passive event handling #22269

mikhalev-im commented Aug 18, 2020 •

edited

Loading

mui-pr-bot commented Aug 18, 2020 •

edited

Loading

oliviertassinari commented Aug 19, 2020 •

edited

Loading

oliviertassinari commented Aug 19, 2020

oliviertassinari commented Aug 19, 2020

oliviertassinari commented Aug 19, 2020

eps1lon left a comment

eps1lon commented Aug 19, 2020

oliviertassinari commented Aug 19, 2020

oliviertassinari commented Aug 19, 2020 •

edited

Loading

eps1lon Aug 19, 2020

eps1lon Aug 19, 2020

eps1lon Aug 19, 2020

oliviertassinari Aug 19, 2020 •

edited

Loading

eps1lon commented Aug 19, 2020

oliviertassinari commented Aug 19, 2020 •

edited

Loading

eps1lon Aug 22, 2020

oliviertassinari Aug 22, 2020

eps1lon Aug 22, 2020

oliviertassinari commented Aug 22, 2020

		@@ -126,6 +126,23 @@ const axisProps = {

		const Identity = (x) => x;

		// TODO: remove support for Safari < 13.

[Slider] Improve touch passive event handling #22269

[Slider] Improve touch passive event handling #22269

Conversation

mikhalev-im commented Aug 18, 2020 • edited Loading

mui-pr-bot commented Aug 18, 2020 • edited Loading

oliviertassinari commented Aug 19, 2020 • edited Loading

oliviertassinari commented Aug 19, 2020

oliviertassinari commented Aug 19, 2020

oliviertassinari commented Aug 19, 2020

eps1lon left a comment

Choose a reason for hiding this comment

eps1lon commented Aug 19, 2020

oliviertassinari commented Aug 19, 2020

oliviertassinari commented Aug 19, 2020 • edited Loading

eps1lon Aug 19, 2020

Choose a reason for hiding this comment

eps1lon Aug 19, 2020

Choose a reason for hiding this comment

eps1lon Aug 19, 2020

Choose a reason for hiding this comment

oliviertassinari Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

eps1lon commented Aug 19, 2020

oliviertassinari commented Aug 19, 2020 • edited Loading

eps1lon Aug 22, 2020

Choose a reason for hiding this comment

oliviertassinari Aug 22, 2020

Choose a reason for hiding this comment

eps1lon Aug 22, 2020

Choose a reason for hiding this comment

oliviertassinari commented Aug 22, 2020

mikhalev-im commented Aug 18, 2020 •

edited

Loading

mui-pr-bot commented Aug 18, 2020 •

edited

Loading

oliviertassinari commented Aug 19, 2020 •

edited

Loading

oliviertassinari commented Aug 19, 2020 •

edited

Loading

oliviertassinari Aug 19, 2020 •

edited

Loading

oliviertassinari commented Aug 19, 2020 •

edited

Loading