Bug 138056

Summary: text-combine needs to center glyphs within the vertical area.
Product: WebKit Reporter: Dave Hyatt <hyatt>
Component: Layout and RenderingAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, esprehn+autocc, glenn, jonlee, kondapallykalyan, mmaxfield, ossy, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
dino: review+
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion none

Description Dave Hyatt 2014-10-24 13:26:53 PDT
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.
Comment 1 Dave Hyatt 2014-10-24 13:29:13 PDT
Created attachment 240433 [details]
Patch
Comment 2 Dave Hyatt 2014-10-24 13:30:42 PDT
Created attachment 240434 [details]
Patch
Comment 3 Myles C. Maxfield 2014-10-24 13:40:49 PDT
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?
Comment 4 Myles C. Maxfield 2014-10-24 13:50:57 PDT
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)
Comment 5 Dave Hyatt 2014-10-24 14:18:57 PDT
(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.
Comment 6 Dave Hyatt 2014-10-24 14:19:42 PDT
(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.
Comment 7 Build Bot 2014-10-24 18:41:53 PDT
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
Comment 8 Build Bot 2014-10-24 19:04:07 PDT
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
Comment 9 Dave Hyatt 2014-10-27 18:00:41 PDT
Landed in r175236.
Comment 10 Csaba Osztrogonác 2014-10-28 06:46:55 PDT
(In reply to comment #9)
> Landed in r175236.

The new test fails on the Apple Mountain Lion and Mavericks bots.
Comment 11 Jon Lee 2014-10-28 09:25:03 PDT
<rdar://problem/15403667>
Comment 12 Alexey Proskuryakov 2014-10-28 11:36:04 PDT
Landed pre-Yosemite results in r175264.