Bug 76684 – Respects font fallback list while webfonts are loading

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

[Kenichi Ishibashi]

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.

[Kenichi Ishibashi]

Created attachment 123250 [details]
Patch

[Alexey Proskuryakov]

This last resort font <http://unicode.org/policies/lastresortfont_eula.html>? I haven't seen that happen on any Web sites.

[Kenichi Ishibashi]

(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.

[Alexey Proskuryakov]

I'm not sure whether we should display the text at all while font is loading.

[Kenichi Ishibashi]

(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.

[Hajime Morrita]

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() ?

[Kenichi Ishibashi]

Created attachment 123645 [details]
Patch

[Kenichi Ishibashi]

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().

[Hajime Morrita]

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?

[Kenichi Ishibashi]

Created attachment 123836 [details]
Patch

[Kenichi Ishibashi]

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.

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.

[Kenichi Ishibashi]

Created attachment 124051 [details]
Patch

[Kenichi Ishibashi]

(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.

[Kenichi Ishibashi]

Hi mitz,

Could you please take another look?

[Kenichi Ishibashi]

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?

[Kenichi Ishibashi]

Created attachment 129650 [details]
Rebased to ToT

[Kenichi Ishibashi]

(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.

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!

[Dimitri Glazkov (Google)]

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.

[Kenichi Ishibashi]

Created attachment 131947 [details]
Patch

[Kenichi Ishibashi]

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.

[Kenichi Ishibashi]

Hi Dimitri,

Could you please take another look?

[Kenichi Ishibashi]

(In reply to comment #23)
> Hi Dimitri,
> 
> Could you please take another look?

Ping Dimitri?

[Dimitri Glazkov (Google)]

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.

[Kenichi Ishibashi]

Created attachment 134453 [details]
Patch for landing

[Kenichi Ishibashi]

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.

[WebKit Review Bot]

Comment on attachment 134453 [details]
Patch for landing

Clearing flags on attachment: 134453

Committed r112489: <http://trac.webkit.org/changeset/112489>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

(In reply to comment #29)
> All reviewed patches have been landed.  Closing bug.

Did the layout test not get landed?

[Tony Chang]

(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.

[Ojan Vafai]

FYI: It looks like the layout tests got dropped in your final patch. Please commit them when you get a chance.

[Kenichi Ishibashi]

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?

[Kenichi Ishibashi]

Reopening to attach new patch.

[Kenichi Ishibashi]

Created attachment 134710 [details]
Patch

[Kenichi Ishibashi]

(In reply to comment #35)
> Created an attachment (id=134710) [details]
> Patch

I'd like to wait Tony's comment for adding Ahem.ttf.

[Alexey Proskuryakov]

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).

[Kenichi Ishibashi]

Created attachment 134748 [details]
Patch

[Kenichi Ishibashi]

Created attachment 134751 [details]
Use perl instead of python

[Kenichi Ishibashi]

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.

[Ojan Vafai]

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

[Kenichi Ishibashi]

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

[Kenichi Ishibashi]

Created attachment 135284 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 135284 [details]
Patch for landing

Clearing flags on attachment: 135284

Committed r112999: <http://trac.webkit.org/changeset/112999>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.