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

Pinch-zoom code broken on Android #22709

Open
Manishearth opened this issue Jan 17, 2019 · 4 comments
Open

Pinch-zoom code broken on Android #22709

Manishearth opened this issue Jan 17, 2019 · 4 comments
Labels

Comments

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Jan 17, 2019

When pinch-zooming on Android the page disappears after a while

I feel like this bug has been around for a while, and it's probably due to #22229. We try to handle pinch-zooming in both the compositing code and the Android code. Removing the Android code makes pinch-zooming not work at all, removing the compositing code for pinch-zoom fixes the issue.

Ideally I'd like all of this logic to live inside Servo since in some cases application code may DefaultPrevented touch events (e.g. panning code inside a canvas), and the Android code can't understand that. We should figure out what's wrong with the pinch code in compositing, and also move flings there too.

The compositing code is at https://github.com/servo/servo/blob/master/components/compositing/touch.rs

video

cc @paulrouget

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Jan 17, 2019

Pinch-zoom is working on my touch-enabled laptop, so something else is off with the Android code. It's not just the compositing code being broken.

@Manishearth Manishearth changed the title Pinch-zoom code broken Pinch-zoom code broken on Android Jan 17, 2019
@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Jan 17, 2019

Ideally I'd like all of this logic to live inside Servo

Raw touch events can be transformed into scrolling and gestures by Servo. Yes.

But in general, I'd rather have Android do the touch events -> gesture event work, this makes the behavior more consistent with the platform.

For example, Android can smoothen a gesture or add inertia. Or users might have configured something at the OS level to disable pinch to zoom (for accessibility) or added custom gestures for zooming.

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Jan 17, 2019

But in general, I'd rather have Android do the touch events -> gesture event work, this makes the behavior more consistent with the platform.

The problem is, Servo needs to be able to intercept these and decide when Android gets to treat them as gestures. I'm not sure if the return values of the touch event handlers is enough, or if it's easy to bubble this up from Servo at all

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Jan 18, 2019

I'm not sure if the return values of the touch event handlers is enough, or if it's easy to bubble this up from Servo at all

Yes. We had to deal with this kind of problem all the time with Gecko. It's not easy, but the event propagation spec and the Android widget APIs will let us do that. I'm ok if for now we let Servo handle everything, but in the future, I'm afraid this will bite us. I really don't want us to end up in the situation Gecko is in today.

There is a lot of back and forth communication when it comes to gestures. Scrolling is a great example of how both Servo and Android need to communicate a lot during a gesture: Android can't just do all the scrolling as it needs to know if an area is scrollable, it can be interrupted, and know the size of the scrollable area (to draw the overscrolling canvas), etc. And Servo can't do all the work itself as it needs to know how to compute the panning physics for this specific platform (it changes on the platform and the platform version) and need follow the user options and implement any accessibility feature that comes with the platform. It's a subtle dance :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.