Bug 138056 – text-combine needs to center glyphs within the vertical area.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dave Hyatt]

text-combine is not currently using the glyph bounds to center the tatechuyoko in the vertical area. It needs to, since right now the text is being placed too high.

[Dave Hyatt]

Created attachment 240433 [details]
Patch

[Dave Hyatt]

Created attachment 240434 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 240434 [details]
Patch

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

Unofficial r=me

> Source/WebCore/rendering/RenderCombineText.cpp:106
> +    glyphOverflow.computeBounds = true;

Is there a perf regression from this? I'd imagine not because tatechuyoko is pretty rare

> LayoutTests/platform/mac/fast/text/tatechuyoko-expected.txt:1
> +layer at (0,0) size 785x668

There's now way we can test this in a platform-independent way?

[Myles C. Maxfield]

Comment on attachment 240434 [details]
Patch

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

> Source/WebCore/rendering/RenderCombineText.cpp:144
> +        m_combinedTextSize = FloatSize(combinedTextWidth, glyphOverflow.bottom + glyphOverflow.top);

It's probably worth taking glyphOverflow.left and .right into consideration (since at this point they are already computed)

[Dave Hyatt]

(In reply to comment #3)
> Comment on attachment 240434 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240434&action=review
> 
> Unofficial r=me
> 
> > Source/WebCore/rendering/RenderCombineText.cpp:106
> > +    glyphOverflow.computeBounds = true;
> 
> Is there a perf regression from this? I'd imagine not because tatechuyoko is
> pretty rare
> 

Nah, should be fine. iBooks already computes glyph bounds for all text anyway.

> > LayoutTests/platform/mac/fast/text/tatechuyoko-expected.txt:1
> > +layer at (0,0) size 785x668
> 
> There's now way we can test this in a platform-independent way?

Not that I can come up with. It's a unique rendering that can't be duplicated easily. I tried with inline-block and different writing-mode but couldn't get it to line up the same way.

[Dave Hyatt]

(In reply to comment #4)
> Comment on attachment 240434 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240434&action=review
> 
> > Source/WebCore/rendering/RenderCombineText.cpp:144
> > +        m_combinedTextSize = FloatSize(combinedTextWidth, glyphOverflow.bottom + glyphOverflow.top);
> 
> It's probably worth taking glyphOverflow.left and .right into consideration
> (since at this point they are already computed)

left and right are not relevant. The text is already centered in that direction.

[Build Bot]

Created attachment 240448 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Created attachment 240449 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Dave Hyatt]

Landed in r175236.

[Csaba Osztrogonác]

(In reply to comment #9)
> Landed in r175236.

The new test fails on the Apple Mountain Lion and Mavericks bots.

[Jon Lee]

<rdar://problem/15403667>

[Alexey Proskuryakov]

Landed pre-Yosemite results in r175264.