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

amp-story and image handling #39707

Closed
jaygray0919 opened this issue Dec 20, 2023 · 4 comments · Fixed by #39806
Closed

amp-story and image handling #39707

jaygray0919 opened this issue Dec 20, 2023 · 4 comments · Fixed by #39806

Comments

@jaygray0919
Copy link

Description

issues with amp-img and amp-selector when used with amp-story

Reproduction Steps

we've found several issues with amp-img and amp-selector when used with amp-story

example #1
https://afdsi.com/_google/attachment-tabs-sans-amp-selector/
selector works using <img>
but is invalid for using <img> rather than <amp-ing>
https://search.google.com/test/amp/result?id=h3VCui8BwFBUGurMaydD7g

example #2
https://afdsi.com/_google/attachment-tabs-avec-amp-img/
valid
https://search.google.com/test/amp/result?id=JzZ5VjQqheO7nJL05PN3_w
but selector from #1 fails when using <amp-img>
when a tab is selected and browser refreshed, image will load
but tab selection fails

example#3
https://afdsi.com/_google/amp-story-avec-list/
valid
https://search.google.com/test/amp/result?id=lKDabZSd5rZUBf3rcFQd6A
but the attachment with amp-list of images fails

here is verification that the examples works stand-alone:
https://afdsi.com/164189302300000061/index.html
using
https://afdsi.com/164189302300000061/data.json

Relevant Logs

No response

Browser(s) Affected

Chrome, Firefox, Safari

OS(s) Affected

No response

Device(s) Affected

No response

AMP Version Affected

No response

@erwinmombay
Copy link
Member

assigning to @ychsieh to triage

@ychsieh
Copy link
Contributor

ychsieh commented Jan 22, 2024

I tried playing with it a little bit. But even with the <img> version(https://afdsi.com/_google/attachment-tabs-sans-amp-selector/), the tab selection would still be lost after refreshes.

For the amp-list inside the page-attachements, I do find this happened in other bugs reported long time ago. I am suspecting some optimization is done so images are not loaded at all in page-attachment. Would take another look at how amp-story-shopping avoid this.

@jaygray0919
Copy link
Author

We also need a generalized amp-story-shopping solution. Let us know how to help deliver a two-for-one (twofer).

@ychsieh
Copy link
Contributor

ychsieh commented Feb 7, 2024

amp-story-shopping reads the shopping config about the URLs and make those URLs the CSS background of individual elements. Therefore, it won't work for standard usage of amp-list inside attachment, which is a customized layout instead of fixed layout like the shopping ones.

The fix is out: #39806. Basically layoutCallback never gets called.

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

Successfully merging a pull request may close this issue.

3 participants