Bug 62684

Summary: Regression: font-size: 100% may cause ruby text to overlap
Product: WebKit Reporter: Roland Steiner <rolandsteiner>
Component: Layout and RenderingAssignee: Kentaro Hara <haraken>
Status: RESOLVED FIXED    
Severity: Normal CC: emader, eric, haraken, hyatt, mitz, rolandsteiner, simon.fraser, webkit.review.bot
Priority: P2 Keywords: HTML5, Regression
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://people.pwf.cam.ac.uk/ssb22/zhimo.html
Attachments:
Description Flags
Simplified test case
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
fixed patch for commit none

Description Roland Steiner 2011-06-14 17:51:06 PDT
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.
Comment 1 Kentaro Hara 2011-07-24 22:25:32 PDT
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.
Comment 2 Kentaro Hara 2011-07-26 19:44:59 PDT
Created attachment 102092 [details]
Patch
Comment 3 mitz 2011-07-26 20:44:57 PDT
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?
Comment 4 Kentaro Hara 2011-07-27 00:40:34 PDT
Created attachment 102101 [details]
Patch
Comment 5 Kentaro Hara 2011-07-27 00:40:50 PDT
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().
Comment 6 Kentaro Hara 2011-07-27 00:43:37 PDT
> 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".
Comment 7 Kentaro Hara 2011-08-01 19:47:53 PDT
Would you please take a look?
Comment 8 Kentaro Hara 2011-09-12 01:27:58 PDT
Created attachment 107029 [details]
Patch
Comment 9 mitz 2011-09-13 13:35:42 PDT
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.
Comment 10 Kentaro Hara 2011-09-13 17:45:29 PDT
Created attachment 107268 [details]
Patch
Comment 11 Kentaro Hara 2011-09-13 17:46:46 PDT
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 12 mitz 2011-09-15 07:53:05 PDT
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).
Comment 13 Kentaro Hara 2011-09-15 11:10:23 PDT
Created attachment 107512 [details]
Patch
Comment 14 Kentaro Hara 2011-09-15 11:12:26 PDT
(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 15 mitz 2011-09-15 22:44:10 PDT
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).
Comment 16 Kentaro Hara 2011-09-15 22:57:48 PDT
Created attachment 107606 [details]
fixed patch for commit
Comment 17 Kentaro Hara 2011-09-15 23:02:39 PDT
(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!
Comment 18 WebKit Review Bot 2011-09-16 18:35:15 PDT
Comment on attachment 107606 [details]
fixed patch for commit

Clearing flags on attachment: 107606

Committed r95350: <http://trac.webkit.org/changeset/95350>
Comment 19 WebKit Review Bot 2011-09-16 18:35:23 PDT
All reviewed patches have been landed.  Closing bug.