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

AMP documents should have touch-action: pan-y #4820

Closed
RByers opened this issue Sep 2, 2016 · 8 comments
Closed

AMP documents should have touch-action: pan-y #4820

RByers opened this issue Sep 2, 2016 · 8 comments

Comments

@RByers
Copy link

RByers commented Sep 2, 2016

When an AMP document is hosted in a swipable container like on Google Search, they must disable horizontal scrolling to let the container handle the touch events. They already have a html { overflow-x: hidden!important} rule, but that's not quite enough. If the container is using passive touch listeners (so as not to interfere with scroll performance) or pointer events it won't have any way to tell the AMP document not to scroll.

How do we reproduce the issue?

  1. Enable Chrome's experimental "forced passive document touch listeners" intervention
    • In Chrome 54 (current dev) at chrome://flags/#document-passive-event-listeners.
    • This is already shipping as a 50% trial for dev-channel users and is expected to ramp up to stable in Chrome 55 or 56 (Chrome 55 or 56)
    • In Chrome 53 you can get similar behavior by setting chrome://flags/#passive-listener-default to "document level true". But I suggest testing with Chrome 54+
  2. Click on an AMP document in google search results
  3. Drag your finger horizontally to start switching documents
  4. Without lifting start dragging vertically.

With the intervention enabled you'll see the AMP document start scrolling vertically and the horizontal carousel motion become janky (touchmove throttled during an active scroll). You'll also see messages like the following in the console:

Unable to preventDefault inside passive event listener invocation.
Ignored attempt to cancel a touchmove event with cancelable=false, for example because scrolling is in progress and cannot be interrupted.

What browsers are affected?

Just with Chrome's intervention on Android for now. But Other AMP containers could have similar problems in other browsers (eg. if they wanted to use pointer events on Edge, or opt-in to passive touch listeners in Mobile Safari or Chrome).

Which AMP version is affected?

Version 1472690078859

Suggested fix

The AMP document should make it's intention to support only vertical scrolling explicit to the browser with a rule like html {touch-action: pan-y;}. Note that this doesn't mean the document cannot contain a sub-scroller that scrolls horizontally, just that the AMP document should not handle any horizontal scroll gestures itself. Adding this style rule in devtools fixes the issue for me.

@RByers
Copy link
Author

RByers commented Sep 2, 2016

/cc @dtapuska @tdresser @KenjiBaheux who are driving the Chrome intervention.

@cramforce
Copy link
Member

Lets do it.

@dvoytenko
Copy link
Contributor

@RByers to confirm, html {touch-action: pan-y;} will not interfere with page touch zoom?

@RByers
Copy link
Author

RByers commented Sep 9, 2016

@RByers to confirm, html {touch-action: pan-y;} will not interfere with page touch zoom?

It does actually - disables pinch-zoom completely! But I thought AMP documents (at least the ones I've seen) always have pinch-zoom disabled due to minimum-scale=1,maximum-scale=1 viewport directives? Is that not always the case? Sorry I didn't think to ask.

The right fix is to instead use touch-action: pan-y pinch-zoom but that's not yet shipping in Chrome. We've recently decided to ignore the silly standardization issues and try to push ahead with shipping anyway. If this is important for you then @dtapuska may be able to bump the priority of his work here.

@dvoytenko
Copy link
Contributor

No, that's not the case. We only require minimum-scale=1. But that's ok. I can only enable this feature when AMP is iframed.

Next question: what kind of effect does pan-y has on a web view, e.g. in Android?

@RByers
Copy link
Author

RByers commented Sep 9, 2016

Ok pan-y pinch-zoom will work on Edge and soon Chrome so perhaps it's worth feature-testing for it? i.e. just set it first to pan-y then pan-y pinch-zoom - the 2nd will be a no-op if it's not supported.

The effect on Android WebView should be identical (it's all the same code) - make any horizontal scrolls over top of the WebView just send touch events without scrolling. I don't know how composition of scrolling works when a WebView is contained inside a Java ScrollView but I can loop in the right engineers if that's a potential concern. I should admit I haven't personally tested touch-action behavior inside a WebView.

@dvoytenko
Copy link
Contributor

Ok. To start with I will only do this for AMP documents that are embedded.

@ericlindley-g ericlindley-g added this to the Current milestone Sep 14, 2016
@dvoytenko
Copy link
Contributor

/cc @RByers @cramforce @ericlindley-g

This has been launched last week.

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

No branches or pull requests

5 participants