This is a regression from a previous fix to try to fix some off center positioning of tate-chu-yoko.
Created attachment 265587 [details] Patch
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
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
rdar://problem/23536621
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
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 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
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 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
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
Created attachment 265625 [details] Patch
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 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 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.
I am attaching some screenshots to help illustrate the correctness of the patch.
Created attachment 265754 [details] Current broken behavior
Created attachment 265755 [details] Setting the origin to minXMaxYCorner only
Created attachment 265756 [details] Adding in the "centering" step.
Created attachment 265757 [details] Shifting by the height of the entire glyph
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.
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.
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.
Created attachment 265762 [details] Shift by the entire height of the glyph
Created attachment 265763 [details] Apply the centering step
(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.
Created attachment 265831 [details] With Patch
Created attachment 265832 [details] Without Patch
Created attachment 265833 [details] Test Page
Based on the screenshots of the test page, it does not look like this patch is correct.
Comment on attachment 265625 [details] Patch r- based on the screenshots I've uploaded.
Created attachment 265836 [details] Same Test Page with Japanese Characters
It's wrong for glyphs which don't get rotated in vertical-text (like Japanese text)
The difference between the two cases is if (platformData.orientation() == Vertical) inside showGlyphsWithAdvances()
Disregard that last comment. Both are horizontal.
(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.
We need to make sure that during RenderText::width(), style().fontCascade().fontDescription().orientation() is Horizontal.
Created attachment 265847 [details] Needs tests
Created attachment 265849 [details] Patch
Comment on attachment 265849 [details] Patch Clearing flags on attachment: 265849 Committed r192639: <http://trac.webkit.org/changeset/192639>
All reviewed patches have been landed. Closing bug.