[Webkit-unassigned] [Bug 62684] Regression: font-size: 100% may cause ruby text to overlap

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=62684


mitz at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #107029|review?                     |review-
               Flag|                            |




--- Comment #9 from mitz at webkit.org  2011-09-13 13:35:42 PST ---
(From update of attachment 107029)
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.