fix(material/slider): incorrectly attributing touches for multiple touch events #22615

crisbeto · 2021-05-02T14:20:44Z

mat-slider is set up so that it binds global touchmove and touchend events after dragging starts so that the user can drag while not keeping their finger right on top of the slider. The problem is that currently we only use the first touch from the list of touches which means that if the user is dragging more than one slider on a multi-touch device, they'll all have the same value.

These changes add some logic that attributes the events to the slider that started the dragging sequence.

Note: I haven't included unit tests for this, because we don't seem to have any tests for touch events on the slider (not sure if there are historical reasons for this). I've been testing the fix against a real touch device.

Fixes #22614.

puschkin · 2021-05-02T16:18:04Z

I couldn't verify this PR.

Steps to reproduce:

yarn run dev app

open dev app on phone (android, chrome)
go to sliders page
touch and move left color slider with finger 1, don't let go
-> works
touch and move right color slider with finger 2, don't let go
-> works for both sliders
release finger 1 from left slider
touch and move center color slider with finger 1, don't let go
-> Doesn't work, the center slider jumps immediately to 100%

puschkin · 2021-05-02T16:22:31Z

src/material/slider/slider.ts

-      if (oldValue != this.value) {
-        this._emitInputEvent();
+      this._touchId = isTouchEvent(event) ?
+        event.touches[event.touches.length - 1]?.identifier : undefined;


I think that this doesn't work as intended, if the touches are not all released in the opposite order as they are started

This is only the touchstart though, after this the order of events doesn't matter.

yes, but here, we get the touch id. I'm not sure if your assumption, that the last touch in the touches list is "our" touch, is correct.

I added a console.log(this._touchId); in line 650 to verify the ID, and when I am repeating my steps from my comment [1], I get the output:
0
1
1
That means, the middle slider listens on the same ID as the right slider

[1] #22615 (comment)

When I was working on it, I tried logging out the identifiers and it seemed like they were being added sequentially any time you tap with a different finger.

I just looked at some code which I implemented for my job. It's simulating a touch device (uinput) on linux.
There, I also have to define the identifier that is used for the touch.

My assumption (which I verify with my code from work) is, that the identifier in the touch event (js in the browser) is given from the touch device (driver) and not from the browser.

That would then explain why it works differently on android and the iPhone

Thank you for looking into it, I'll go with a modified version of your proposal for now and we can iterate on it if it ends up breaking something.

Also yeah, it's weird that there isn't a standard way for the identifiers to be generated. I noticed that the Chrome dev tools has a different way of doing it too.

I'll test your implementation this evening.

Okay, I've tested on android with firefox and chrome.
On both devices, multitouch works now as expected, including the scenario that was not working before.

Great!
Thanks a lot!

yay, thank you for verifying 🎉

crisbeto · 2021-05-02T16:54:44Z

Regarding the issue mentioned above, I couldn't reproduce the issue with step 6. I tried following the instructions, but it worked as expected. Here's a recording from my device:

RPReplay_Final1619974289.MP4

puschkin · 2021-05-02T17:21:12Z

Regarding the issue mentioned above, I couldn't reproduce the issue with step 6. I tried following the instructions, but it worked as expected.

Well, yes I agree, that looks as it should.
If you want I can try to make a video as well.

What device/browser are you using? Maybe the touch IDs are always increasing on your setup?

crisbeto · 2021-05-02T17:40:13Z

I've testing it on an iPhone 7 running iOS 14.4.

puschkin · 2021-05-02T17:43:19Z

@crisbeto Are you using Safari? Could you maybe try to reproduce my issue with chrome on the IPhone?

crisbeto · 2021-05-02T18:02:57Z

Yes, I was testing on Safari, but I just tried Chrome and it worked the same. My understanding is that Chrome on iOS is basically a Safari web view due to some Apple policy that doesn't allow custom web browsers.

puschkin · 2021-05-06T11:32:24Z

Some general question:
Are there other components which share a similar behavior towards dragging via touch?
Perhaps this multitouch behavior could be outsourced into a directive or something in future.

crisbeto · 2021-05-06T15:50:42Z

I believe that the only other place where this would be a problem is cdk/drag-drop, but I'm not sure how we could generalize it well.

mmalerba

LGTM

…uch events `mat-slider` is set up so that it binds global `touchmove` and `touchend` events after dragging starts so that the user can drag while not keeping their finger right on top of the slider. The problem is that currently we only use the first touch from the list of touches which means that if the user is dragging more than one slider on a multi-touch device, they'll all have the same value. These changes add some logic that attributes the events to the slider that started the dragging sequence. Fixes angular#22614.

puschkin · 2021-05-20T17:43:37Z

Hey, I just want to check back what's blocking this PR from being merged.

angular-automatic-lock-bot · 2021-06-21T00:02:04Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release labels May 2, 2021

crisbeto requested a review from mmalerba May 2, 2021 14:20

crisbeto requested a review from devversion as a code owner May 2, 2021 14:20

google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 2, 2021

crisbeto mentioned this pull request May 2, 2021

bug(mat-slider): Multi touch doesn't work as expected #22614

Closed

puschkin reviewed May 2, 2021

View reviewed changes

crisbeto force-pushed the 22614/slider-multi-touch branch from ec732bc to 8272516 Compare May 5, 2021 18:46

mmalerba approved these changes May 6, 2021

View reviewed changes

mmalerba added the action: merge The PR is ready for merge by the caretaker label May 6, 2021

crisbeto force-pushed the 22614/slider-multi-touch branch from 8272516 to 63a5cd5 Compare May 9, 2021 09:41

devversion approved these changes May 10, 2021

View reviewed changes

wagnermaciel added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels May 21, 2021

wagnermaciel merged commit 2f40a8d into angular:master May 21, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(material/slider): incorrectly attributing touches for multiple touch events #22615

fix(material/slider): incorrectly attributing touches for multiple touch events #22615

crisbeto commented May 2, 2021 •

edited

puschkin commented May 2, 2021 •

edited

puschkin May 2, 2021

crisbeto May 2, 2021

puschkin May 2, 2021

puschkin May 2, 2021

crisbeto May 2, 2021

puschkin May 5, 2021

crisbeto May 5, 2021

puschkin May 6, 2021

puschkin May 6, 2021

crisbeto May 6, 2021

crisbeto commented May 2, 2021

puschkin commented May 2, 2021

crisbeto commented May 2, 2021

puschkin commented May 2, 2021

crisbeto commented May 2, 2021

puschkin commented May 6, 2021

crisbeto commented May 6, 2021

mmalerba left a comment

puschkin commented May 20, 2021

angular-automatic-lock-bot bot commented Jun 21, 2021

fix(material/slider): incorrectly attributing touches for multiple touch events #22615

fix(material/slider): incorrectly attributing touches for multiple touch events #22615

Conversation

crisbeto commented May 2, 2021 • edited

puschkin commented May 2, 2021 • edited

Steps to reproduce:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented May 2, 2021

puschkin commented May 2, 2021

crisbeto commented May 2, 2021

puschkin commented May 2, 2021

crisbeto commented May 2, 2021

puschkin commented May 6, 2021

crisbeto commented May 6, 2021

mmalerba left a comment

Choose a reason for hiding this comment

puschkin commented May 20, 2021

angular-automatic-lock-bot bot commented Jun 21, 2021

crisbeto commented May 2, 2021 •

edited

puschkin commented May 2, 2021 •

edited