Bug 49464

Summary: Fonts with no vertical information should still render CJK glyphs upright
Product: WebKit Reporter: Dave Hyatt <hyatt>
Component: Layout and RenderingAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Critical CC: abarth, dglazkov, eric, Kida, mitz, takano, webkit-ews, webkit.review.bot
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 46123    
Attachments:
Description Flags
Patch
none
Patch
mitz: review+
Patch to address Takano's concerns and that fixes @font-face bugs as well.
mitz: review+
Patch none

Description Dave Hyatt 2010-11-12 11:44:06 PST
Fonts with no vertical information should still render CJK glyphs upright.  See:

http://people.mozilla.org/~jdaggett/tests/verticalmargins.html

The font is Times-Roman, and it has Japanese glyphs but no vertical metrics.  The glyphs should still render upright.

Basically our code that is trying to make this decision purely based off the entire font is too simplistic.  We need to do better.
Comment 1 Dave Hyatt 2010-11-12 13:16:13 PST
Created attachment 73770 [details]
Patch

Looking for feedback.  Not ready to land yet.
Comment 2 WebKit Review Bot 2010-11-12 13:35:19 PST
Attachment 73770 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5730023
Comment 3 Early Warning System Bot 2010-11-12 13:43:41 PST
Attachment 73770 [details] did not build on qt:
Build output: http://queues.webkit.org/results/5740035
Comment 4 Yasuo Kida 2010-11-12 13:53:32 PST
I am not sure that this is an issue with missing vertical metric. Times font does not have Japanese glyphs and you are seeing them through the font fallback. The font used in the fallback is Hiragino, which has vertical metric. I guess the issue is around the handling of fallback fonts.
Comment 5 Dave Hyatt 2010-11-12 14:58:34 PST
Created attachment 73781 [details]
Patch
Comment 6 Eric Seidel (no email) 2010-11-12 14:59:17 PST
Attachment 73770 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5750034
Comment 7 mitz 2010-11-12 15:07:20 PST
Comment on attachment 73781 [details]
Patch

Looks ok but would be good to test with a font that actually has CJK but not vertical support.
Comment 8 Early Warning System Bot 2010-11-12 15:15:38 PST
Attachment 73781 [details] did not build on qt:
Build output: http://queues.webkit.org/results/5774035
Comment 9 Takumi Takano 2010-11-12 15:58:48 PST
It seems Font::isCJKIdeograph doesn't check CJK Extension B, C, D and CJK Compatibility Ideographs Supplement. They are mapped U+20000 and up. We need to count surrogate pairs here...
Comment 10 WebKit Review Bot 2010-11-12 16:14:41 PST
Attachment 73781 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5840003
Comment 11 Dave Hyatt 2010-11-12 20:40:50 PST
Created attachment 73807 [details]
Patch to address Takano's concerns and that fixes @font-face bugs as well.
Comment 12 WebKit Review Bot 2010-11-12 20:54:06 PST
Attachment 73807 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5985004
Comment 13 mitz 2010-11-12 20:58:32 PST
Comment on attachment 73807 [details]
Patch to address Takano's concerns and that fixes @font-face bugs as well.

r=me, but try not to break cr-linux and efl
Comment 14 Early Warning System Bot 2010-11-12 20:58:39 PST
Attachment 73807 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6007004
Comment 15 Dave Hyatt 2010-11-12 21:08:12 PST
Going to keep using the bug to post patches to get the other ports building.  This is just a matter of getting orientation() into everyone's FontPlatformData object, even if it just returns Horizontal on non-Mac platforms for now.
Comment 16 Dave Hyatt 2010-11-12 21:09:12 PST
Created attachment 73808 [details]
Patch
Comment 17 Dave Hyatt 2010-11-13 10:46:26 PST
Fixed in r71970.
Comment 18 WebKit Review Bot 2010-11-13 11:51:14 PST
http://trac.webkit.org/changeset/71970 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
fast/blockflow/broken-ideographic-font.html