Summary: | Regression: font-size: 100% may cause ruby text to overlap | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Roland Steiner <rolandsteiner> | ||||||||||||||||
Component: | Layout and Rendering | Assignee: | 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
Roland Steiner
2011-06-14 17:51:06 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. 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? Created attachment 102101 [details]
Patch
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(). > 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".
Would you please take a look? 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. Created attachment 107268 [details]
Patch
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).
Created attachment 107512 [details]
Patch
(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).
Created attachment 107606 [details]
fixed patch for commit
(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 on attachment 107606 [details] fixed patch for commit Clearing flags on attachment: 107606 Committed r95350: <http://trac.webkit.org/changeset/95350> All reviewed patches have been landed. Closing bug. |