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

Feedback that lazy loading on iOS is sometimes broken #10267

Closed
cramforce opened this Issue Jul 5, 2017 · 23 comments

Comments

Projects
None yet
5 participants
@cramforce
Copy link
Member

cramforce commented Jul 5, 2017

I've heard in various places (Twitter, here in bugs) that lazy loading in iOS is sometimes broken. I believe @jpettitt mentioned at some point to have seen this.

We should spend some quality time trying to repro this.

@dvoytenko's font sizing bug could be a candidate, or some other measuring related issue. This seems bad enough to spend a day or 2 investigating.

@cramforce cramforce added this to the Fixit Week EOQ2 milestone Jul 5, 2017

@jpettitt

This comment has been minimized.

Copy link
Collaborator

jpettitt commented Jul 5, 2017

Subjectively it seems to happen on pages that are not in the browser cache. I've not been able to generate a solid repro case.

@aghassemi

This comment has been minimized.

Copy link
Member

aghassemi commented Jul 5, 2017

Anyone has one or two Urls that have shown this behaviour?

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Jul 5, 2017

@aghassemi It was reported for https://scotthelme.co.uk/revocation-is-broken/amp/

Testing with empty cache might be a good hint.

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Jul 5, 2017

@bazzadp

This comment has been minimized.

Copy link

bazzadp commented Jul 5, 2017

Seems worse in Twitter app. Loading same URL in Safari often seems to work, though I think I've seen it on Google search results in Safari too (couldn't be 100% certain on that though). Which is weird as I thought all apps on iOS used same rending engine as Safari app?

Twitter now automatically uses AMP URLs when they exist (in mobile only?) even if plain URL was used, so happening a lot more :-( And no link bar as Google cache is not used, so no easy way to revert to non-AMP version (other than click button to load in Safari and then edit URL).

@bazzadp

This comment has been minimized.

Copy link

bazzadp commented Jul 5, 2017

Reproduced repeatedly with that URL (https://scotthelme.co.uk/revocation-is-broken/amp/) on:

iPhone 6s (iOS 10.3.2) in Twitter app. Only first two images load. Cannot reproduce in Safari at the mo.

iPad Air (iOS 10.3.2) in Twitter app. Only first 6 images load. Cannot reproduce in Safari at the mo.

Both on Wifi with good internet connection.

@aghassemi

This comment has been minimized.

Copy link
Member

aghassemi commented Jul 5, 2017

Thanks @bazzadp, very helpful!

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Jul 5, 2017

@dvoytenko Could it be that this was caused by the same underlying cause as #10195

The example page does have a custom font and lots of text.

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Jul 6, 2017

@bazzadp Can you always repro? if you reload the page (via enter in URL bar), does it repro then?

@aghassemi

This comment has been minimized.

Copy link
Member

aghassemi commented Jul 6, 2017

This repros a good 50% of the time for me in Private mode on Safari/iOS 10.3.2. I have also been able to 100% repo using the followin approach:

1- load the url, scroll to the middle
2- reload, safari maintains the scroll position but now images won't load anymore.

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Jul 6, 2017

Adjusting priority to P0.

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Jul 6, 2017

I think it is a separate issue.

Forcing relayout when it repros does not fix it.

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Jul 6, 2017

Layout state is READY_FOR_LAYOUT and they seem to be measured correctly with repro.

@aghassemi

This comment has been minimized.

Copy link
Member

aghassemi commented Jul 6, 2017

seems font related, removing loading of the custom font (e.g. remove <link rel="stylesheet" type="text/css" href="https://fonts.googleapis.com/css?family=Merriweather:300,700,700italic,300italic|Open+Sans:700,600,400" />) fixed it.

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

dvoytenko commented Jul 6, 2017

Looking. Does seem like it could be font related.

@aghassemi

This comment has been minimized.

Copy link
Member

aghassemi commented Jul 6, 2017

@dvoytenko @cramforce I have been able to repro this using local proxy without Dima's recent fix for #10195 and not been able to repro with Dima's fix. Not 100% sure it is not an unlucky coincidence but I looks like #10221 has fixed this. I suggest we cherry-pick it on Canary (that fix missed the cut by few hours on Friday)

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Jul 6, 2017

Did some more testing. Pretty sure that this would be fixed by #10221. My comment above "Forcing relayout when it repros does not fix it." is wrong. It does fix it, same as resizing the window.

@bazzadp

This comment has been minimized.

Copy link

bazzadp commented Jul 6, 2017

So, just for my understanding, is the reason it's worse in Twitter App because it presumably doesn't use a browser cache like real Safari does, so font has to be downloaded each time?

Btw I'm very impressed with your response to this issue. I really wish I'd raised it here when I first saw it, instead of just getting more and more annoyed with it, and eventually taking that frustration to above Twitter rant. I do have some fundamental concerns with AMP, but your support of the product, your technical skills and the open way it is run - all of which are demonstrated in resolution of this bug - are definitely not part of those concerns! I will make every effort to get better at raising issues here in future. Thanks all.

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Jul 6, 2017

I don't have an iPhone with Twitter with me, but the in-app-browser definitely uses storage separate from Safari. The font issue discussed here happens when this is true:

  • AMP JS loaded (e.g. it was already in cache)
  • document.readyState reaches at least interactive
  • Fonts have not loaded yet

I suspect that differences between WebKit instances are random and just depend on whether they happen to be in the race-state from above or not. iOS 11 fixes the most important underlying bugs.

cramforce added a commit to cramforce/amphtml that referenced this issue Jul 6, 2017

Wait until doc-ready before scheduling font-wait.
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

Wait until doc-ready before scheduling font-wait.
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

Wait until doc-ready before scheduling font-wait. (#10279)
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
@dvoytenko

This comment has been minimized.

Copy link
Collaborator

dvoytenko commented Jul 6, 2017

For reference, here's the WebKit bug: https://bugs.webkit.org/show_bug.cgi?id=174031. It has already been fixed in Safari nightly. Hopefully it will make it into Safari 11 release. We found a workaround, but it's definitely better with the native fix.

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Jul 6, 2017

This should now be fixed after going to https://cdn.ampproject.org/experiments.html and opting into "Dev Channel".

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Jul 6, 2017

We also patched this into last week's canary, so it will go to everyone soon.

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Jul 6, 2017

@aghassemi Could you make sure it is fixed with the canary?

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