Bug 71765 – Text dispappear when SVG font has no latin character

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Kenichi Ishibashi]

Created attachment 113990 [details]
test case

If a SVG font has no latin character, text which uses the font disappear. Text in the attached html should be displayed as "!いうえお".

SVGFontData::initializeFontData() doesn't initialize font metrics when the associated GlyphPage doesn't have glyphPageZero, which is supposed to store latin character mappings. This causes the problem.

[Kenichi Ishibashi]

Created attachment 113991 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 113991 [details]
Patch

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

Fix looks great to me! I only have a minor comment regarding the test. r=me.

> LayoutTests/svg/custom/svg-fonts-no-latin-glyph.html:55
> +    setTimeout(doTest, 100);

Doesn't setTimeout(doTest, 0) work?

[Kenichi Ishibashi]

Created attachment 114006 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 113991 [details]
Patch

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

Hi Nikolas,

Thank you for your prompt review. Could you take another look?

>> LayoutTests/svg/custom/svg-fonts-no-latin-glyph.html:55
>> +    setTimeout(doTest, 100);
> 
> Doesn't setTimeout(doTest, 0) work?

setTimeout(doTest, 0) doesn't provide enough time to load font at least on my machine, so I changed to use retry counter. Retry can occur one or two times as far as I tested. The counter is set to 100 just in case.

[Nikolas Zimmermann]

