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.
Created attachment 166073 [details] Patch for reproducing the bug
Created attachment 166075 [details] The font
Created attachment 166076 [details] expected result
Created attachment 166077 [details] actual result 1
Created attachment 166080 [details] actual result 2 (the 'if' in OpenTypeVerticalData::advanceHeight() commented out)
Could you try the patch at https://bugs.webkit.org/show_bug.cgi?id=97676 ?
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
Created attachment 167137 [details] Patch
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 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>
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?
(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.
Created attachment 167182 [details] Patch
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 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
(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.
(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.
(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..
> 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.
Created attachment 167386 [details] for landing
Comment on attachment 167386 [details] for landing EWS first.
Created attachment 167411 [details] For landing. Expect failure on Mac
Created attachment 167412 [details] For landing. Expect failure on Mac
Comment on attachment 167412 [details] For landing. Expect failure on Mac Clearing flags on attachment: 167412 Committed r130570: <http://trac.webkit.org/changeset/130570>
All reviewed patches have been landed. Closing bug.