ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

Bug: amp-live-list scrolls scroll does not account for AMP Viewer banner #6973

Closed ericlindley-g closed 3 years ago

ericlindley-g commented 7 years ago

To repro:

Open a document using amp-live-list in the Google AMP Viewer context Tap for an update Notice that the document scrolls to the top, minus the height of the banner (top of most recent update is cut off by the banner)

rbp15 commented 7 years ago

@ericlindley-g If I say "AMP Viewer" -- I take it that is a more easily understood term for what I've been calling the production carousel?

ericlindley-g commented 7 years ago

@randallbpotter15 — Yep, that's essentially right. The Google AMP Viewer is what will open (and display an AMP) if you tap on an item in a carousel on google.com that contains AMPs, but the Viewer will also open if you tap on an AMPd "blue link" on the page (which isn't in a carousel).

erwinmombay commented 7 years ago

@ericlindley-g will look into this tomorrow. you mind showing me a replication?

ericlindley-g commented 7 years ago

Interesting — can't repro in Mac/desktop/Chrome mobile emulator, but can repro on iPhone 6s / iOS 10 / Chrome at this link (or any live blog)

Before scroll:

screen shot 2017-01-19 at 12 03 58 pm

Expected behavior after scroll:

img_2078

Actual behavior After scroll:

img_2077

erwinmombay commented 7 years ago

so this is caused because the viewers header is hidden before the scroll and then visible after the scroll up (so the calculation is incorrect). @dvoytenko suggested that we probably shouldn't display the header again on programmatic scrolls (non user initiated).

wdyt @ericlindley-g

ericlindley-g commented 7 years ago

Is this the same regardless of context? That is, does it behave the same in browser/platform combos that don't hide the header? I ask because I agree that we probably shouldn't display the header on programmatic scrolls (though it may depend on what triggers the scroll — this amp-live-list case feels pretty clear that we shouldn't re-show the header) — but this one fix may not fix the bug across all platform/browser combos.

QES commented 7 years ago

Noticed that this is a very similar issue to the one I reported in https://github.com/ampproject/amphtml/issues/7949 but in a different content and a more general one in my report there is a know header getting in the way with a known size that could be worked around if there was a way to specify where the roll went to. In this example the added ampcache header makes this more noticeable.

Creating a generic solution is non-trivial as knowing the size of the header before hand is complex, but for sites with header elements in place having a way to force the scroll to a fixed place might allow for it to be solved in other ways.

Allowing sites to use some kind of DIV/SPAN tab with an ID that matches the amp-live-list one might provide a way if it exists to scroll to a predetermined place on the page.

ampprojectbot commented 6 years ago

This issue hasn't been updated in awhile. @erwinmombay Do you have any updates?

ampprojectbot commented 6 years ago

This issue hasn't been updated in awhile. @erwinmombay Do you have any updates?

ampprojectbot commented 6 years ago

This issue hasn't been updated in awhile. @erwinmombay Do you have any updates?

shimonenator commented 4 years ago

Are there any plans to address this at some point?

erwinmombay commented 4 years ago

@nainar do we have anyone in ui wg that owns live list these days?

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.