Bug 97824 – OpenTypeVerticalData issue with DroidSansFallback.ttf on chromium-android and chromium-linux

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Xianzhu Wang]

Bug 97227 enabled OpenTypeVerticalData on Linux. However, if replace the fonts used in layout tests (/usr/share/fonts/truetype/kochi/kochi-gothic.ttf, /usr/share/fonts/truetype/kochi/kochi-mincho.ttf) with Android's font DroidSansFallback.ttf (containing CJK glyphs and others), DumpRenderTree generates incorrect result for verticle CJK text.

Steps to reproduce:
1. Apply the attached patch and build DumpRenderTree
2. Save the attached DroidSansFallback.ttf to /tmp
3. Run new-run-webkit-tests --no-new-test-results testvert.html

Expected result: see the attached expected result
Actual result: see the attached actual result 1. The CJK glyphs are only displayed small parts of them

Reason: On chromium-linux unitsPerEm() has only a default value 1000, so the OpenTypeVerticalData::advanceHeight() computes incorrect height advance (4.096).

For the default fonts kochi-gothic.ttf and kochi-mincho.ttf, m_advanceHeights is empty, so the default height is returned, and it happens to be correct.

With the 'if' block in OpenTypeVerticalData::advanceHeight() commented out (forced to use the default height), it produces another incorrect result (see attached actual result 2): The punctuations are not correctly substituted.

The results are the same on chromium-android with the font.

[Xianzhu Wang]

Created attachment 166073 [details]
Patch for reproducing the bug

[Xianzhu Wang]

Created attachment 166075 [details]
The font

[Xianzhu Wang]

Created attachment 166076 [details]
expected result

[Xianzhu Wang]

Created attachment 166077 [details]
actual result 1

[Xianzhu Wang]

Created attachment 166080 [details]
actual result 2 (the 'if' in OpenTypeVerticalData::advanceHeight() commented out)

[Kenichi Ishibashi]

Could you try the patch at https://bugs.webkit.org/show_bug.cgi?id=97676 ?

[Xianzhu Wang]

Created attachment 166105 [details]
Result with patch of bug 97676

(In reply to comment #6)
> Could you try the patch at https://bugs.webkit.org/show_bug.cgi?id=97676 ?

With the patch, it produces different result as shown in the attachment:
1) Incorrect return value from OpenTypeVerticalData::verticalAdvance() because of the incorrect default unitsPerEm
2) Incorrect orientation of punctuations

[Xianzhu Wang]

Created attachment 167137 [details]
Patch

[Tony Chang]

Comment on attachment 167137 [details]
Patch

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

r- because we should test the other code paths.

> Source/WebCore/platform/graphics/Font.cpp:560
> +    // CJK Vertical Forms.
> +    if (c >= 0xFE10 && c <= 0xFE1F)
> +        return true;

It looks like Font::isCJKIdeographOrSymbol is used in other places like the TextIterator and GlyphPageTreeNodeMac.cpp.  Can we have a test for those test cases?

> LayoutTests/fast/writing-mode/vertical-subst-font-vert-no-dflt-expected.html:34
> +        setTimeout(function() { testRunner.notifyDone(); }, 100);

Hmm, the 100ms seems like it would be flaky and it's slow.  In fast/writing-mode/broken-ideographic-font.html, it loads the font as an Image and in the error handler, calls notifyDone.  Does that always work?  If so, we should use that pattern instead of waiting.

[Xianzhu Wang]

Comment on attachment 167137 [details]
Patch

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

>> LayoutTests/fast/writing-mode/vertical-subst-font-vert-no-dflt-expected.html:34
>> +        setTimeout(function() { testRunner.notifyDone(); }, 100);
> 
> Hmm, the 100ms seems like it would be flaky and it's slow.  In fast/writing-mode/broken-ideographic-font.html, it loads the font as an Image and in the error handler, calls notifyDone.  Does that always work?  If so, we should use that pattern instead of waiting.

Just checked other tests containing non-local @font-face. fast/css/font-face-remote.html doesn't have such waiting, but uses a body onload event handler defined in body part under the text using the font:

<body onload="finished()">
  ... some text using the font ...
  <script>
    testRunner.waitUntilDone();
    document.body.offsetTop();
    function finished() {
       testRunner.notifyDone();
    }
  </script>
</body>

I'd like this method, but I'm wondering what the different it is compared to the following two methods:
Method 1:
<body>
  ... some text using the font ...
  <script>
    document.body.offsetTop;
  </script>
</body>

Method 2:
<body onload="document.body.offsetTop">
  ... some text using the font ...
</body>

[Tony Chang]

