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

I2I: amp-fx=float-in-(top|bottom) #20881

Closed
alanorozco opened this issue Feb 15, 2019 · 4 comments
Closed

I2I: amp-fx=float-in-(top|bottom) #20881

alanorozco opened this issue Feb 15, 2019 · 4 comments
Assignees
Labels
INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code P2: Soon Type: Feature Request
Milestone

Comments

@alanorozco
Copy link
Member

alanorozco commented Feb 15, 2019

Background

#20125 requests a feature to track scrolling in order to hide/show headers:

A way to dispatch events based on the user's scroll direction. This would be great for animations, such as a header that's removed when the user scrolls down and reappears when the user scroll up again. Might be nice to be able to put a range of scrolling: on:scrollUp.100vh.event for full screen scrolling and on:scrollUp.1vh.event for any user scroll interaction.

This approach more or less works, but it has some drawbacks and open questions that the alternative described here would eliminate.

Goal

To provide an independent component for element toggling on scroll for performance, UX and DX.

Issues with event-trigger approach

amp-animation usage and definition

The main use-case for this feature is presumably toggled headers on scroll up/down, similar to behavior on the Google SERP viewer, Safari/Chrome for iOS or Chrome for Android. In order to build such a feature using the method described in #20125, the author would need to define three separate things: action handlers for scrollUp/scrollDown, an amp-animation for scrollDown and an amp-animation for scrollUp.

Not only is this tedious but it's also fragile. In order to create an e.g. a slide-to-toggle-header animation, the absolute value of the translate has to match the the header's height and be included twice; once for each animation. This can obviously lead to mismatching and would be better off being calculated dynamically, similar to how amp-story handles animation presets.

Responsive designs are also an issue in this case, since headers could vary by height, so multiple elements and animations would have to be included for each media query configuration, aided by a combination of transparency and pointer-events: none CSS nightmare.

The amp-animation requirement is also a performance issue, since the extension is rather large (currently at 33.627kB gzipped) and we could just simply evade it and perform simple CSS animations instead.

Leaky abstraction

Triggering simple events on scroll may lead authors to hand-roll behavior where better performing alternatives are available, e.g. amp-position-observer, even if they're low-trust. Examples are scroll-bound or scroll-triggered animations, where listening to an element's position is a better pattern than listening to scroll events directly.

UX

In a significant number of cases, AMPHTML pages are rendered in the Google SERP viewer, which has the same header-toggle-on-scroll behavior. The logic for deciding when to toggle is not strictly simple (internal: go/amp-viewer-scroll), and is bound to hard-coded thresholds (internal: go/amp-viewer-scroll-thresholds). If the AMP-side logic or thresholds do not match the viewer's, independent headers would be toggled at different times, which would be extremely awkward. If we get rid of the threshold configuration from the event-based approach this is not strictly an issue, but we would be exposing a misnamed/misleading API instead.

Semantics & Encapsulation

Similar to misnamed APIs, we have to consider where to put this.

  • Core. Bloat on render blocking script and attaching scroll handlers when we don't need to.

  • amp-position-observer. It's an extension, so that's good. However the toggle-on-scroll feature doesn't quite fit with its model: PositionObserver either uses available browser APIs or polyfills that are element-bound, not offset-bound. This feature would be completely parallel to the rest of the component, so it's a lose-lose: people who only want position observing will get scroll triggers regardless, and people who only want scroll triggers will get position observing too.

  • amp-fx-collection. Makes more sense with the use and is more likely to be included than amp-position-observer by itself.

amp-fx-collection

Including a new fx preset allows us to encapsulate the desired behavior with clear boundaries and without bloat. It also enables some invariants that are good for UX and DX:

  • No overloading. Scroll handlers will be attached for this feature only, without exposing the events themselves. This ensures that authors don't hand-roll poor-performing and/or undesired behavior (like scroll-triggered scene animations or, god forbid, interstitials).

  • Sized & positioned. Headers will always be at the top. Bottom navigation will always be at the bottom. Toggled elements are restricted to a certain height relative to viewport so they don't hijack the document. Animation offsets are dynamically calculated, so everything is always translated and positioned correctly, regardless of media queries and without hard-coded offset literals.

High-level usage

The amp-fx attribute is set with either float-in-top or float-in-bottom.

<!-- Header will slide in and out as the user scrolls up/down -->
<header amp-fx="float-in-top">
  <h1>⚡ Ampersand News</h1>
</header>

