Bug 76684 - Respects font fallback list while webfonts are loading
: Respects font fallback list while webfonts are loading
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Text
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Kenichi Ishibashi
http://crbug.com/104233
:
Depends on: 82725
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-19 20:46 PST by Kenichi Ishibashi
Modified: 2012-04-03 02:08 PDT (History)
11 users (show)

See Also:


Attachments
Patch (7.19 KB, patch)
2012-01-19 20:55 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (7.84 KB, patch)
2012-01-23 16:05 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (7.98 KB, patch)
2012-01-24 16:23 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (5.91 KB, patch)
2012-01-25 18:17 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Rebased to ToT (5.87 KB, patch)
2012-03-01 00:09 PST, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (10.58 KB, patch)
2012-03-14 16:07 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch for landing (4.12 KB, patch)
2012-03-28 17:12 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (7.27 KB, patch)
2012-03-29 18:47 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch (3.62 KB, patch)
2012-03-30 01:54 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Use perl instead of python (3.53 KB, patch)
2012-03-30 02:23 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch for landing (3.37 KB, patch)
2012-04-03 00:30 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenichi Ishibashi 2012-01-19 20:46:37 PST
During webfonts are loading, WebKit uses the last resort fonts for layout. I think we should respect generic font family names (if they exists) for the font fallback mechanism, not just using the last resort fonts. This way, we can provide a workaround to detect webfonts are fully loaded. See http://crbug.com/104233 for more details.
Comment 1 Kenichi Ishibashi 2012-01-19 20:55:51 PST
Created attachment 123250 [details]
Patch
Comment 2 Alexey Proskuryakov 2012-01-20 10:34:21 PST
This last resort font <http://unicode.org/policies/lastresortfont_eula.html>? I haven't seen that happen on any Web sites.
Comment 3 Kenichi Ishibashi 2012-01-22 16:18:26 PST
(In reply to comment #2)
> This last resort font <http://unicode.org/policies/lastresortfont_eula.html>? I haven't seen that happen on any Web sites.

No. I use that term to describe the fonts which are returned by getNonRetainedLastResortFallbackFont(). These fonts are differ among ports. For example, mac port returns "Times" or "Lucida Grande". Sorry for using misleading term.
Comment 4 Alexey Proskuryakov 2012-01-22 16:22:50 PST
I'm not sure whether we should display the text at all while font is loading.
Comment 5 Kenichi Ishibashi 2012-01-22 16:27:43 PST
(In reply to comment #4)
> I'm not sure whether we should display the text at all while font is loading.

This patch don't display the text during font loading (IMHO, the text should be displayed, though). It tries to find fonts based on generic font family names (if they exist on "font-family") for layout.
Comment 6 Hajime Morrita 2012-01-23 14:02:02 PST
Comment on attachment 123250 [details]
Patch

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

> Source/WebCore/css/CSSFontFaceSource.cpp:40
>  

These extra dependencies are really sad. Also, there is fontDataForGenericFamily in CSSFontSelector.cpp. 
So my feeling is that  getGenericFallbackFontData should be a part of  CSSFontSelector, not CSSFontFaceSource, something like CSSFontSelector::getGetFallbackFontData() ?
Comment 7 Kenichi Ishibashi 2012-01-23 16:05:23 PST
Created attachment 123645 [details]
Patch
Comment 8 Kenichi Ishibashi 2012-01-23 16:06:30 PST
Comment on attachment 123250 [details]
Patch

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

Thank you for review!

>> Source/WebCore/css/CSSFontFaceSource.cpp:40
>>  
> 
> These extra dependencies are really sad. Also, there is fontDataForGenericFamily in CSSFontSelector.cpp. 
> So my feeling is that  getGenericFallbackFontData should be a part of  CSSFontSelector, not CSSFontFaceSource, something like CSSFontSelector::getGetFallbackFontData() ?

I agree. Removed these dependencies and moved the function to CSSFontSelector as CSSFontSelector::getGenericFallbackFontData().
Comment 9 Hajime Morrita 2012-01-24 15:17:34 PST
Comment on attachment 123645 [details]
Patch

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

> Source/WebCore/css/CSSFontSelector.cpp:623
> +{

CSSFontSelector.cpp already has fontDataForGenericFamily() which looks almost same as this method. What is the difference between these two?
Comment 10 Kenichi Ishibashi 2012-01-24 16:23:23 PST
Created attachment 123836 [details]
Patch
Comment 11 Kenichi Ishibashi 2012-01-24 16:29:59 PST
Comment on attachment 123645 [details]
Patch

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

>> Source/WebCore/css/CSSFontSelector.cpp:623
>> +{
> 
> CSSFontSelector.cpp already has fontDataForGenericFamily() which looks almost same as this method. What is the difference between these two?

The only difference is fontDataForGenericFamily() uses the given font family name to select an appropriate font data, while this method selects a font data based on FontDescription's generic family. Revised the patch to use fontDataForGenericFamily() in this method to share the logic.
Comment 12 mitz@webkit.org 2012-01-25 11:06:35 PST
Why go with a generic family? Wouldn’t it be better to just use the rest of the fallback list (which may include a generic family, and terminates in the “last resort” font)? I imagine this could be done by having the temporary font we supply be one that has no coverage, so everything falls back through the rest of the list. I guess the vertical metrics would still need to come from somewhere, possibly the next item on the list. This would have the advantage that in the case the font eventually fails to load, and the fallback is used, the layout wouldn’t change at all.
Comment 13 Kenichi Ishibashi 2012-01-25 18:17:24 PST
Created attachment 124051 [details]
Patch
Comment 14 Kenichi Ishibashi 2012-01-25 18:18:52 PST
(In reply to comment #12)
> Why go with a generic family? Wouldn’t it be better to just use the rest of the fallback list (which may include a generic family, and terminates in the “last resort” font)? I imagine this could be done by having the temporary font we supply be one that has no coverage, so everything falls back through the rest of the list. I guess the vertical metrics would still need to come from somewhere, possibly the next item on the list. This would have the advantage that in the case the font eventually fails to load, and the fallback is used, the layout wouldn’t change at all.

The solution is what I was looking for. I couldn't figure out how I can use the rest of the fallback list. Thank you so much for your suggestion.
Comment 15 Kenichi Ishibashi 2012-01-30 15:41:05 PST
Hi mitz,

Could you please take another look?
Comment 16 Kenichi Ishibashi 2012-02-02 06:55:07 PST
I'd appreciate if someone could review the patch. I pinged not only on the bugzilla but also by sending an email. What else can I do?
Comment 17 Kenichi Ishibashi 2012-03-01 00:09:59 PST
Created attachment 129650 [details]
Rebased to ToT
Comment 18 Kenichi Ishibashi 2012-03-01 00:12:54 PST
(In reply to comment #17)
> Created an attachment (id=129650) [details]
> Rebased to ToT

Hi mitz,

Could you please take a look? We want to fix the issue.
Comment 19 komoroske 2012-03-02 10:02:14 PST
This issue is one that affects the Google Docs suite of apps and they're very keen to get fixed.  Any help would be appreciated!
Comment 20 Dimitri Glazkov (Google) 2012-03-14 14:30:56 PDT
Comment on attachment 129650 [details]
Rebased to ToT

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

> Source/WebCore/ChangeLog:6
> +        For layout, use the rest of the fallback list during webfonts are loading.

during -> while

> Source/WebCore/ChangeLog:7
> +        This is done by returning the temporary font that has no coverage.

Can you explain this a bit? What's a "coverage"?

> Source/WebCore/css/CSSSegmentedFontFace.cpp:119
> +            // If the faceFontData is loading, add a range which has no coverage
> +            // so that using the rest of fallback list for layout.
> +            if (faceFontData->isLoading())
> +                newFontData->appendRange(FontDataRange(0, 0, faceFontData));
>              else {
> -                for (unsigned j = 0; j < numRanges; ++j)
> -                    newFontData->appendRange(FontDataRange(ranges[j].from(), ranges[j].to(), faceFontData));
> +                const Vector<CSSFontFace::UnicodeRange>& ranges = m_fontFaces[i]->ranges();
> +                unsigned numRanges = ranges.size();
> +                if (!numRanges)
> +                    newFontData->appendRange(FontDataRange(0, 0x7FFFFFFF, faceFontData));
> +                else {
> +                    for (unsigned j = 0; j < numRanges; ++j)
> +                        newFontData->appendRange(FontDataRange(ranges[j].from(), ranges[j].to(), faceFontData));
> +                }

can this be made into a static helper with a good descriptive name? You may not need a comment then.

> LayoutTests/fast/text/fallback-font-while-loading.html:21
> +    style.innerText = '@font-face { font-family: Ahem; src: url(../../resources/Ahem.ttf?' + Date.now() + '); }';

Is this really the best way to ensure that we have not cached a file? Don't we have some tools in loader-related tests?

> LayoutTests/fast/text/fallback-font-while-loading.html:22
> +    document.getElementsByTagName('head')[0].appendChild(style);

document.head is simpler.
Comment 21 Kenichi Ishibashi 2012-03-14 16:07:18 PDT
Created attachment 131947 [details]
Patch
Comment 22 Kenichi Ishibashi 2012-03-14 16:11:17 PDT
Comment on attachment 129650 [details]
Rebased to ToT

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

Thank you so much for review.

>> Source/WebCore/ChangeLog:6
>> +        For layout, use the rest of the fallback list during webfonts are loading.
> 
> during -> while

Done. (I've just noticed I should have also fixed the title. Will fix next patch)

>> Source/WebCore/ChangeLog:7
>> +        This is done by returning the temporary font that has no coverage.
> 
> Can you explain this a bit? What's a "coverage"?

Done.

>> Source/WebCore/css/CSSSegmentedFontFace.cpp:119
>> +                }
> 
> can this be made into a static helper with a good descriptive name? You may not need a comment then.

Done.

>> LayoutTests/fast/text/fallback-font-while-loading.html:21
>> +    style.innerText = '@font-face { font-family: Ahem; src: url(../../resources/Ahem.ttf?' + Date.now() + '); }';
> 
> Is this really the best way to ensure that we have not cached a file? Don't we have some tools in loader-related tests?

We don't have webfont loader-related tests. I revised the test to use cgi.

>> LayoutTests/fast/text/fallback-font-while-loading.html:22
>> +    document.getElementsByTagName('head')[0].appendChild(style);
> 
> document.head is simpler.

Done.
Comment 23 Kenichi Ishibashi 2012-03-20 21:41:28 PDT
Hi Dimitri,

Could you please take another look?
Comment 24 Kenichi Ishibashi 2012-03-27 17:14:48 PDT
(In reply to comment #23)
> Hi Dimitri,
> 
> Could you please take another look?

Ping Dimitri?
Comment 25 Dimitri Glazkov (Google) 2012-03-28 09:48:12 PDT
Comment on attachment 131947 [details]
Patch

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

ok. In the future, please don't wait a week if you don't get a response from a reviewer. In this particular case, you have my email, and heck, you even have my IM. We all get a ton of mail -- it's likely that some will fall through cracks. Be persistent :)

> Source/WebCore/css/CSSSegmentedFontFace.cpp:93
> +    if (!numRanges)
> +        newFontData->appendRange(FontDataRange(0, 0x7FFFFFFF, faceFontData));

Can do early return here.
Comment 26 Kenichi Ishibashi 2012-03-28 17:12:04 PDT
Created attachment 134453 [details]
Patch for landing
Comment 27 Kenichi Ishibashi 2012-03-28 17:12:43 PDT
Comment on attachment 131947 [details]
Patch

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

I see. Thank you for review!

>> Source/WebCore/css/CSSSegmentedFontFace.cpp:93
>> +        newFontData->appendRange(FontDataRange(0, 0x7FFFFFFF, faceFontData));
> 
> Can do early return here.

Done.
Comment 28 WebKit Review Bot 2012-03-28 18:41:19 PDT
Comment on attachment 134453 [details]
Patch for landing

Clearing flags on attachment: 134453

Committed r112489: <http://trac.webkit.org/changeset/112489>
Comment 29 WebKit Review Bot 2012-03-28 18:41:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Tony Chang 2012-03-29 11:28:16 PDT
(In reply to comment #29)
> All reviewed patches have been landed.  Closing bug.

Did the layout test not get landed?
Comment 31 Tony Chang 2012-03-29 11:29:58 PDT
(In reply to comment #30)
> (In reply to comment #29)
> > All reviewed patches have been landed.  Closing bug.
> 
> Did the layout test not get landed?

Also, Ahem.ttf is already in LayoutTests/resources/Ahem.ttf, so we don't need to check in another copy.
Comment 32 Ojan Vafai 2012-03-29 11:52:29 PDT
FYI: It looks like the layout tests got dropped in your final patch. Please commit them when you get a chance.
Comment 33 Kenichi Ishibashi 2012-03-29 17:54:56 PDT
Thank you for the notice guys! I'll create a patch to add the test.

> Also, Ahem.ttf is already in LayoutTests/resources/Ahem.ttf, so we don't need to check in another copy.

The test will be served via local http server so I think we can't use LayoutTests/resources/Ahem.ttf from the test. No?
Comment 34 Kenichi Ishibashi 2012-03-29 18:47:51 PDT
Reopening to attach new patch.
Comment 35 Kenichi Ishibashi 2012-03-29 18:47:57 PDT
Created attachment 134710 [details]
Patch
Comment 36 Kenichi Ishibashi 2012-03-29 18:49:17 PDT
(In reply to comment #35)
> Created an attachment (id=134710) [details]
> Patch

I'd like to wait Tony's comment for adding Ahem.ttf.
Comment 37 Alexey Proskuryakov 2012-03-30 00:06:06 PDT
Comment on attachment 134710 [details]
Patch

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

getahem.cgi needs a better name - nothing in it tells the reader that it's going to do it slowly.

I'd suggest following an existing pattern for a slow loading test (find LayoutTests/http/tests/ -name *slow* | grep -v svn), and using a language that one of those tests already uses, not python.

r- because there is no need to add another copy of Ahem.

> LayoutTests/ChangeLog:3
> +        Respects font fallback list while webfonts are loading

This is grammatically incorrect. Did you mean to say something like "Fallback fonts should be used while a web font is being loaded"?

> LayoutTests/http/tests/webfont/fallback-font-while-loading.html:15
> +    layoutTestController.overridePreference('WebKitSerifFont', 'Arial');

Is this needed because you know that this string has different width when using Arial and when using Ahem, but aren't sure about default serif font? That's worth explaining in a comment.

> LayoutTests/http/tests/webfont/fallback-font-while-loading.html:37
> +addTextWithWebfont();

Why does this element need to be added dynamically?

> LayoutTests/http/tests/webfont/getahem.cgi:7
> +font_data = open("../resources/Ahem.ttf").read()

This loads a local file already. You can avoid adding Ahem.ttf by using a relative path to an existing one ("../../../resources/Ahem.ttf" if I counted the dots correctly).
Comment 38 Kenichi Ishibashi 2012-03-30 01:54:43 PDT
Created attachment 134748 [details]
Patch
Comment 39 Kenichi Ishibashi 2012-03-30 02:23:09 PDT
Created attachment 134751 [details]
Use perl instead of python
Comment 40 Kenichi Ishibashi 2012-03-30 02:24:20 PDT
Comment on attachment 134710 [details]
Patch

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

Thank you for review!

>> LayoutTests/ChangeLog:3
>> +        Respects font fallback list while webfonts are loading
> 
> This is grammatically incorrect. Did you mean to say something like "Fallback fonts should be used while a web font is being loaded"?

Thanks. Fixed.

>> LayoutTests/http/tests/webfont/fallback-font-while-loading.html:15
>> +    layoutTestController.overridePreference('WebKitSerifFont', 'Arial');
> 
> Is this needed because you know that this string has different width when using Arial and when using Ahem, but aren't sure about default serif font? That's worth explaining in a comment.

Done.

>> LayoutTests/http/tests/webfont/fallback-font-while-loading.html:37
>> +addTextWithWebfont();
> 
> Why does this element need to be added dynamically?

I just used the way that the original repro used, but there is no need to do it. Revised to use tags.

>> LayoutTests/http/tests/webfont/getahem.cgi:7
>> +font_data = open("../resources/Ahem.ttf").read()
> 
> This loads a local file already. You can avoid adding Ahem.ttf by using a relative path to an existing one ("../../../resources/Ahem.ttf" if I counted the dots correctly).

You are right. Done.
Comment 41 Ojan Vafai 2012-03-30 10:37:20 PDT
Comment on attachment 134751 [details]
Use perl instead of python

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

> LayoutTests/http/tests/webfont/fallback-font-while-loading.html:23
> +    // Set Arial as serif to make sure that the string has different width when using Ahem and when using serif.
> +    layoutTestController.overridePreference('WebKitSerifFont', 'Arial');

Nit: could you just use arial directly as the font-family instead of serif?

> LayoutTests/http/tests/webfont/fallback-font-while-loading.html:37
> +//addTextWithWebfont();

Please delete
Comment 42 Kenichi Ishibashi 2012-04-01 17:25:39 PDT
Thank you for review. r112489 was rolled out so I'll revise the patch with original change later.

(In reply to comment #41)
> (From update of attachment 134751 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134751&action=review
> 
> > LayoutTests/http/tests/webfont/fallback-font-while-loading.html:23
> > +    // Set Arial as serif to make sure that the string has different width when using Ahem and when using serif.
> > +    layoutTestController.overridePreference('WebKitSerifFont', 'Arial');
> 
> Nit: could you just use arial directly as the font-family instead of serif?
> 
> > LayoutTests/http/tests/webfont/fallback-font-while-loading.html:37
> > +//addTextWithWebfont();
> 
> Please delete
Comment 43 Kenichi Ishibashi 2012-04-03 00:30:11 PDT
Created attachment 135284 [details]
Patch for landing
Comment 44 WebKit Review Bot 2012-04-03 02:08:17 PDT
Comment on attachment 135284 [details]
Patch for landing

Clearing flags on attachment: 135284

Committed r112999: <http://trac.webkit.org/changeset/112999>
Comment 45 WebKit Review Bot 2012-04-03 02:08:24 PDT
All reviewed patches have been landed.  Closing bug.