Bug 151309 – Tatechuyoko in ruby sits too high

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dave Hyatt]

This is a regression from a previous fix to try to fix some off center positioning of tate-chu-yoko.

[Dave Hyatt]

Created attachment 265587 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Jon Lee]

rdar://problem/23536621

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Dave Hyatt]

Created attachment 265625 [details]
Patch

[WebKit Commit Bot]

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.

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

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.

[Dave Hyatt]

I am attaching some screenshots to help illustrate the correctness of the patch.

[Dave Hyatt]

Created attachment 265754 [details]
Current broken behavior

[Dave Hyatt]

Created attachment 265755 [details]
Setting the origin to minXMaxYCorner only

[Dave Hyatt]

Created attachment 265756 [details]
Adding in the "centering" step.

[Dave Hyatt]

Created attachment 265757 [details]
Shifting by the height of the entire glyph

[Dave Hyatt]

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.

[Dave Hyatt]

The content of attachment 265756 [details] has been deleted by
    Dave Hyatt <[e-mail]>
without providing any reason.

The token used to delete this attachment was generated at 2015-11-18 11:21:21 PST.

[Dave Hyatt]

The content of attachment 265757 [details] has been deleted by
    Dave Hyatt <[e-mail]>
without providing any reason.

The token used to delete this attachment was generated at 2015-11-18 11:21:40 PST.

[Dave Hyatt]

Created attachment 265762 [details]
Shift by the entire height of the glyph

[Dave Hyatt]

Created attachment 265763 [details]
Apply the centering step

[Myles C. Maxfield]

(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.

[Myles C. Maxfield]

Created attachment 265831 [details]
With Patch

[Myles C. Maxfield]

Created attachment 265832 [details]
Without Patch

[Myles C. Maxfield]

Created attachment 265833 [details]
Test Page

[Myles C. Maxfield]

Based on the screenshots of the test page, it does not look like this patch is correct.

[Myles C. Maxfield]

Comment on attachment 265625 [details]
Patch

r- based on the screenshots I've uploaded.

[Myles C. Maxfield]

Created attachment 265836 [details]
Same Test Page with Japanese Characters

[Myles C. Maxfield]

It's wrong for glyphs which don't get rotated in vertical-text (like Japanese text)

[Myles C. Maxfield]

The difference between the two cases is if (platformData.orientation() == Vertical) inside showGlyphsWithAdvances()

[Myles C. Maxfield]

Disregard that last comment. Both are horizontal.

[Myles C. Maxfield]

(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.

[Myles C. Maxfield]

We need to make sure that during RenderText::width(), style().fontCascade().fontDescription().orientation() is Horizontal.

[Myles C. Maxfield]

Created attachment 265847 [details]
Needs tests

[Myles C. Maxfield]

Created attachment 265849 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 265849 [details]
Patch

Clearing flags on attachment: 265849

Committed r192639: <http://trac.webkit.org/changeset/192639>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.