On build time, amp-fx-collection async'ly ensures that target:

  • is present
  • is a block element
  • is position: fixed
  • is overflow: hidden
  • is either top: 0 or bottom: 0 when position="bottom"
  • is no taller than a certain viewport percentage

Otherwise, it will do nothing.

Triggering

The component attaches scroll handlers that match the Google SERP viewer's header-toggling logic, and thresholds for consistency. On toggle, it calculates animation offsets corresponding to the target element's height and triggers a time-bound animation.

Performance

Multiple amp-fx="float-in-*" elements can live in one page for different targets. A single Observable should be instantiated from a single scroll handle that all the elements will listen to.

Opportunities

In Google SERP cases, we already receive a postMessage that tells us to toggle, including duration and curve. No additional scroll handlers would be needed and toggling logic would always match, regardless of viewer changes.

Milestones

  1. Self-contained scroll logic is the common case, so build that first.
  2. Receiving toggle message from Google SERP viewer is an optimization and should come later.

Drawbacks

It's up to the authors not to do this:

image

We can consider restricting the amount of toggle elements that are alive at once. Alas we don't currently do anything more sophisticated to disregard excessive floating elements, so relying on good faith that including several floating bars is bad UX would simply preserve the status quo.

/cc @ampproject/wg-ui-and-a11y @aghassemi @nainar @CrystalOnScript

@alanorozco alanorozco added the INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code label Feb 15, 2019
@alanorozco alanorozco self-assigned this Feb 15, 2019
@aghassemi aghassemi added this to the New FRs milestone Feb 16, 2019
@alanorozco alanorozco changed the title I2I: amp-scroll-toggle I2I: amp-fx=float-in-(top|bottom) Feb 20, 2019
@cathyxz
Copy link
Contributor

cathyxz commented Feb 22, 2019

I have a use case for #20125, which has been closed in favor of this issue. For context, it's what I outlined here: #21016 (comment)

img_0276

I can partially implement this with position observer, amp-animation, translateY and a position https://codepen.io/cathyxz/pen/YBmKBq. So if we remove the animation exit, and instead hide this sticky footer on scrollDown, I think that should get us all of the way there.

Is scrollUp and scrollDown still something we're looking at adding? Could they be used to trigger animations?

@alanorozco
Copy link
Member Author

I feel strongly that we shouldn't expose scrollUp and scrollDown directly. However, I think your use case is valid. Let's discuss offline or setup a meeting on monday?

@alanorozco
Copy link
Member Author

alanorozco commented Feb 25, 2019

(@cathyxz Sorry for leaving this hanging before, I filed #21044 with an alternative. We can still chat on Monday.)

alanorozco added a commit that referenced this issue Feb 26, 2019
Partial for #20881.

Base implementation for `amp-fx="float-in-(top|bottom)"`. Slides elements in-and-out accordingly. Matches behavior of Google viewer's sliding header, see http://go/amp-viewer-scroll and http://go/amp-viewer-scroll-thresholds internally.

Demo: https://scroll-toggle.glitch.me/

Also:

* refactors `amp-fx-collection` for types and size. (~10% down from total bundle)
* adds a bunch of string → types tests (and killed a splice bug, thanks @amphtml-lgtm-bot!)
noranazmy pushed a commit to noranazmy/amphtml that referenced this issue Mar 22, 2019
Partial for ampproject#20881.

Base implementation for `amp-fx="float-in-(top|bottom)"`. Slides elements in-and-out accordingly. Matches behavior of Google viewer's sliding header, see http://go/amp-viewer-scroll and http://go/amp-viewer-scroll-thresholds internally.

Demo: https://scroll-toggle.glitch.me/

Also:

* refactors `amp-fx-collection` for types and size. (~10% down from total bundle)
* adds a bunch of string → types tests (and killed a splice bug, thanks @amphtml-lgtm-bot!)
bramanudom pushed a commit to bramanudom/amphtml that referenced this issue Mar 22, 2019
Partial for ampproject#20881.

Base implementation for `amp-fx="float-in-(top|bottom)"`. Slides elements in-and-out accordingly. Matches behavior of Google viewer's sliding header, see http://go/amp-viewer-scroll and http://go/amp-viewer-scroll-thresholds internally.

Demo: https://scroll-toggle.glitch.me/

Also:

* refactors `amp-fx-collection` for types and size. (~10% down from total bundle)
* adds a bunch of string → types tests (and killed a splice bug, thanks @amphtml-lgtm-bot!)
@aghassemi
Copy link
Contributor

Implemented!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code P2: Soon Type: Feature Request
Projects
None yet
Development

No branches or pull requests

3 participants