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-list appears not to load on Safari in some cases #10195

Closed
dvoytenko opened this issue Jun 29, 2017 · 14 comments
Closed

amp-list appears not to load on Safari in some cases #10195

dvoytenko opened this issue Jun 29, 2017 · 14 comments
Assignees

Comments

@dvoytenko
Copy link
Contributor

From @nzeeshan

I am using amp-mustache to render a list of links on my amp page. It works fine on Android devices, however on Safari iOS, it doesn't work. Here's a screenshot of what I get when accessing the page through Safari.

AMP page URL
JSON URL

I am using full paths, not even relative paths and not sure how to resolve this. What am I doing wrong and why does it only work on Android?

@dvoytenko
Copy link
Contributor Author

My initial sense is that, for some reason, Safari measures amp-list incorrectly.

@nzeeshan
Copy link

Hi @dvoytenko, it actually doesn't work on Chrome iOS either.

@dvoytenko
Copy link
Contributor Author

@nzeeshan That makes sense - they use the same browser engine.

@nzeeshan
Copy link

@dvoytenko, Got it. But what should I do to resolve this? Any guidance? The #3405 ticket states that it's a CORS issue. And then I notice you mentioning that it's easily solvable. But don't know how to make this adjustment on my AMP page.

@dvoytenko
Copy link
Contributor Author

This is actually a very peculiar case. It looks like this has not to do with CORS or amp-list itself, but with the way Safari treats @font-face. Basically, the measurements on font-family: "Akkurat" initially on Safari are wildly out of wack - the text is reported to take up to 3x more space than it actually does. This is rather odd: the font/line heights are available and thus always correctly calculated. But the default font width is very outsized. This throws off a lot of measurements. I'm not yet sure how to address this - it's definitely something we want to fix on AMP runtime level. I don't yet see good signals to remeasure things reliably, but I'll keep researching this. Unfortunately, I don't yet have a good advice for you aside from postponing using @font-face until I find the solution on Safari. Weirdly, <link rel="stylesheet"> does not appear to exhibit the same problem.

/cc @cramforce @jridgewell

@dvoytenko dvoytenko assigned dvoytenko and unassigned dreamofabear Jun 30, 2017
@dvoytenko
Copy link
Contributor Author

I think I got to the bottom of this. Basically, Safari behavior is broken with font-face due to these two issues:
https://bugs.webkit.org/show_bug.cgi?id=174030
https://bugs.webkit.org/show_bug.cgi?id=174031

I believe I can work it around. Please stay tuned. Interestingly enough, the 174031 appears to have been fixed in Safari 11.

@dvoytenko
Copy link
Contributor Author

@nzeeshan Ok. I believe this is fixed now for your case. This should be in canary next week.

cramforce added a commit to cramforce/amphtml that referenced this issue Jul 6, 2017
The 3s timeout counts from the time the font is requested which cannot be done before the doc is ready to be styled.

Follow up for ampproject#10195 and ampproject#10267
cramforce added a commit to cramforce/amphtml that referenced this issue Jul 6, 2017
The 3s timeout counts from the time the font is requested which cannot be done before the doc is ready to be styled.

Follow up for ampproject#10195 and ampproject#10267
cramforce added a commit that referenced this issue Jul 6, 2017
The 3s timeout counts from the time the font is requested which cannot be done before the doc is ready to be styled.

Follow up for #10195 and #10267
@nzeeshan
Copy link

nzeeshan commented Aug 9, 2017

Hi @dvoytenko, did your fix every made to production? Still happens on Safari as well as Chrome for iOS ... the list of links don't render.

@dvoytenko
Copy link
Contributor Author

@nzeeshan On the same https://www.bnymellon.com/us/en/our-thinking/amp/swimming-against-the-tide.jsp URL? I've been hitting it for some time and it consistently succeeds. Can you clarify?

@morsssss
Copy link
Contributor

We're seeing this now at this URL, in the mobile simulator in Chrome:

https://www.google.com/amp/s/www.bnymellon.com/us/en/our-thinking/amp/why-do-earnings-seasons-matter.jsp

Yes, I only see this in the Google AMP Viewer.

Look at the bottom of the page, where a bit of the template appears: {{title}}

It works normally here:

https://www.bnymellon.com/us/en/our-thinking/amp/why-do-earnings-seasons-matter.jsp

@dvoytenko
Copy link
Contributor Author

Ok, got it. It appears that somehow template is empty on the cache. This might be due to some cached version being incorrect, but also possible to be due to cache bug. We are looking asap.

Internal tracking b/65642597

@dvoytenko
Copy link
Contributor Author

Alright. This turns out to be due to <p> element containing <template> and eventually <div>. This touches on a slightly ambiguous part of HTML parser and it breaks our parser. We are addressing this, but in the meantime, this can be fixed by changing the <p> above <amp-list> element to a <div>.

@nzeeshan Could you please try that and lmk?

@dvoytenko
Copy link
Contributor Author

To close a loop on this issue: this has been fixed and verified.

@arefLA
Copy link

arefLA commented Jun 16, 2019

hi guys,
i had the same problem and i tried almost anything , the amp-list in safari 10 doesnt load the json endpoint,
but the solution for me is to add
credentials="include" into my amp-list tag:

the final amp-list:
<amp-list credentials="include" id="smallcartsList" width="auto" height="@HeightRequest" [height]="CurrentCartList.items.length == 0 ? 5 : 50 * CurrentCartList.items.length" media="(max-width: 1199px)" layout="fixed-height" binding="no" src="/API/CartList/GetCartsList" [src]="cartlistsource" template="cartlistdetail"> </amp-list>

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

No branches or pull requests

5 participants