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

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

Closed
puschkin opened this issue May 2, 2021 · 6 comments · Fixed by #22615
Closed

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

puschkin opened this issue May 2, 2021 · 6 comments · Fixed by #22615
Assignees
Labels
area: material/slider P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent

Comments

@puschkin
Copy link

puschkin commented May 2, 2021

I'm trying to place several mat-sliders next to each other and be able to adjust them with multi-touch.

Problem is that, as soon as I start dragging a second fader, the fader gets the values of the other fader.

Reproduction

Use StackBlitz to reproduce your issue:

Steps to reproduce:

  1. open application (stackblitz, link see above) on a multi-touch enabled device, e.g. smartphone (I tried with android / chrome and on a multi touch screen on linux /chrome + firefox)
  2. move first slider by touching it. don't lift the finger
  3. move the second slider by touching it, also don't lift the finger

Expected Behavior

The value of the slider is adjusted independently

Actual Behavior

The second slider always has the same value as the first slider.

Environment

  • Angular: 11.2.11
  • CDK/Material: 11.2.11
  • Browser(s): Chrome, Firefox
  • Operating System (e.g. Windows, macOS, Ubuntu): ArchLinux, Android

Investigation

I already figured out why this bug appears and made an attempt to fix it - which seems to work - at least in my use case.

The problem is, that on the touch events, there is no distinction if the event is for this particular fader or not.
With my PR, I'm adding a filter before those events and only choose the targetTouch that is relevant for that fader.

Here is the Pull request (first only against my own fork)
https://github.com/puschkin/components/pull/1

@puschkin puschkin added the needs triage This issue needs to be triaged by the team label May 2, 2021
@crisbeto crisbeto self-assigned this May 2, 2021
@crisbeto crisbeto added area: material/slider has pr P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent and removed needs triage This issue needs to be triaged by the team labels May 2, 2021
@puschkin
Copy link
Author

puschkin commented May 2, 2021

I did some further investigation of the touch events and their implementation in different browsers:
I am additionally using the targetTouches and the target properties of the TouchEvent.

https://developer.mozilla.org/en-US/docs/Web/API/TouchEvent/targetTouches
https://developer.mozilla.org/en-US/docs/Web/API/Touch/target

They seem both to be implemented in most browsers, except for InternetExplorer and Safari (does that mean, also other Webkit based browsers?) - So this would either need another strategy how to determine the right touch events or a fallback if those properties don't exist.

crisbeto added a commit to crisbeto/material2 that referenced this issue May 2, 2021
…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.
@crisbeto
Copy link
Member

crisbeto commented May 2, 2021

I don't think that the approach in the linked PR would work, because the target of the event is only going to match if the user's pointer is on top of the slider. We bind an event on the document so that the user can start dragging from the slider, but their finger can be moved away from it afterwards.

I've submitted a fix at #22615 that matches the touchmove and touchend event to the correct slider by looking at the unique ID provided by the browser.

@puschkin
Copy link
Author

puschkin commented May 2, 2021

I don't think that the approach in the linked PR would work, because the target of the event is only going to match if the user's pointer is on top of the slider. We bind an event on the document so that the user can start dragging from the slider, but their finger can be moved away from it afterwards.

At least in chrome and firefox (android and linux), I didn't encounter issues with the 'target', even when I move the fingers away from the slider. I think that the target doesn't change during a touch chain (touchStart, n x touchMove, touchEnd/Cancel).

I've submitted a fix at #22615 that matches the touchmove and touchend event to the correct slider by looking at the unique ID provided by the browser.

I tried a similar approach, it also worked, seems a bit more overhead and was not that stable at my side.
I'll test and review your code!
Thanks for your fast reaction on that topic!

BTW, is there a good guide, how to develop on this repository and use a development version in a project created e.g. with angular cli?
My naive approach was to clone the repo, run yarn run build
Then go to my application and replace the @angular/material version in the package.json with file:/path/to/components/dist/release/material, but then, I got several compile issues.
First, I had to manually update all dependencies in my project , to match all peer-dependencies in the angular/material repo
then, I had errors when I ran "ng serve"
E.g.:
Error: [...]/components/src/material/slider/slider.css:1:10: Unknown word

@crisbeto
Copy link
Member

crisbeto commented May 2, 2021

I was testing it on an iOS device and it seemed like the target would sometimes be the event I was touching outside of the slider, but I think that the identifier would be a more stable solution anyway. Matching the identifier to a touch object does involve a linear-time search, but at the worst case, the list of touches will be 5 which is still quick to go through.

As for developing locally, there's more information at https://github.com/angular/components/blob/master/DEV_ENVIRONMENT.md.

@puschkin
Copy link
Author

puschkin commented May 2, 2021

thanks @crisbeto
Yes, probably you're right that the identifier is the better solution.
Sure, I also wouldn't see any performance issue there.

crisbeto added a commit to crisbeto/material2 that referenced this issue May 5, 2021
…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.
crisbeto added a commit to crisbeto/material2 that referenced this issue May 9, 2021
…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.
wagnermaciel pushed a commit that referenced this issue May 21, 2021
…uch events (#22615)

`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 #22614.
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: material/slider P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants