search

ampproject / amphtml

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

`amp-font.js` shows missing fonts as having been loaded when run under phantomjs #10377

Closed rsimha closed 6 years ago

rsimha commented 7 years ago

When the fonts on the page are missing, the text sometimes remains looks blue because of the .comic-amp-font-loading CSS style.

This stems from here: https://github.com/ampproject/amphtml/blob/b9d0f1bee92768d810fe9d83fea912040802461b/examples/visual-tests/font.amp.404/font.amp.html#L42

This is causing regular failures at https://percy.io/ampproject/amphtml. For example, see https://percy.io/ampproject/amphtml/builds/284124?snapshot=8861261.

This issue is similar in nature to #10156, where we were trying to wait on images to load. In this case, it's fonts. For a more general solution, we might still need to add a global "page fully loaded" event (and a sentinel CSS element) to the AMP runtime, at least for the visual tests.

rsimha commented 7 years ago

/cc @cramforce @erwinmombay @choumx

cramforce commented 7 years ago

Can you make amp-font add a CSS class that the tests waits for?

rsimha commented 7 years ago

Update: See comment thread in #10389. Once we add code to wait for font loading, it appears that the underlying issue is due to amp-font.js reporting that the fonts were loaded even though they clearly weren't. This repros when a page is loaded in phantomjs, but not when loaded in a chrome browser pointed at a local gulp serve instance.

rsimha commented 7 years ago

I did some more debugging, and here's what I found:

On Chrome, fontloader.js attempts to load the fonts with the native api and fails while doing so (as expected).

With PhantomJs, canUseNativeApis_() is false, so the fonts are loaded with polyfill. Inside loadWithPolyfill_(), we seem to be reaching the case comparators.some(comparator => comparator.compare()) most of the time (although not reliably), at which point the font loading is flagged as successful even though the fonts weren't actually available to be loaded.

@choumx @cramforce, could this be a bug in the polyfill font loading code, that's being caught by phantomjs?

cramforce commented 7 years ago

Could be.

rsimha commented 7 years ago

@camelburrito, here is what led me to my conclusion that polyfill font loading might have a bug:

camelburrito commented 7 years ago

The polyfill keeps measuring size of a span and compares it with the original size (before the font load is triggered) and keeps doing this till the size changes or it times out. So it is expected to be flaky sometimes.

In this case we are getting a success when the font has not loaded, my questions are :

  1. The snapshot shows text in red which means font loading failed - isnt that what you are looking for here ?
  2. can phantom JS really download ttf fonts and have them take effect? From my quick google searching it does not look like it will

Also now that safari uses the native API too we might only need the polyfill for IE. I am thinking that this test is probably not needed (might be a good example on how to write a percy test) as there might be frequent flakes due to the random nature of amp-fonts.

rsimha commented 7 years ago
  1. The snapshot shows text in red which means font loading failed - isnt that what you are looking for here ?

That's not true. What you're seeing as red is Percy highlighting the fact that the text does not match what is expected. The actual text is black (try clicking on the snapshot), and the actual page contains .comic-amp-font-loaded when it should contain .comic-amp-font-missing.

  1. can phantom JS really download ttf fonts and have them take effect? From my quick google searching it does not look like it will

Yes, it can. See the positive test where the fonts are available. For example, https://percy.io/ampproject/amphtml/builds/286821?snapshot=8979169.

rsimha commented 7 years ago

@camelburrito, am I to understand that font loading is supposed to be flaky with polyfill, meaning when a font is missing, the font loader sometimes reports a success, and at other times, a failure? If so, that seems like buggy behavior, right?

Also now that safari uses the native API too we might only need the polyfill for IE. I am thinking that this test is probably not needed (might be a good example on how to write a percy test) as there might be frequent flakes due to the random nature of amp-fonts.

Malte asked for this test specifically to make sure that we don't have unexpected results when a font load fails. So I'd argue that the behavior should be consistent, at which point the test will make sure that it is.

Perhaps it makes sense to no longer use polyfill for safari? Would you be able to try that in a test PR?

camelburrito commented 7 years ago

Its the other way around , sometimes we timeout and fail even while the download is still in progress.

Missing fonts should always fail.

Perhaps it makes sense to no longer use polyfill for safari? Would you be able to try that in a test PR? This does not need any changes,the versions of safari that have the new font API should automatically use the native APIs

rsimha commented 7 years ago

Missing fonts should always fail.

In that case, we have a bug, because missing fonts are showing as loaded most of the time.

rsimha commented 7 years ago

@camelburrito, let me know if you need any more info here, or if you're not convinced this is a bug in the polyfill font loader.

camelburrito commented 7 years ago

@rsimha-amp - the issue is not replicable in real browsers, i need to know what browser percy is using to replicate this.

rsimha commented 7 years ago

@camelburrito It's phantomjs, which is a webkit based headless browser. Our visual diff tests rely heavily on it, but I'd imagine you'll see the bug in any webkit browser that doesn't support native font loading.

I was able to repro this locally by modifying the code in fontloader.js to choose the polyfill path even when native font loading was supported.

rsimha commented 7 years ago

@camelburrito Bumping this issue, since it's blocking one of the visual tests.

rsimha commented 6 years ago

Turns out that amp-font modifies the font loading CSS classes in the <html> tag and not the <body> tag. The html files in examples/visual-tests/ are an artifact of old behavior, and updating it should fix this bug.

rsimha commented 6 years ago

Turns out that the polyfill code path of detecting if a font has loaded or not is buggy, and is falsely suggesting that the font load was successful. Bumping this bug. See https://travis-ci.org/ampproject/amphtml/jobs/319334387 for the latest failure.

@camelburrito Are you still the best person to investigate this bug? If not, can you please reassign?

rsimha commented 6 years ago

This bug is now moot because the visual tests no longer use phantomjs to render pages. I ran the Fonts 404 test on Chrome, and all I had to do was remove the check for .comic-amp-font-loading while continuing to make sure that .comic-amp-font-missing was found on the page. Fix coming up.