Bug 25207 - Text not visible while external font downloading
Summary: Text not visible while external font downloading
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Myles C. Maxfield
URL: http://hsivonen.iki.fi/
Keywords: InRadar
: 25209 44404 158339 (view as bug list)
Depends on:
Blocks: 33998
  Show dependency treegraph
 
Reported: 2009-04-15 06:24 PDT by Henri Sivonen
Modified: 2017-07-07 04:04 PDT (History)
37 users (show)

See Also:


Attachments
Patch (8.29 KB, patch)
2010-09-13 04:42 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Patch (19.92 KB, patch)
2010-09-15 03:41 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Pre Source/WebCore move (19.97 KB, patch)
2011-01-11 02:27 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Post Source/WebCore move (20.43 KB, patch)
2011-01-11 19:17 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Patch (20.38 KB, patch)
2011-05-20 01:12 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.40 MB, application/zip)
2011-05-20 02:05 PDT, WebKit Review Bot
no flags Details
Patch (28.60 KB, patch)
2011-05-23 01:18 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.43 MB, application/zip)
2011-05-23 02:16 PDT, WebKit Review Bot
no flags Details
Added test for initial rendering (29.24 KB, patch)
2011-05-24 00:28 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.27 MB, application/zip)
2011-05-24 01:12 PDT, WebKit Review Bot
no flags Details
Patch (31.28 KB, patch)
2011-06-06 21:24 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Patch (22.41 KB, patch)
2016-06-03 18:15 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Henri Sivonen 2009-04-15 06:24:22 PDT
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.
Comment 1 Anthony Ricaud 2009-04-15 06:40:04 PDT
*** Bug 25209 has been marked as a duplicate of this bug. ***
Comment 2 Anthony Ricaud 2009-04-15 06:40:56 PDT
As said in my duplicate, this is the behaviour of Firefox3.1 beta 3 and Opera 10 alpha.
Comment 3 Alexey Proskuryakov 2009-04-29 22:55:02 PDT
This does feel weird indeed, although changing the font once download completes isn't great either.
Comment 4 Oli Studholme 2009-04-29 23:09:44 PDT
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).
Comment 5 Oli Studholme 2009-05-06 21:04:52 PDT
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.
Comment 6 Dave Hyatt 2009-05-07 09:03:27 PDT
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.

Comment 7 Anonymous 2009-05-13 16:05:17 PDT
What about prominently showing 'Loading needed fonts...' if the font download is taking more than a few seconds?
Comment 8 jjm 2009-07-08 04:43:47 PDT
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?)
Comment 9 Trevor Downs 2010-01-03 09:10:26 PST
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
Comment 10 Patrick 2010-08-17 01:56:21 PDT
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.
Comment 11 Yuzo Fujishima 2010-09-12 22:24:05 PDT
*** Bug 44404 has been marked as a duplicate of this bug. ***
Comment 12 Yuzo Fujishima 2010-09-13 04:42:53 PDT
Created attachment 67397 [details]
Patch
Comment 13 mitz 2010-09-13 09:22:28 PDT
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?
Comment 14 Yuzo Fujishima 2010-09-15 03:41:12 PDT
Created attachment 67663 [details]
Patch
Comment 15 Yuzo Fujishima 2010-09-15 03:49:07 PDT
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.)
Comment 16 Paul Irish 2010-10-02 17:24:05 PDT
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.
Comment 17 Simon Fraser (smfr) 2010-10-05 16:01:24 PDT
Paul: can you give a link to the www-style discussion you referred to in the duplicate bug?
Comment 18 Yuzo Fujishima 2010-10-13 19:16:21 PDT
Not the one Simon has requested, but this is also relevant:
http://lists.w3.org/Archives/Public/www-style/2010Oct/0161.html
Comment 19 Yuzo Fujishima 2010-10-29 00:18:35 PDT
Ping? I can adjust the wait time before using temporary font in any way, say, to 3 sec.
Comment 20 Simon Fraser (smfr) 2010-10-29 08:59:22 PDT
There's discussion in the CSS Working Group about what the behavior should be here.
Comment 21 Eric Seidel (no email) 2010-12-03 10:38:47 PST
@smfr:  Any result from said discussion?
Comment 22 Simon Fraser (smfr) 2010-12-03 10:43:44 PST
There's plenty of disagreement.
Comment 23 David Levin 2010-12-27 11:12:04 PST
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.
Comment 24 David Levin 2010-12-27 11:14:12 PST
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?
Comment 25 Yuzo Fujishima 2011-01-11 02:27:53 PST
Created attachment 78504 [details]
Pre Source/WebCore move
Comment 26 Yuzo Fujishima 2011-01-11 19:17:33 PST
Created attachment 78642 [details]
Post Source/WebCore move
Comment 27 Eric Seidel (no email) 2011-04-26 15:43:44 PDT
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).
Comment 28 Yuzo Fujishima 2011-05-20 00:33:37 PDT
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.
Comment 29 Yuzo Fujishima 2011-05-20 01:12:55 PDT
Created attachment 94188 [details]
Patch
Comment 30 Yuzo Fujishima 2011-05-20 01:14:09 PDT
I've uploaded a patch but it is not for review -- it lacks tests as of now.
Comment 31 WebKit Review Bot 2011-05-20 02:05:35 PDT
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
Comment 32 WebKit Review Bot 2011-05-20 02:05:42 PDT
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
Comment 33 Yuzo Fujishima 2011-05-23 01:18:42 PDT
Created attachment 94380 [details]
Patch
Comment 34 Yuzo Fujishima 2011-05-23 02:09:20 PDT
Added tests.

