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

Dave Hyatt
Reported 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.
Attachments
Patch (24.59 KB, patch)
2010-11-12 13:16 PST, Dave Hyatt 		no flags
DetailsFormatted DiffDiff
Patch (12.38 KB, patch)
2010-11-12 14:58 PST, Dave Hyatt 		mitz: review+
DetailsFormatted DiffDiff
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+
DetailsFormatted DiffDiff
Patch (97.48 KB, patch)
2010-11-12 21:09 PST, Dave Hyatt 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Dave Hyatt
Comment 1 2010-11-12 13:16:13 PST
Created attachment 73770 [details] Patch Looking for feedback. Not ready to land yet.
WebKit Review Bot
Comment 2 2010-11-12 13:35:19 PST
Attachment 73770 [details] did not build on chromium: Build output: http://queues.webkit.org/results/5730023
Early Warning System Bot
Comment 3 2010-11-12 13:43:41 PST
Attachment 73770 [details] did not build on qt: Build output: http://queues.webkit.org/results/5740035
Yasuo Kida
Comment 4 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.
Dave Hyatt
Comment 5 2010-11-12 14:58:34 PST
Created attachment 73781 [details] Patch
Eric Seidel (no email)
Comment 6 2010-11-12 14:59:17 PST
Attachment 73770 [details] did not build on chromium: Build output: http://queues.webkit.org/results/5750034
mitz
Comment 7 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.
Early Warning System Bot
Comment 8 2010-11-12 15:15:38 PST
Attachment 73781 [details] did not build on qt: Build output: http://queues.webkit.org/results/5774035
Takumi Takano
Comment 9 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...
WebKit Review Bot
Comment 10 2010-11-12 16:14:41 PST
Attachment 73781 [details] did not build on chromium: Build output: http://queues.webkit.org/results/5840003
Dave Hyatt
Comment 11 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.
WebKit Review Bot
Comment 12 2010-11-12 20:54:06 PST
Attachment 73807 [details] did not build on chromium: Build output: http://queues.webkit.org/results/5985004
mitz
Comment 13 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
Early Warning System Bot
Comment 14 2010-11-12 20:58:39 PST
Attachment 73807 [details] did not build on qt: Build output: http://queues.webkit.org/results/6007004
Dave Hyatt
Comment 15 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.
Dave Hyatt
Comment 16 2010-11-12 21:09:12 PST
Created attachment 73808 [details] Patch
Dave Hyatt
Comment 17 2010-11-13 10:46:26 PST
Fixed in r71970.
WebKit Review Bot
Comment 18 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
Note You need to log in before you can comment on or make changes to this bug.