Bug 49464 - Fonts with no vertical information should still render CJK glyphs upright
Summary: Fonts with no vertical information should still render CJK glyphs upright
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P1 Critical
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks: 46123
  Show dependency treegraph
 
Reported: 2010-11-12 11:44 PST by Dave Hyatt
Modified: 2010-11-13 11:51 PST (History)
8 users (show)

See Also:


Attachments
Patch (24.59 KB, patch)
2010-11-12 13:16 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (12.38 KB, patch)
2010-11-12 14:58 PST, Dave Hyatt
mitz: review+
Details | Formatted Diff | Diff
Patch to address Takano's concerns and that fixes @font-face bugs as well. (91.66 KB, patch)
2010-11-12 20:40 PST, Dave Hyatt
mitz: review+
Details | Formatted Diff | Diff
Patch (97.48 KB, patch)
2010-11-12 21:09 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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