[webkit-reviews] review denied: [Bug 62684] Regression: font-size: 100% may cause ruby text to overlap : [Attachment 107029] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Kentaro Hara <haraken at google.com>'s request for
review:
Bug 62684: Regression: font-size: 100% may cause ruby text to overlap
https://bugs.webkit.org/show_bug.cgi?id=62684

Attachment 107029: Patch
https://bugs.webkit.org/attachment.cgi?id=107029&action=review

------- Additional Comments from mitz at webkit.org
(In reply to comment #5)
> > > Source/WebCore/rendering/RenderRubyRun.cpp:298
> > > +    int logicalLeftOverhang = 0;
> > > +    if (startRenderer && startRenderer->isText())
> > > +        logicalLeftOverhang =
min<int>(toRenderText(startRenderer)->minLogicalWidth(),
startRenderer->style(firstLine)->fontSize()) / 2;
> > > +    int logicalRightOverhang = 0;
> > > +    if (endRenderer && endRenderer->isText())
> > > +        logicalRightOverhang =
min<int>(toRenderText(endRenderer)->minLogicalWidth(),
endRenderer->style(firstLine)->fontSize()) / 2;
> > 
> > Why is it okay to set the logical left overhang based on the startRenderer
regardless of writing direction?
> 
> There are two places where call getOverhang(). 
> 
> One place is RenderBlock::setMarginsForRubyRun() and it swaps startRenderer
and endRenderer before calling getOverhang(). It is OK. 

In setMarginsForRubyRun(), the previousObject and nextObject variables point to
objects that occur before and after the ruby, respectively, in BidiRun order,
which is visual left-to-right order (in horizontal writing modes); in the
right-to-left case, the start renderer is the one on the right, namely the
nextObject.

> The other place is RenderBlock::LineBreaker::nextLineBreak() (it calls
applyOverhang() and the applyOverhang() calls getOverhang()), but it does not
swap them.

nextLineBreak() iterates over the objects on the line in logical order. It
passes the object that precedes the ruby ('last') as the start renderer to
applyOverhang() and the object that comes after the ruby ('next') as the end
renderer. applyOverhang() passes those start and end renderers through to
getOverhang().

So I added the code to swap them when RenderBlock::LineBreaker::nextLineBreak()
calls applyOverhang().

That swapping is wrong. It breaks this case:

<div style="direction: rtl; unicode-bidi: bidi-override; font: 20px ahem;
border: solid blue; width: 45px;">
    a<ruby>b<rt>cde</rt></ruby><span style="font-size: 25px;">b</span>
</div>

You should only patch RenderRubyRun::getOverhang() and your change should check
the writing direction (or using the existing check in that function) rather
than map start to left as you did.