Bug 97824 - OpenTypeVerticalData issue with DroidSansFallback.ttf on chromium-android and chromium-linux
Summary: OpenTypeVerticalData issue with DroidSansFallback.ttf on chromium-android and...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xianzhu Wang
URL:
Keywords:
Depends on: 98100
Blocks: 97261
  Show dependency treegraph
 
Reported: 2012-09-27 15:32 PDT by Xianzhu Wang
Modified: 2012-10-05 17:38 PDT (History)
7 users (show)

See Also:


Attachments
Patch for reproducing the bug (1.55 KB, patch)
2012-09-27 15:33 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
The font (3.66 MB, application/x-font-ttf)
2012-09-27 15:33 PDT, Xianzhu Wang
no flags Details
expected result (48 bytes, text/plain)
2012-09-27 15:34 PDT, Xianzhu Wang
no flags Details
actual result 1 (5.90 KB, image/png)
2012-09-27 15:36 PDT, Xianzhu Wang
no flags Details
actual result 2 (the 'if' in OpenTypeVerticalData::advanceHeight() commented out) (8.78 KB, image/png)
2012-09-27 15:42 PDT, Xianzhu Wang
no flags Details
Result with patch of bug 97676 (8.64 KB, image/png)
2012-09-27 17:35 PDT, Xianzhu Wang
no flags Details
Patch (8.13 KB, patch)
2012-10-04 11:09 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
Patch (6.63 KB, patch)
2012-10-04 15:02 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
for landing (7.30 KB, patch)
2012-10-05 14:49 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
For landing. Expect failure on Mac (8.28 KB, patch)
2012-10-05 16:27 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
For landing. Expect failure on Mac (8.13 KB, patch)
2012-10-05 16:29 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xianzhu Wang 2012-09-27 15:32:04 PDT
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.
Comment 1 Xianzhu Wang 2012-09-27 15:33:23 PDT
Created attachment 166073 [details]
Patch for reproducing the bug
Comment 2 Xianzhu Wang 2012-09-27 15:33:43 PDT
Created attachment 166075 [details]
The font
Comment 3 Xianzhu Wang 2012-09-27 15:34:38 PDT
Created attachment 166076 [details]
expected result
Comment 4 Xianzhu Wang 2012-09-27 15:36:12 PDT
Created attachment 166077 [details]
actual result 1
Comment 5 Xianzhu Wang 2012-09-27 15:42:32 PDT
Created attachment 166080 [details]
actual result 2 (the 'if' in OpenTypeVerticalData::advanceHeight() commented out)
Comment 6 Kenichi Ishibashi 2012-09-27 15:59:04 PDT
Could you try the patch at https://bugs.webkit.org/show_bug.cgi?id=97676 ?
Comment 7 Xianzhu Wang 2012-09-27 17:35:35 PDT
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
Comment 8 Xianzhu Wang 2012-10-04 11:09:13 PDT
Created attachment 167137 [details]
Patch
Comment 9 Tony Chang 2012-10-04 11:48:44 PDT
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.
Comment 10 Xianzhu Wang 2012-10-04 12:25:08 PDT
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>
Comment 11 Tony Chang 2012-10-04 13:17:10 PDT
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?
Comment 12 Xianzhu Wang 2012-10-04 14:48:18 PDT
(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.
Comment 13 Xianzhu Wang 2012-10-04 15:02:25 PDT
Created attachment 167182 [details]
Patch
Comment 14 Xianzhu Wang 2012-10-04 15:06:28 PDT
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.
Comment 15 Build Bot 2012-10-04 15:28:17 PDT
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
Comment 16 Tony Chang 2012-10-04 15:33:05 PDT
(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.
Comment 17 Xianzhu Wang 2012-10-04 15:46:12 PDT
(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.
Comment 18 Kenichi Ishibashi 2012-10-04 16:24:48 PDT
(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..
Comment 19 Xianzhu Wang 2012-10-05 14:48:16 PDT
> 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.
Comment 20 Xianzhu Wang 2012-10-05 14:49:40 PDT
Created attachment 167386 [details]
for landing
Comment 21 Xianzhu Wang 2012-10-05 14:50:34 PDT
Comment on attachment 167386 [details]
for landing

EWS first.
Comment 22 Xianzhu Wang 2012-10-05 16:27:08 PDT
Created attachment 167411 [details]
For landing. Expect failure on Mac
Comment 23 Xianzhu Wang 2012-10-05 16:29:18 PDT
Created attachment 167412 [details]
For landing. Expect failure on Mac
Comment 24 WebKit Review Bot 2012-10-05 17:38:07 PDT
Comment on attachment 167412 [details]
For landing. Expect failure on Mac

Clearing flags on attachment: 167412

Committed r130570: <http://trac.webkit.org/changeset/130570>
Comment 25 WebKit Review Bot 2012-10-05 17:38:11 PDT
All reviewed patches have been landed.  Closing bug.