Bug 25207 – Text not visible while external font downloading

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Henri Sivonen]

Happens in latest nightly (42533) on Mac and in Safari 4 beta on Windows XP.

Steps to reproduce:
1) Load http://hsivonen.iki.fi/

Actual results:
When a given run of text is to be rendered with a external font, the run of text is not drawn until the font has been downloaded.

Expected result:
If the external font is not already in cache and layout starts, expected the next best available font to be used during font download and the page to be redrawn once the external font is available.

[Anthony Ricaud]

*** Bug 25209 has been marked as a duplicate of this bug. ***

[Anthony Ricaud]

As said in my duplicate, this is the behaviour of Firefox3.1 beta 3 and Opera 10 alpha.

[Alexey Proskuryakov]

This does feel weird indeed, although changing the font once download completes isn't great either.

[Oli Studholme]

While English fonts are about 30KB, multibyte fonts are much larger and make this problem more noticeable. For example, the free Japanese M+ fonts are 350KB+ per style (http://bit.ly/mplus).

[Oli Studholme]

The relevant setion of CSS3 Web Fonts unfortunately suggests this behaviour (assuming $B!F(Bblock on this download$B!G(B means $B!F(Bstop text layout algorithm until @font-face finishes downloading$B!G(B):
4.4 $B!H(BThe UA may choose to block on this download or may choose to proceed to the next step while the font downloads.$B!I(B
    http://www.w3.org/TR/css3-webfonts/#algorithm

I believe that this is an error in the spec, as a page in which no text initially displays appears broken to the user. This was my initial reaction to seeing Henri$B!G(Bs page before finding out about the bug, and I actually hit the back button before the @font-face downloaded. This sentence should be changed to $B!H(BThe UA must proceed to the next step while the font downloads.$B!I(B I$B!G(Bll post this to the www-style list.

[Dave Hyatt]

We might consider displaying something after a timeout, but no way are we going to draw the wrong thing immediately and then flicker to the right thing.  That would look terrible.

[Anonymous]

What about prominently showing 'Loading needed fonts...' if the font download is taking more than a few seconds?

[jjm]

I see that both the OTF and TTF versions of the same font are downloaded. Shouldn't Webkit just settle on the Opentype (or first) version?

(Just like settling on the local() version does, right?)

[Trevor Downs]

I'd like to have the option to show the text or not while an external font loads as an option somewhere, something like "WebKitShowLocalFontWhileLoading 1" being an option in both the global WebKit config and whatever application is using WebKit.

Personally I would turn on showing a local font while a page loads just so the page doesn't look weird/broken. I believe Firefox 3.6b5 shows the local font until the remote is available.


Perhaps to avoid the abruptness of changing fonts we could add a text-morph transition? ;p

[Patrick]

I think Firefox's current behaviour (showing a local font, then switching to the downloaded font once the download finishes) is preferable over just not showing any text that should be rendered with a font that is currently being downloaded.

Comment #7 (in conjunction with current behaviour) is also acceptable, although I'd show that "Downloading font..." message pretty early, maybe after half a second.

[Yuzo Fujishima]

*** Bug 44404 has been marked as a duplicate of this bug. ***

[Yuzo Fujishima]

Created attachment 67397 [details]
Patch

Comment on attachment 67397 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67397&action=prettypatch

> WebCore/ChangeLog:5
> +        Fix for Bug 25207 - Text not visible while extenal font downloading
Typo: “extenal”

> WebCore/ChangeLog:29
> +        * loader/CachedFont.cpp:
> +        (WebCore::CachedFont::beginLoadIfNeeded):
> +        * loader/CachedResourceLoader.cpp:
> +        (WebCore::CachedResourceLoader::CachedResourceLoader):
> +        (WebCore::CachedResourceLoader::startFontLoadWaitLimitTimer):
> +        (WebCore::CachedResourceLoader::fontLoadWaitLimitTimer):
> +        * loader/CachedResourceLoader.h:
> +        (WebCore::CachedResourceLoader::fontLoadWaitLimitExceeded):
> +        * platform/graphics/Font.cpp:
> +        (WebCore::Font::drawText):
> +        * platform/graphics/FontFallbackList.cpp:
> +        (WebCore::FontFallbackList::fontLoadWaitLimitExceeded):
> +        * platform/graphics/FontFallbackList.h:
> +        * platform/graphics/FontSelector.h:
There should be function-level comments about the changes.

> WebCore/platform/graphics/FontFallbackList.cpp:32
> +#include "CachedResourceLoader.h"
This is a layering violation. Platform code should not depend on higher-level WebCore code.

> WebCore/platform/graphics/FontFallbackList.cpp:145
> +    if (m_fontSelector)
> +        if (CachedResourceLoader* loader = m_fontSelector->cachedResourceLoader())
> +            if (loader->fontLoadWaitLimitExceeded())
> +                return true;
The first and second if statements require braces.

> WebCore/platform/graphics/FontSelector.h:34
> +class CachedResourceLoader;
Again, layering violation.

This patch doesn’t implement the behavior described in <https://bugs.webkit.org/show_bug.cgi?id=44404#c0>, but that behavior seems preferable, especially in case the font ultimately fails to load. Has consensus been reached on www-style regarding this?

[Yuzo Fujishima]

Created attachment 67663 [details]
Patch

[Yuzo Fujishima]

Hi, mitz,

Thank you for your review.

I've rewrote the patch to make the behavior similar to
https://bugs.webkit.org/show_bug.cgi?id=44404#c0

I've described the behavior in ChangeLog. I hope you find it reasonable.
(https://bugzilla.mozilla.org/show_bug.cgi?id=499292#c18
says "Couldn't find the www-style thread so I'm unsure of suggested timeout or
download metrics to suggest." and I don't know about the thread either.)

[Paul Irish]

0.5 seconds sounds like a very short time to me. The goal is to avoid the Flash of Unstyled Text if possible, but not hide the text if the page is taking a long time to load (e.g. on mobile connection).

Mozilla is considering 3 seconds, at the moment: 
https://bugzilla.mozilla.org/show_bug.cgi?id=499292#c20

I, too, think 3-5 seconds is a reasonable timeout.

Thanks for this patch, Yuzo! I'm super excited to see this land.

[Simon Fraser (smfr)]

Paul: can you give a link to the www-style discussion you referred to in the duplicate bug?

[Yuzo Fujishima]

Not the one Simon has requested, but this is also relevant:
http://lists.w3.org/Archives/Public/www-style/2010Oct/0161.html

[Yuzo Fujishima]

Ping? I can adjust the wait time before using temporary font in any way, say, to 3 sec.

[Simon Fraser (smfr)]

There's discussion in the CSS Working Group about what the behavior should be here.

[Eric Seidel (no email)]

@smfr:  Any result from said discussion?

[Simon Fraser (smfr)]

There's plenty of disagreement.

[David Levin]

Comment on attachment 67663 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67663&action=review

I looked over it quickly and noted a minor nit, but nothing major.

I'd like to look more closely before I r+, but before I do that, I don't understand why there isn't a test for this (r- based on no test) -- more on this in my comments.

> WebCore/ChangeLog:27
> +        No new tests because testing dynamic loading behavior is difficult to

It sounds like the behavior being fixed is something very deterministic (not a race condition, etc.).

Specifically, it occurs when the font being returned takes too long. Why can't we make the font server in the test hang while returning a result and use that to test it? (There are some xhr tests that do this.)

> WebCore/loader/CachedFont.cpp:196
> +    CachedResourceClientWalker w(m_clients);

Don't use abbreviations "w" for variable names.

> WebCore/loader/CachedFont.cpp:197
> +    while (CachedResourceClient *c = w.next())

And "c".

> WebCore/platform/graphics/FontFallbackList.cpp:93
> +        if (m_fontList[i].second) {

Consider using "continue" to avoid deep nesting.

[David Levin]

Whoops I just noticed a lot more discussion in the bug.

It sounds like the correct behavior is being decided so perhaps it is premature for a patch?

[Yuzo Fujishima]

Created attachment 78504 [details]
Pre Source/WebCore move

[Yuzo Fujishima]

Created attachment 78642 [details]
Post Source/WebCore move

[Eric Seidel (no email)]

Comment on attachment 78642 [details]
Post Source/WebCore move

This seems easy to write a test for.  I don't see why we want this though.  A 0.5 second timeout should be easy to test with a layout test.  Just point at a font which never loads (which is easy to do with an http test).

[Yuzo Fujishima]

Now the specification has a section on what to render while the web font is downloading.
http://dev.w3.org/csswg/css3-fonts/#font-face-loading

Browsers are behaving as follows:

Firefox 4:
1. Text is rendered as blank for 3 seconds or so.
2. Text is rendered with fallback fonts until download completes.
3. Text is rendered with the web fonts.

Opera 11:
1. Nothing is rendered for 2 seconds or so.
2. Text is rendered with fallback fonts until download completes.
3. Text is rendered with the web fonts.

IE 9:
1. Text is rendered with fallback fonts until download completes.
2. Text is rendered with the web fonts.

Chrome, Safari:
1. Text is rendered as blank until download completes.
2. Text is rendered with the web fonts.

I think FIrefox 4's behavior is most desirable.
My patch is intended to work in basically the same way if we change the wait from 0.5 sec to 3 seconds.

Please speak up if you have objections.

[Yuzo Fujishima]

Created attachment 94188 [details]
Patch

[Yuzo Fujishima]

I've uploaded a patch but it is not for review -- it lacks tests as of now.

[WebKit Review Bot]

Comment on attachment 94188 [details]
Patch

Attachment 94188 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8717438

New failing tests:
fast/forms/textfield-overflow.html

[WebKit Review Bot]

Created attachment 94191 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Yuzo Fujishima]

Created attachment 94380 [details]
Patch

[Yuzo Fujishima]

Added tests.

Reviewers, can you review the patch?

[WebKit Review Bot]

Comment on attachment 94380 [details]
Patch

Attachment 94380 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8732016

New failing tests:
http/tests/webfonts/slow-loading.html
fast/forms/textfield-overflow.html

[WebKit Review Bot]

Created attachment 94387 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Yuzo Fujishima]

Created attachment 94579 [details]
Added test for initial rendering

[WebKit Review Bot]

Comment on attachment 94579 [details]
Added test for initial rendering

Attachment 94579 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8734031

New failing tests:
http/tests/webfonts/slow-loading.html
fast/forms/textfield-overflow.html

[WebKit Review Bot]

Created attachment 94581 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Yuzo Fujishima]

(In reply to comment #38)
> (From update of attachment 94579 [details])
> Attachment 94579 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/8734031
> 
> New failing tests:
> http/tests/webfonts/slow-loading.html

This passed locally on linux for me.


> fast/forms/textfield-overflow.html

This is probably related to https://bugs.webkit.org/show_bug.cgi?id=61768

[Yuzo Fujishima]

Created attachment 96194 [details]
Patch

[Yuzo Fujishima]

Reviewers, ping?

Now tests passes also for cr-linux.

[Yuzo Fujishima]

Ping, reviewers?

It's sad that WebKit has been lagging behind others.
(Please see comment #28.)

[Jon Buckley]

We're running into this problem on the Processing.js project too. We'd love to see this fixed for our users, so... pinging any reviewers.

[Yuzo Fujishima]

I'd like to release the ownership.

[Paul Irish]

On the Blink side, this ticket is now tracked at  http://crbug.com/235303

[Radar WebKit Bug Importer]

<rdar://problem/15167413>

[Antti Koivisto]

We should back external font loading with aggressive speculative caching. 

- Never (within reason) remove fonts from cache
- Speculatively use cache entries even when expired
- Speculatively use cached font that is likely the same as the requested font (based on file name etc)
- Validate speculative loads asynchronously. If it turns out speculation was wrong then switch to the right font when data is available.

[Myles C. Maxfield]

*** Bug 158339 has been marked as a duplicate of this bug. ***

[Myles C. Maxfield]

Created attachment 280497 [details]
Patch

[Dean Jackson]

Comment on attachment 280497 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280497&action=review

> Source/WebCore/css/CSSFontFace.cpp:442
> +    if (newStatus == Status::Loading)
> +        m_timeoutTimer.startOneShot(webFontsShouldAlwaysFallBack() ? 0 : 3);

Why 3?

> LayoutTests/fast/text/web-font-load-fallback-during-loading.html:29
> +var token = window.setInterval(function () {

Why do you keep this variable? You never clear the interval.

[Myles C. Maxfield]

Comment on attachment 280497 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280497&action=review

>> Source/WebCore/css/CSSFontFace.cpp:442
>> +        m_timeoutTimer.startOneShot(webFontsShouldAlwaysFallBack() ? 0 : 3);
> 
> Why 3?

There are two extremes:

1. If the timeout it too short, we will see a flash of the fallback text before the desired font finishes downloading.
2. If the timeout is too long, users can't read their text

3 seconds is a happy medium between the two extremes.

It also happens to be the same as the other browsers.

>> LayoutTests/fast/text/web-font-load-fallback-during-loading.html:29
>> +var token = window.setInterval(function () {
> 
> Why do you keep this variable? You never clear the interval.

2 lines down :P

[WebKit Commit Bot]

Comment on attachment 280497 [details]
Patch

Clearing flags on attachment: 280497

Committed r201676: <http://trac.webkit.org/changeset/201676>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.