Bug 151309 - Tatechuyoko in ruby sits too high
Summary: Tatechuyoko in ruby sits too high
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-11-16 09:42 PST by Dave Hyatt
Modified: 2015-11-19 11:16 PST (History)
10 users (show)

See Also:


Attachments
Patch (122.46 KB, patch)
2015-11-16 09:47 PST, Dave Hyatt
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (643.70 KB, application/zip)
2015-11-16 10:31 PST, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (747.24 KB, application/zip)
2015-11-16 10:35 PST, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (775.99 KB, application/zip)
2015-11-16 10:42 PST, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-yosemite (774.50 KB, application/zip)
2015-11-16 12:29 PST, Build Bot
no flags Details
Patch (123.39 KB, patch)
2015-11-16 14:51 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Current broken behavior (8.49 KB, image/png)
2015-11-18 11:13 PST, Dave Hyatt
no flags Details
Setting the origin to minXMaxYCorner only (9.06 KB, image/png)
2015-11-18 11:14 PST, Dave Hyatt
no flags Details
Adding in the "centering" step. (deleted)
2015-11-18 11:14 PST, Dave Hyatt
no flags Details
Shifting by the height of the entire glyph (deleted)
2015-11-18 11:15 PST, Dave Hyatt
no flags Details
Shift by the entire height of the glyph (9.81 KB, image/png)
2015-11-18 11:24 PST, Dave Hyatt
no flags Details
Apply the centering step (10.11 KB, image/png)
2015-11-18 11:25 PST, Dave Hyatt
no flags Details
With Patch (72.63 KB, image/png)
2015-11-18 19:31 PST, Myles C. Maxfield
no flags Details
Without Patch (73.13 KB, image/png)
2015-11-18 19:31 PST, Myles C. Maxfield
no flags Details
Test Page (231 bytes, text/html)
2015-11-18 19:31 PST, Myles C. Maxfield
no flags Details
Same Test Page with Japanese Characters (236 bytes, text/html)
2015-11-18 19:46 PST, Myles C. Maxfield
no flags Details
Needs tests (3.83 KB, patch)
2015-11-18 22:10 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (8.17 KB, patch)
2015-11-18 22:38 PST, Myles C. Maxfield
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 2015-11-16 09:42:54 PST
This is a regression from a previous fix to try to fix some off center positioning of tate-chu-yoko.
Comment 1 Dave Hyatt 2015-11-16 09:47:08 PST
Created attachment 265587 [details]
Patch
Comment 2 Build Bot 2015-11-16 10:31:27 PST
Comment on attachment 265587 [details]
Patch

Attachment 265587 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/437640

New failing tests:
fast/text/text-combine-rendering.html
Comment 3 Build Bot 2015-11-16 10:31:29 PST
Created attachment 265589 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 4 Jon Lee 2015-11-16 10:33:24 PST
rdar://problem/23536621
Comment 5 Build Bot 2015-11-16 10:35:50 PST
Comment on attachment 265587 [details]
Patch

Attachment 265587 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/437648

New failing tests:
fast/text/text-combine-rendering.html
Comment 6 Build Bot 2015-11-16 10:35:52 PST
Created attachment 265590 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 7 Build Bot 2015-11-16 10:42:13 PST
Comment on attachment 265587 [details]
Patch

Attachment 265587 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/437641

New failing tests:
fast/text/text-combine-rendering.html
Comment 8 Build Bot 2015-11-16 10:42:15 PST
Created attachment 265591 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 9 Build Bot 2015-11-16 12:29:20 PST
Comment on attachment 265587 [details]
Patch

Attachment 265587 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/437926

New failing tests:
fast/text/text-combine-rendering.html
Comment 10 Build Bot 2015-11-16 12:29:23 PST
Created attachment 265604 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 11 Dave Hyatt 2015-11-16 14:51:29 PST
Created attachment 265625 [details]
Patch
Comment 12 WebKit Commit Bot 2015-11-16 14:54:24 PST
Attachment 265625 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Myles C. Maxfield 2015-11-16 20:29:45 PST
Comment on attachment 265587 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265587&action=review

> Source/WebCore/rendering/RenderCombineText.cpp:82
> +    result.move(0, m_combinedTextSize.height());

Let's discuss this tomorrow. I'm not sure this is right.
Comment 14 Myles C. Maxfield 2015-11-17 12:20:59 PST
Comment on attachment 265625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265625&action=review

> Source/WebCore/rendering/RenderCombineText.cpp:82
> +    result.move(0, m_combinedTextSize.height());

