Skip to content

[amp-story-player] Forward scrolling touch events to parent window#31051

Merged
Enriqe merged 12 commits intoampproject:masterfrom
Enriqe:player-scrolling-bug
Nov 19, 2020
Merged

[amp-story-player] Forward scrolling touch events to parent window#31051
Enriqe merged 12 commits intoampproject:masterfrom
Enriqe:player-scrolling-bug

Conversation

@Enriqe
Copy link
Copy Markdown
Contributor

@Enriqe Enriqe commented Nov 9, 2020

Closes #28009

Forwards vertical touch events from the iframe into scroll events for the parent window in order to allow scrolling.

Works well except that inertia scrolling doesn't really work since we are setting the scrolling coordinates via JS.

@Enriqe Enriqe requested a review from gmajoulet November 9, 2020 19:07
@Enriqe Enriqe self-assigned this Nov 9, 2020
@Enriqe
Copy link
Copy Markdown
Contributor Author

Enriqe commented Nov 9, 2020

Oops, forgot to take into account when there's a page attachment. Will update it in a bit.

@gmajoulet
Copy link
Copy Markdown
Contributor

Can you upload a demo? I'm curious to see how the no scroll inertia feels

@Enriqe
Copy link
Copy Markdown
Contributor Author

Enriqe commented Nov 10, 2020

As discussed offline I will experiment manually adding some inertia and testing how if feels and interacts with regular scrolling outside the player.

@amp-owners-bot
Copy link
Copy Markdown

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/amp-story-draggable-drawer.js

@Enriqe
Copy link
Copy Markdown
Contributor Author

Enriqe commented Nov 10, 2020

I introduced the smooth scrolling that @gmajoulet recommended and tweaked it a bit. I still need to play a bit more with the parameters but it's looking good. Here is a demo if you want to try it out on a real device as well. https://temp-player.web.app/examples/amp-story/player-local-stories.html

@Enriqe
Copy link
Copy Markdown
Contributor Author

Enriqe commented Nov 18, 2020

I've updated the PR with a new PageScroller class that encapsulates all the scrolling logic.

The basic principles of how it works are listed on the JSDoc. The implementation as well as the numbers are based from many different implementations/articles/papers I saw on the wild, so it might not feel as good as native scrolling, but it's as close as I could get while not complicating the code too much.

It supports the following features from native scrolling:

  • 1:1 scrolling (scroll while finger is dragging on the element)
  • Momentum scrolling (inertia after swiping rapidly)
  • Tap-to-stop (tapping while inertia is running will stop it).
  • Back-to-back fast swipes on the element will add up a multiplier and increase the offset.

It does NOT support:

  • Scrollbar (using win.scroll() does not show browser scroll bars).
  • Interaction between native and the PageScroller scrolling. Meaning if you scroll fast enough on the player to get some inertia scrolling, and then you swipe on the actual page, there might be some time where the actual page scroll will be unresponsive (because the PageScroller is still running).

The demo has been updated here (swipe to next story if you want to try out a story without a page attachment): https://temp-player.web.app/examples/amp-story/player-local-stories.html

PTAL @gmajoulet

@gmajoulet
Copy link
Copy Markdown
Contributor

Can you try the demo on Chrome mobile emulation? It doesn't really work for me :((

@gmajoulet gmajoulet requested review from mszylkowski and processprocess and removed request for gmajoulet November 18, 2020 18:31
@gmajoulet
Copy link
Copy Markdown
Contributor

to @mszylkowski and @processprocess for the review if/once the demo works

@mszylkowski
Copy link
Copy Markdown
Contributor

When I scroll the player just a few pixels, it scrolls the whole page all the way down as if I did a full scroll motion. Seems like the scroll makes the tap seem lower (because the page moves up) and takes that into the scroll... Does that happen for anyone else?

@Enriqe
Copy link
Copy Markdown
Contributor Author

Enriqe commented Nov 18, 2020

Sorry, I think I changed something recently that broke chrome emulation (probably when testing with a real device). Will comment back when it's ready.

@Enriqe
Copy link
Copy Markdown
Contributor Author

Enriqe commented Nov 19, 2020

Ok, I think demo should be fixed. You might need to clear your cache if testing on a real device to get the latest changes. PTAL @processprocess @mszylkowski.

@gmajoulet
Copy link
Copy Markdown
Contributor

The demo behavior is really great! I love that you can scroll to the end of the page attachment and then it scrolls the main page, just like a HTML scrollbox.

@amanintech
Copy link
Copy Markdown
Member

Thanks, feels great this coming live.
@choumx in #28009, it was mentioned that there will be some level of publisher controls. Are they done in this PR?
If yes, we can add it to the docs?

ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
…mpproject#31051)

* forward scrolling to parent window

* newline

* only stop propagation and preventDefault at appropiate places

* introduce smooth scrolling

* clean up stoppropagation and prevent default logic

* revert preventDefault changes

* implement pageScroller with cubic bezier functions

* change to easeOutQuartFn, add more docs

* fix multiplier

* use screenX/Y for horizontal swiping, clientX/Y for vertical. throttle scroll. fix types.

* revert sample

* fix type, revert cached winheight
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[amp-story-player] Player should allow scrolling when possible

5 participants