AMP runtime does not honor height constraints correctly for Responsive Layouts

ayy1ma commented 2 years ago

Description

It seems the common AMP runtime is not honoring the height property for amp-list (when using amp-template and mustache) under following scenarios -

Inconsistent (breaks some times) height of the module displayed when the module is loaded. It seems the height is incorrect and leads to module getting cut sometimes (reproducible in the test code attached). It seems the moment I do any DOM event, like click Hide/Show it shows the complete module (including the monkey emoji) -

Sometimes using the code, if you go to http://127.0.0.1:5000/static/test.html

Note in the above module the emoji - see-no-evil-monkey does not shows up

But after a few refresh, the full module shows up with see-no-evil-monkey as the last image as shown below -

After the responsive layout has been zoomed back to original width, the height of the responsive module stays same and does not collapse back (also reproducible in the test code attached)

ie: After reducing the width of the chrome window to the minimum and expanding back to original size, there is a huge gap at the bottom of the module as show below -

Reproduction Steps

Use the attached code, and follow the instructions in README.md (you would need Python 3.6 and flask as mentioned in README)

Extract the following zip - how_tall_am_i.zip

how_tall_am_i.zip

Relevant Logs

Instructions in README.md (browser based testing)

Browser(s) Affected

Chrome, Firefox, Safari

OS(s) Affected

Mac

Device(s) Affected

Mac

AMP Version Affected

No response

ayy1ma commented 2 years ago

On #1 I see following warning in the chrome console when the height get's cut off. It seems the moment I do any DOM event, like click Hide/Show it shows the complete module (including the monkey emoji) -

ayy1ma commented 2 years ago

Screen grab of first issue -

https://user-images.githubusercontent.com/97769160/149602690-4b5daa96-cbcf-4fbc-9e9f-ab2f2b24c6e1.mov

ayy1ma commented 2 years ago

Screen grab of second issue -

https://user-images.githubusercontent.com/97769160/149602702-37569f82-8438-4590-a368-b65537666fd7.mov

zhangsu commented 2 years ago

First issue

I think this is working as intended and not a bug. By design, AMP forbids content jumping caused by layout shifts. If auto-expanding the amp-list would result in content below the amp-list pushed down, then AMP will not auto-expand it and instead will render the amp-list in its container with the initial size determined by the initial layout you specify on the amp-list. In this case, the amp-list has responsive layout with identical initial width and height, meaning that it will be initially rendered in a perfect square filling up its parent container.

In your example, as illustrated by the first screen grab, there's some content (text and rectangle images with rounded corners) beneath the bottom of the amp-list. The bottom in this case refers to the initial bottom of the perfect square amp-list container before it auto-expanded itself. The first few refreshes you did in the video that resulted in the amp-list auto-expanding itself were done in one of the following scenarios:

The entire amp-list is off-screen
The bottom of the amp-list is off-screen
The bottom of the amp-list is near the bottom of the viewport or browser window

The amp-list is allowed to auto-expand itself In these scenarios because doing that will either not cause any content jumping at all or only cause very minor content jumping that the user won't care about.

If the bottom of the amp-list is off-screen, then auto-expanding the amp-list will cause the content beneath the amp-list to be also pushed down off-screen, and the user will never see any content being pushed down because it's off-screen.
If the entire amp-list is off-screen, then it's effectively the same as the above.
If the bottom of the amp-list is near the bottom of the viewport, then there's only so much content that can be visibly pushed down. The rationale in this case is that those tiny portion of content that can get visibly pushed down is not being consumed by the user anyway, so it's okay for them to be visually jumping. The end goal is to prevent the frustrating user experience where the user is trying to read or interact with some elements of the UI but then the elements jump away in the next second.

In the video, the refresh that resulted in the amp-list not auto-expanding was done when the amp-list doesn't meet any of the conditions above. As a result, the amp-list was not auto-expanded, because it would otherwise result in a visual content jump where the text and rectangles with rounded corners were first displayed at one placed and then suddenly jumped to another when the amp-list loads.

This behavior is documented in the amp-list documentation:

Optionally, the component can contain an element with the overflow attribute. AMP will display this element if all of the following conditions are met:

The contents rendered into the amp-list exceed its specified size.

The bottom of amp-list is within the viewport.

The bottom of amp-list is not near the bottom of the page (defined as the minimum of either the bottom 15% of the document or the bottom 1000px)

If amp-list is outside the viewport, it will be automatically expanded.

https://amp.dev/documentation/components/amp-list/?format=email#specifying-an-overflow

The documentation there talks about when the "overflow" element appears, but it's essentially documenting when the amp-list auto-expands itself during initial load of the page. The amp-list auto-expands itself during initial load if and only if the overflow element does not appear.

You can specify the overflow element in cases like this. When the user clicks on the overflow element, it will expand the amp-list. The idea here is that content jumping as a result of a user gesture is okay, because the user does expect something to change in the UI after performing some actions. It's only a bad UX when the content jumps without the user gesturing it. This is also why you were able to expand the amp-list by hiding and re-showing the amp-list, as illustrated in the video.

Another thing to note here is that amp-list can also auto-expand itself if there's no content beneath it, which is documented in the following section:

Notice however, that the typical placement of elements at the bottom of the document almost always guarantees that the AMP runtime can resize them.

https://amp.dev/documentation/components/amp-list/?format=email#displaying-a-dynamic-list

This is pretty much the only reliable way to ensure that amp-list auto-expands, since you can't predict the size of the user's viewport beforehand and thus cannot design the UI in a way that always meets the viewport conditions documented above.

Second issue

/to @samouri Not sure if there's GitHub issue tracking this but I think it's basically b/110198135.

OP: I think you can actually work around this issue by switching from responsive layout to container layout, but of course it also depends on other requirements you may need. <amp-list layout=container> isn't documented at the moment, but it's already being used by email senders. See https://github.com/ampproject/amphtml/issues/26873 for more details.

stale[bot] commented 1 year 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.

ampproject / amphtml