Reviewers, can you review the patch?
Comment 35 WebKit Review Bot 2011-05-23 02:16:13 PDT
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
Comment 36 WebKit Review Bot 2011-05-23 02:16:20 PDT
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
Comment 37 Yuzo Fujishima 2011-05-24 00:28:14 PDT
Created attachment 94579 [details]
Added test for initial rendering
Comment 38 WebKit Review Bot 2011-05-24 01:12:41 PDT
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
Comment 39 WebKit Review Bot 2011-05-24 01:12:47 PDT
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
Comment 40 Yuzo Fujishima 2011-05-31 00:42:50 PDT
(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
Comment 41 Yuzo Fujishima 2011-06-06 21:24:59 PDT
Created attachment 96194 [details]
Patch
Comment 42 Yuzo Fujishima 2011-06-08 19:24:48 PDT
Reviewers, ping?

Now tests passes also for cr-linux.
Comment 43 Yuzo Fujishima 2011-06-22 23:15:50 PDT
Ping, reviewers?

It's sad that WebKit has been lagging behind others.
(Please see comment #28.)
Comment 44 Jon Buckley 2011-08-17 18:32:26 PDT
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.
Comment 45 Yuzo Fujishima 2012-04-02 09:56:03 PDT
I'd like to release the ownership.
Comment 46 Paul Irish 2013-04-24 18:39:22 PDT
On the Blink side, this ticket is now tracked at  http://crbug.com/235303
Comment 47 Radar WebKit Bug Importer 2013-10-07 11:30:23 PDT
<rdar://problem/15167413>
Comment 48 Antti Koivisto 2013-10-17 06:13:50 PDT
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.
Comment 49 Myles C. Maxfield 2016-06-03 18:12:49 PDT
*** Bug 158339 has been marked as a duplicate of this bug. ***
Comment 50 Myles C. Maxfield 2016-06-03 18:15:11 PDT
Created attachment 280497 [details]
Patch
Comment 51 Dean Jackson 2016-06-03 18:25:34 PDT
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.
Comment 52 Myles C. Maxfield 2016-06-03 18:28:17 PDT
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
Comment 53 WebKit Commit Bot 2016-06-03 18:59:28 PDT
Comment on attachment 280497 [details]
Patch

Clearing flags on attachment: 280497

Committed r201676: <http://trac.webkit.org/changeset/201676>
Comment 54 WebKit Commit Bot 2016-06-03 18:59:38 PDT
All reviewed patches have been landed.  Closing bug.