(In reply to comment #4)
> (From update of attachment 113991 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113991&action=review
> 
> Hi Nikolas,
> 
> Thank you for your prompt review. Could you take another look?
> 
> >> LayoutTests/svg/custom/svg-fonts-no-latin-glyph.html:55
> >> +    setTimeout(doTest, 100);
> > 
> > Doesn't setTimeout(doTest, 0) work?
> 
> setTimeout(doTest, 0) doesn't provide enough time to load font at least on my machine, so I changed to use retry counter. Retry can occur one or two times as far as I tested. The counter is set to 100 just in case.

I see, I just looked at some other existing SVG Fonts tests, that are all potentially flakey :(
The right approach is used in svg/custom/script-tests/svg-fonts-in-text-controls.js.
Move your font into an external SVG file, reference it from your test file.
In the testcase, after enabling dumpAsText, force a layout, then listen for load events.
That should notify you of the right time, where the font is really available.

Hope that works!

[Kenichi Ishibashi]

(In reply to comment #5)
> I see, I just looked at some other existing SVG Fonts tests, that are all potentially flakey :(
> The right approach is used in svg/custom/script-tests/svg-fonts-in-text-controls.js.
> Move your font into an external SVG file, reference it from your test file.
> In the testcase, after enabling dumpAsText, force a layout, then listen for load events.
> That should notify you of the right time, where the font is really available.

Thank you for the information. Does this mean I should use separated a separated js test like svg-fonts-in-text-controls.js? I've heard it was obsoleted from my colleague. I'm happy to revise the test by using script-tests it if it's not obsoleted. Just following your suggestion without script-tests didn't work.

[Nikolas Zimmermann]

(In reply to comment #6)
> (In reply to comment #5)
> > I see, I just looked at some other existing SVG Fonts tests, that are all potentially flakey :(
> > The right approach is used in svg/custom/script-tests/svg-fonts-in-text-controls.js.
> > Move your font into an external SVG file, reference it from your test file.
> > In the testcase, after enabling dumpAsText, force a layout, then listen for load events.
> > That should notify you of the right time, where the font is really available.
> 
> Thank you for the information. Does this mean I should use separated a separated js test like svg-fonts-in-text-controls.js? I've heard it was obsoleted from my colleague. I'm happy to revise the test by using script-tests it if it's not obsoleted. Just following your suggestion without script-tests didn't work.

There's no need to switch your test to the script-tests framework, it's indeed deprecated. But the idea of forcing a layout and then registering to the onload listener is still superior.

[Nikolas Zimmermann]

(In reply to comment #6)
> (In reply to comment #5)
> > I see, I just looked at some other existing SVG Fonts tests, that are all potentially flakey :(
> > The right approach is used in svg/custom/script-tests/svg-fonts-in-text-controls.js.
> > Move your font into an external SVG file, reference it from your test file.
> > In the testcase, after enabling dumpAsText, force a layout, then listen for load events.
> > That should notify you of the right time, where the font is really available.
> 
> Thank you for the information. Does this mean I should use separated a separated js test like svg-fonts-in-text-controls.js? I've heard it was obsoleted from my colleague. I'm happy to revise the test by using script-tests it if it's not obsoleted. Just following your suggestion without script-tests didn't work.

Sorry, I didn't read the last line before replying.
Are you on IRC? Easier to discuss your testcase...

[Kenichi Ishibashi]

(In reply to comment #8)
> Sorry, I didn't read the last line before replying.
> Are you on IRC? Easier to discuss your testcase...

Sorry, I left desk without logging out from IRC, and I won't back today.. Will try to revise the test tomorrow. Thank you so much for your help.

[Kenichi Ishibashi]

> There's no need to switch your test to the script-tests framework, it's indeed deprecated. But the idea of forcing a layout and then registering to the onload listener is still superior.

Here is a revised test I wrote (which doesn't work as expected):

<html>
<head>
<meta charset=utf-8>
</head>
<body>
<p>Following character should be rendered as an exlamation.</p>
<script>
function doTest() {
    var height = document.getElementById('text').offsetHeight;
    var result = document.createElement('div');
    result.innerHTML = 'character height is ' + height + '<br />';
    if (height == 16)
        result.innerHTML += 'PASS';
    else
        result.innerHTML += 'FAIL';
    document.body.appendChild(result);
    if (window.layoutTestController)
        layoutTestController.notifyDone();
}

var styleElement = document.createElement('style');
styleElement.innerText = '@font-face { font-family: Font; src: url(resources/no-latin-glyph-font.svg#Font) format("svg"); }';
document.getElementsByTagName('head')[0].appendChild(styleElement);

var spanElement = document.createElement('span');
spanElement.style.fontFamily = 'Font';
spanElement.id = 'text';
spanElement.innerText = ' ';
document.body.appendChild(spanElement);

if (window.layoutTestController) {
    layoutTestController.dumpAsText();
    layoutTestController.waitUntilDone();
    document.body.offsetWidth;
}
//window.addEventListener('load', function() {setTimeout(doTest, 100);}, true);
window.addEventListener('load', doTest, true);
</script>
</body>

What am I missing? using setTimeout in the event handler of load event works fine. Naturally, moving script tag into <head> doesn't work
 because it refer document.body.

[Nikolas Zimmermann]

Our IRC discussion:
Kenichis testcase should work as-is (the last before this comment). It turns out several of the svg/custom/svg-fonts* are flakey, even those which don't load any external resources (they declare the SVG Font right in the host document) and triggering manual layouts, via the offsetTop/Width trick.

That indicates theres a general problem with SVG Fonts that is timing dependant.
(The chromium flakiness dashboard helps a lot! Thanks for that.)

I'd highly appreciate if you could look into how SVG Fonts are resolved during layout, and where it goes wrong.

[Kenichi Ishibashi]

(In reply to comment #11)
> I'd highly appreciate if you could look into how SVG Fonts are resolved during layout, and where it goes wrong.

According to https://bugs.webkit.org/show_bug.cgi?id=65123#c10, http://trac.webkit.org/changeset/91738 might affect the loading behavior. (Indeed, it modifies some svg-font related fonts)

Should we use setTimeout trick for all webfonts related tests?

[Kenichi Ishibashi]

Created attachment 115096 [details]
Patch

[Kenichi Ishibashi]

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

In response to https://bugs.webkit.org/show_bug.cgi?id=65123#c11, I've updated the patch so that we don't need to use timer. This patch uses event dispatching mechanism for notification. The event is only dispatched when a settings flag is enabled. The flag is set via window.internals object. Please let me know if there is more appropriate way to do this.

Thanks,

[Nikolas Zimmermann]

Comment on attachment 115096 [details]
Patch

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

Superb work bashi, I think this needs a split-up though, as its mixing things.

> Source/WebCore/css/CSSFontFace.cpp:108
> +    if (Document* document = fontSelector->document()) {
> +        if (document->settings() && document->settings()->fontLoadedEventEnabled() && document->domWindow()) {
> +            RefPtr<Event> event = Event::create("webkitfontloaded", false, false);
> +            document->domWindow()->dispatchEvent(event, document);
> +        }
> +    }

Excellent approach! Though I think you should land this separated.
There are numerous of bugs reports regarding flakey SVG Fonts tests, that could and should all be fixed, by listening to that new event.
Could you make a new bug report, find all bug reports for flakey svg fonts (most come from chromium), mark them as depending of your master bug, fixup all these tests, finish & land patch, then come back to this bug report.

> LayoutTests/svg/custom/svg-fonts-no-latin-glyph.html:27
> +    document.documentElement.offsetTop;
> +    window.addEventListener('webkitfontloaded', doTest);

I'd reverse the order here.

> LayoutTests/svg/custom/svg-fonts-no-latin-glyph.html:31
> +    setTimeout(function() {
> +        document.getElementById('result').innerHTML = 'TIMEOUT';
> +        layoutTestController.notifyDone();
> +    }, 500);

I'd just avoid that, it's confusing.

[Kenichi Ishibashi]

Created attachment 117832 [details]
Patch

[Kenichi Ishibashi]

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

As of r101858, we can use the same approach used in svg/custom/script-tests/svg-fonts-in-text-controls.js.

Nikolas, Could you please take another look?

[WebKit Review Bot]

Comment on attachment 117832 [details]
Patch

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

New failing tests:
http/tests/inspector/resource-tree/resource-tree-document-url.html

[Kenichi Ishibashi]

Created attachment 119725 [details]
Patch (Revised to ToT)

[Nikolas Zimmermann]

Comment on attachment 119725 [details]
Patch (Revised to ToT)

Looks great, r=me!

[WebKit Review Bot]

Comment on attachment 119725 [details]
Patch (Revised to ToT)

Clearing flags on attachment: 119725

Committed r103155: <http://trac.webkit.org/changeset/103155>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Somehow the layout test for this change doesn't pass on the Qt bot: http://build.webkit.org/results/Qt%20Linux%20Release/r103155%20(41158)/results.html