I'm going to investigate this.
Comment 15 Dave Hyatt 2015-11-18 11:13:20 PST
I am attaching some screenshots to help illustrate the correctness of the patch.
Comment 16 Dave Hyatt 2015-11-18 11:13:45 PST
Created attachment 265754 [details]
Current broken behavior
Comment 17 Dave Hyatt 2015-11-18 11:14:15 PST
Created attachment 265755 [details]
Setting the origin to minXMaxYCorner only
Comment 18 Dave Hyatt 2015-11-18 11:14:37 PST
Created attachment 265756 [details]
Adding in the "centering" step.
Comment 19 Dave Hyatt 2015-11-18 11:15:03 PST
Created attachment 265757 [details]
Shifting by the height of the entire glyph
Comment 20 Dave Hyatt 2015-11-18 11:20:04 PST
I think these screenshots make it pretty obvious that my patch is correct.

When you set the origin just to the minXMaxYCorner of the box, the entire glyph starts just outside the box with its bottom edge aligned with the top edge of the box. Shifting down by the entire glyph height (top + bottom) puts the top edge of the glyph flush with the top edge. Then all that remains is to center.
Comment 21 Dave Hyatt 2015-11-18 11:21:24 PST
The content of attachment 265756 [details] has been deleted by
    Dave Hyatt <hyatt@apple.com>
without providing any reason.

The token used to delete this attachment was generated at 2015-11-18 11:21:21 PST.
Comment 22 Dave Hyatt 2015-11-18 11:21:42 PST
The content of attachment 265757 [details] has been deleted by
    Dave Hyatt <hyatt@apple.com>
without providing any reason.

The token used to delete this attachment was generated at 2015-11-18 11:21:40 PST.
Comment 23 Dave Hyatt 2015-11-18 11:24:20 PST
Created attachment 265762 [details]
Shift by the entire height of the glyph
Comment 24 Dave Hyatt 2015-11-18 11:25:31 PST
Created attachment 265763 [details]
Apply the centering step
Comment 25 Myles C. Maxfield 2015-11-18 14:04:17 PST
(In reply to comment #24)
> Created attachment 265763 [details]
> Apply the centering step

Your approach and mine differ in that you are lowering the glyph by a distance of glyphOverflow.bottom. However, the screenshots you uploaded use glyphs which don't include any descenders, thereby making glyphOverflow.bottom = 0.
Comment 26 Myles C. Maxfield 2015-11-18 19:31:09 PST
Created attachment 265831 [details]
With Patch
Comment 27 Myles C. Maxfield 2015-11-18 19:31:28 PST
Created attachment 265832 [details]
Without Patch
Comment 28 Myles C. Maxfield 2015-11-18 19:31:57 PST
Created attachment 265833 [details]
Test Page
Comment 29 Myles C. Maxfield 2015-11-18 19:32:24 PST
Based on the screenshots of the test page, it does not look like this patch is correct.
Comment 30 Myles C. Maxfield 2015-11-18 19:33:21 PST
Comment on attachment 265625 [details]
Patch

r- based on the screenshots I've uploaded.
Comment 31 Myles C. Maxfield 2015-11-18 19:46:18 PST
Created attachment 265836 [details]
Same Test Page with Japanese Characters
Comment 32 Myles C. Maxfield 2015-11-18 19:57:10 PST
It's wrong for glyphs which don't get rotated in vertical-text (like Japanese text)
Comment 33 Myles C. Maxfield 2015-11-18 20:05:21 PST
The difference between the two cases is if (platformData.orientation() == Vertical) inside showGlyphsWithAdvances()
Comment 34 Myles C. Maxfield 2015-11-18 20:33:53 PST
Disregard that last comment. Both are horizontal.
Comment 35 Myles C. Maxfield 2015-11-18 21:05:07 PST
(In reply to comment #34)
> Disregard that last comment. Both are horizontal.

This is because when we call RenderText::width() inside RenderCombineText::combineText(), this computes the width incorporating textOrientation and writingMode.

Therefore, for CJK characters, this "width" returns the distance from the top of the glyph to the bottom of the glyph. For Latin characters, this "width" returns the distance from the left of the glyph to the right of the glyph.

However, when combining the text, we always want the width to be from the left of the glyph to the right of the glyph. (Or the other way - the important thing is that we are consistent between CJK & Latin.)

The reason Hyatt's patch appears to work (even though it doesn't) is because CJK characters are often squarish and often don't have much descent.
Comment 36 Myles C. Maxfield 2015-11-18 21:55:56 PST
We need to make sure that during RenderText::width(), style().fontCascade().fontDescription().orientation() is Horizontal.
Comment 37 Myles C. Maxfield 2015-11-18 22:10:24 PST
Created attachment 265847 [details]
Needs tests
Comment 38 Myles C. Maxfield 2015-11-18 22:38:55 PST
Created attachment 265849 [details]
Patch
Comment 39 WebKit Commit Bot 2015-11-19 11:16:51 PST
Comment on attachment 265849 [details]
Patch

Clearing flags on attachment: 265849

Committed r192639: <http://trac.webkit.org/changeset/192639>
Comment 40 WebKit Commit Bot 2015-11-19 11:16:59 PST
All reviewed patches have been landed.  Closing bug.