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

Set pan-y on root to enable passive touch handlers #4895

Merged
merged 4 commits into from
Sep 11, 2016

Conversation

dvoytenko
Copy link
Contributor

Partial for #4820.

/cc @RByers @cramforce
/cc @ericlindley-g heads up: we will need testing help to launch this.

// TODO(dvoytenko, #4894): Cleanup the experiment by moving this to CSS:
// `html {touch-action: pan-y}`
if (isExperimentOn(this.win_, 'pan-y')) {
setStyle(this.win_.document.documentElement, 'touch-action', 'pan-y');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment for the why

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@cramforce
Copy link
Member

LGTM

// `html {touch-action: pan-y}` (will require adding `amp-embedded` class).
// The enables passive touch handlers, e.g. for document swipe, since they
// no will longer need to try to cancel vertical scrolls during swipes.
if (viewer.isEmbedded() && isExperimentOn(this.win_, 'pan-y')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a harm in doing it in non-embed cases?
(IMO, the less we differ in embed vs non-embed modes, the better. For example we may introduce some code that having pan-y breaks it, but since we primarily test in non-embed case during dev, so it might go unnoticed for longer than it should)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll lose pinch-zoom on Chrome.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a good reason. Maybe document that and mention that in embed mode we already have lost pinch zoom, so it won't be a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Added more docs. Also, really, this is only needed when doc is embedded - otherwise there's no document swipe or things like that.

@dvoytenko dvoytenko merged commit 6d99003 into ampproject:master Sep 11, 2016
@dvoytenko dvoytenko deleted the pany1 branch September 11, 2016 00:44
dreamofabear pushed a commit to dreamofabear/amphtml that referenced this pull request Sep 16, 2016
* Set pan-y on root to enable passive touch handlers

* more docs

* Only set touch-action for embedded cases

* more docs
mityaha pushed a commit to ooyala/amphtml that referenced this pull request Nov 30, 2016
* Set pan-y on root to enable passive touch handlers

* more docs

* Only set touch-action for embedded cases

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

Successfully merging this pull request may close these issues.

3 participants