https://bugs.webkit.org/show_bug.cgi?id=73688 seems to suggest that onload fires after the font has finished loading.

Since we get the pixels after onload, maybe you don't need the timeout at all?

[Xianzhu Wang]

(In reply to comment #11)
> https://bugs.webkit.org/show_bug.cgi?id=73688 seems to suggest that onload fires after the font has finished loading.
> 
> Since we get the pixels after onload, maybe you don't need the timeout at all?

Agreed.

I got the impression that remote fonts might be loaded after onload because a test for bug 98100 failed on ews bot but succeeded locally. However later change to the test proved that there should be another reason but I haven't figured out because of lack of information (the ews bot doesn't give layout test failure zip). Finally I converted the test into a text only test.

Now the test in this patch failed on mac-ews. I can only investigate it until I get a mac build environment.

[Xianzhu Wang]

Created attachment 167182 [details]
Patch

[Xianzhu Wang]

Comment on attachment 167137 [details]
Patch

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

>> Source/WebCore/platform/graphics/Font.cpp:560
>> +        return true;
> 
> It looks like Font::isCJKIdeographOrSymbol is used in other places like the TextIterator and GlyphPageTreeNodeMac.cpp.  Can we have a test for those test cases?

I'm afraid I couldn't handle all the paths, so in the last patch I removed two lines from the test to avoid the requirement of adding this. The coverage of the test is still enough to cover the patch itself.

[Build Bot]

Comment on attachment 167182 [details]
Patch

Attachment 167182 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14176252

New failing tests:
fast/writing-mode/vertical-subst-font-vert-no-dflt.html

[Tony Chang]

(In reply to comment #15)
> (From update of attachment 167182 [details])
> Attachment 167182 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/14176252
> 
> New failing tests:
> fast/writing-mode/vertical-subst-font-vert-no-dflt.html

Hmm, maybe there is a race here.  I would try something like what you said in comment #10.

[Xianzhu Wang]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 167182 [details] [details])
> > Attachment 167182 [details] [details] did not pass mac-ews (mac):
> > Output: http://queues.webkit.org/results/14176252
> > 
> > New failing tests:
> > fast/writing-mode/vertical-subst-font-vert-no-dflt.html
> 
> Hmm, maybe there is a race here.  I would try something like what you said in comment #10.

It also failed with the timeout. Other tests under fast/writing-mode using the same timeout succeed on mac. I think it is a real failure (i.e. a bug) on Mac. I really miss layout-test-results.zip.

[Kenichi Ishibashi]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (From update of attachment 167182 [details] [details] [details])
> > > Attachment 167182 [details] [details] [details] did not pass mac-ews (mac):
> > > Output: http://queues.webkit.org/results/14176252
> > > 
> > > New failing tests:
> > > fast/writing-mode/vertical-subst-font-vert-no-dflt.html
> > 
> > Hmm, maybe there is a race here.  I would try something like what you said in comment #10.
> 
> It also failed with the timeout. Other tests under fast/writing-mode using the same timeout succeed on mac. I think it is a real failure (i.e. a bug) on Mac. I really miss layout-test-results.zip.

FYI, I applied the patch and ran the test. It passed on my mac pro (mountain lion). Hmm..

[Xianzhu Wang]

> FYI, I applied the patch and ran the test. It passed on my mac pro (mountain lion). Hmm..

Are you building chromium-mac or pure WebKit mac? I reproduced the failure on mac with a pure WebKit mac. The failure has two reasons:

1) The vertical text in the expected file is wrapped on Mac, but that of the test file is not wrapped. This looks like a bug.

2) I made a mistake about the tortoise shell brackets in the vert GSUB table in the font: mapped square brakets to tortoise shell brackets and used those in the test. (The original DroidSansFallback.ttf doesn't have the mistake. I manually edited the GSUB table because the original table was lost when fontforge saved the file.) The mistake is respected by our opentype code but not by Mac. Hardly to say it a bug of Mac because of the non-standard GSUB table.

Will modify the font and the test to make it pass on Mac. Will file a bug about the problematic vertical layout (1) on Mac.

[Xianzhu Wang]

Created attachment 167386 [details]
for landing

[Xianzhu Wang]

Comment on attachment 167386 [details]
for landing

EWS first.

[Xianzhu Wang]

Created attachment 167411 [details]
For landing. Expect failure on Mac

[Xianzhu Wang]

Created attachment 167412 [details]
For landing. Expect failure on Mac

[WebKit Review Bot]

Comment on attachment 167412 [details]
For landing. Expect failure on Mac

Clearing flags on attachment: 167412

Committed r130570: <http://trac.webkit.org/changeset/130570>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.