Bug 62684 – Regression: font-size: 100% may cause ruby text to overlap

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Roland Steiner]

Created attachment 97205 [details]
Simplified test case

Originally reported at: http://code.google.com/p/chromium/issues/detail?id=86079

Setting font-size: 100% for <rt> may cause the ruby text to overlap (In the linked page the author uses this to improve legibility). This is a regression from previous versions, which makes me think the code for ruby overhang might be the culprit.

See above Chromium bug entry for more detail and a screenshot.

[Kentaro Hara]

I confirmed on Mac Safari that this is a regression caused by r82903 (https://bugs.webkit.org/show_bug.cgi?id=49334). I am making a patch.

[Kentaro Hara]

Created attachment 102092 [details]
Patch

Comment on attachment 102092 [details]
Patch

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

This bug is about the overlap, but the patch changes other things. Most notably, it allows greater overhang (more than half a ruby) in some cases. If you want to change the maximum amount of overhang allowed, file a separate bug and explain why you want to change that. The current implementation follows the example in <http://www.w3.org/TR/2009/NOTE-jlreq-20090604/#en-subheading2_3_6>.

> 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?

[Kentaro Hara]

Created attachment 102101 [details]
Patch

[Kentaro Hara]

Mitz, thank you very much for the comments.

(In reply to comment #3)
> (From update of attachment 102092 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102092&action=review
> 
> This bug is about the overlap, but the patch changes other things. Most notably, it allows greater overhang (more than half a ruby) in some cases. If you want to change the maximum amount of overhang allowed, file a separate bug and explain why you want to change that. The current implementation follows the example in <http://www.w3.org/TR/2009/NOTE-jlreq-20090604/#en-subheading2_3_6>.

Sorry, I wanted to fix the bug in this patch, but my change was wrong. I fixed the patch as follows:

before:
logicalLeftOverhang = min<int>(toRenderText(startRenderer)->minLogicalWidth(), startRenderer->style(firstLine)->fontSize()) / 2;

after:
logicalLeftOverhang = min<int>(toRenderText(startRenderer)->minLogicalWidth(), rubyText->style(firstLine)->fontSize()) / 2;

My intention is that the length of a ruby overhang should be no more than the width of the ruby "and no more than the width of the neighboring text block". 

I confirmed that the test case attached above and http://people.pwf.cam.ac.uk/ssb22/zhimo.html (reported by http://code.google.com/p/chromium/issues/detail?id=86079) are rendered correctly with my patch. 

> > 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. 

The other place is RenderBlock::LineBreaker::nextLineBreak() (it calls applyOverhang() and the applyOverhang() calls getOverhang()), but it does not swap them. So I added the code to swap them when RenderBlock::LineBreaker::nextLineBreak() calls applyOverhang().

[Kentaro Hara]

> My intention is that the length of a ruby overhang should be no more than the width of the ruby "and no more than the width of the neighboring text block".

Correct: My intention is that the length of a ruby overhang should be no more than half the width of the ruby's font size "and no more than half the width of the neighboring text block".

[Kentaro Hara]

Would you please take a look?

[Kentaro Hara]

Created attachment 107029 [details]
Patch

Comment on attachment 107029 [details]
Patch

(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.

[Kentaro Hara]

Created attachment 107268 [details]
Patch

[Kentaro Hara]

Thank you, I got it.

> 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.

Done.

Comment on attachment 107268 [details]
Patch

Thanks. This looks very good, but I think you can still improve it. Instead of adjusting the left/right overhang in the beginning of the function, which requires you to query the writing direction and the ruby text size twice, why not wait until after startOverhang and endOverhang are set? At that point, in the end of the function, you can simply clamp startOverhang down to half the width of the startRenderer (and similarly for the end). You won’t even need to repeat the isText() checks, because you could just check if the overhang is already 0 (if it is, you don’t need to do anything; if it’s not, then the adjacent renderer is text).

[Kentaro Hara]

Created attachment 107512 [details]
Patch

[Kentaro Hara]

(In reply to comment #12)
> (From update of attachment 107268 [details])
> Thanks. This looks very good, but I think you can still improve it. Instead of adjusting the left/right overhang in the beginning of the function, which requires you to query the writing direction and the ruby text size twice, why not wait until after startOverhang and endOverhang are set? At that point, in the end of the function, you can simply clamp startOverhang down to half the width of the startRenderer (and similarly for the end). You won’t even need to repeat the isText() checks, because you could just check if the overhang is already 0 (if it is, you don’t need to do anything; if it’s not, then the adjacent renderer is text).

mitz: Thank you, Done.

Comment on attachment 107512 [details]
Patch

I’m not sure why it’s necessary to move the “half the ruby text font size” limit from the beginning to the end (and to potentially evaluate rubyText->style(firstLine)->fontSize()) twice).

[Kentaro Hara]

Created attachment 107606 [details]
fixed patch for commit

[Kentaro Hara]

(In reply to comment #15)
> (From update of attachment 107512 [details])
> I’m not sure why it’s necessary to move the “half the ruby text font size” limit from the beginning to the end (and to potentially evaluate rubyText->style(firstLine)->fontSize()) twice).

Fixed and committed. Thank you very much for reviewing this!

[WebKit Review Bot]

Comment on attachment 107606 [details]
fixed patch for commit

Clearing flags on attachment: 107606

Committed r95350: <http://trac.webkit.org/changeset/95350>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.