Bug 97824

Summary:

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

Product:

WebKit

Reporter:

Xianzhu Wang <wangxianzhu>

Component:

Platform

Assignee:

Xianzhu Wang <wangxianzhu>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bashi, kojii, peter, senorblanco, tkent, tony, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

98100

Bug Blocks:

97261

Attachments:

Description	Flags
Patch for reproducing the bug	none
The font	none
expected result	none
actual result 1	none
actual result 2 (the 'if' in OpenTypeVerticalData::advanceHeight() commented out)	none
Result with patch of bug 97676	none
Patch	none
Patch	none
for landing	none
For landing. Expect failure on Mac	none
For landing. Expect failure on Mac	none

Xianzhu Wang

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

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
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Xianzhu Wang

Comment 1 2012-09-27 15:33:23 PDT

Created attachment 166073 [details] Patch for reproducing the bug

Xianzhu Wang

Comment 2 2012-09-27 15:33:43 PDT

Created attachment 166075 [details] The font

Xianzhu Wang

Comment 3 2012-09-27 15:34:38 PDT

Created attachment 166076 [details] expected result

Xianzhu Wang

Comment 4 2012-09-27 15:36:12 PDT

Created attachment 166077 [details] actual result 1

Xianzhu Wang

Comment 5 2012-09-27 15:42:32 PDT

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

Kenichi Ishibashi

Comment 6 2012-09-27 15:59:04 PDT

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

Xianzhu Wang

Comment 7 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

Xianzhu Wang

Comment 8 2012-10-04 11:09:13 PDT

Created attachment 167137 [details] Patch

Tony Chang

Comment 9 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.

Xianzhu Wang

Comment 10 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>

Tony Chang

Comment 11 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?

Xianzhu Wang

Comment 12 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.

Xianzhu Wang

Comment 13 2012-10-04 15:02:25 PDT

Created attachment 167182 [details] Patch

Xianzhu Wang

Comment 14 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.

Build Bot

Comment 15 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

Tony Chang

Comment 16 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.

Xianzhu Wang

Comment 17 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.

Kenichi Ishibashi

Comment 18 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..

Xianzhu Wang

Comment 19 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.

Xianzhu Wang

Comment 20 2012-10-05 14:49:40 PDT

Created attachment 167386 [details] for landing

Xianzhu Wang

Comment 21 2012-10-05 14:50:34 PDT

Comment on attachment 167386 [details] for landing EWS first.

Xianzhu Wang

Comment 22 2012-10-05 16:27:08 PDT

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

Xianzhu Wang

Comment 23 2012-10-05 16:29:18 PDT

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

WebKit Review Bot

Comment 24 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>

WebKit Review Bot

Comment 25 2012-10-05 17:38:11 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.