Bug 49334 – Implement Default Ruby Overhang Behavior

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Eric Mader]

Ruby as currently implemented essentially implements ruby-overhang: none. The default behavior is specified as ruby-overhang: auto. In the absence of an implementation of the ruby-overhang property, change the behavior to be ruby-overhang: auto.

[Eric Mader]

Created attachment 74032 [details]
Implement default Ruby Overhang behavior.

This patch includes a lot of changed layout tests, since it changes how ruby text overlaps surrounding text.

[Dave Hyatt]

Comment on attachment 74032 [details]
Implement default Ruby Overhang behavior.

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

I guess I wouldn't bother generating pixel results.  The committer will probably just have to do it.  I see a bunch of new pixel results for tests that have nothing to do with ruby.

> WebCore/ChangeLog:10
> +        (WebCore::RenderBlock::findNextLineBreak): Compute overhang margtins for RenderRubyRun objects.

Typo. margtins -> margins.

> WebCore/rendering/RenderBlockLineLayout.cpp:362
> +#if 0
> +            // This call resets ruby overhang calculations
> +            // done in findNextLineBreak().
>              renderBox->computeLogicalWidth();
> +#endif

Don't #if 0 this.  Just take the code out.

> WebCore/rendering/RenderBlockLineLayout.cpp:1612
> +                    RenderRubyRun* rr = static_cast<RenderRubyRun*> (o);

You have an extra space between <RenderRubyRun*> and (o).

> WebCore/rendering/RenderBlockLineLayout.cpp:1615
> +                    rr->setOverhangMargins(last, bidiNext(this, o));
> +                }

I don't see a need for a local here, since you don't use it for anything.  You can just say

static_cast<RenderRubyRun*>(o)->setOverhangMargins(last, bidiNext(this, o));

> WebCore/rendering/RenderRubyRun.cpp:67
> +    int rubyLength = text? text->maxPreferredLogicalWidth() : 0;
> +    int baseLength = base? base->maxPreferredLogicalWidth() : 0;

It seems like you should just be using logicalWidth() here, but maybe I'm missing something.

> WebCore/rendering/RenderRubyRun.cpp:73
> +#if 0
> +    // Remove height of ruby text from overall height.
> +    if (text)
> +        setMarginBefore(-text->height());
> +#endif

No #if 0 code please.  Just take it out.

> WebCore/rendering/RenderRubyRun.cpp:83
> +            setMarginStart(marginLeft() - (overhang / 2));
> +        

marginLeft() is wrong for vertical text.  It should be marginStart().

> WebCore/rendering/RenderRubyRun.cpp:85
> +        if (nextIsText)
> +            setMarginEnd(marginRight() - (overhang / 2));

marginRight() should be marginEnd().

> WebCore/rendering/RenderRubyRun.cpp:262
> +    

Extra whitespace introduced in two places in RenderRubyRun.  Look at the pretty diff to see it so you can get rid of it.

> WebCore/rendering/RenderRubyRun.h:77
> +    

Accidental extra whitespace here.

[Eric Mader]

Created attachment 74291 [details]
Review changes plus change to ignore leading for ruby base text.

I call maxPreferredLogicalWidth() instead of logicalWidth() because in cases where the ruby text is wider than the base text, base->logicalWidth() seems to return the width of the ruby text. (e.g. in a case where the width of the ruby text was 40 and the width of the base text was 32, base->logicalWidth() returned 40 instead of 32.

[Dave Hyatt]

Comment on attachment 74291 [details]
Review changes plus change to ignore leading for ruby base text.

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

> WebCore/rendering/InlineFlowBox.cpp:560
> -            Length parentLineHeight = curr->renderer()->parent()->style()->lineHeight();
>              bool baselineSet = false;
>              baseline = 0;
>              int baselineToBottom = 0;
>              for (size_t i = 0; i < usedFonts->size(); ++i) {
> -                int halfLeading = (usedFonts->at(i)->lineSpacing() - usedFonts->at(i)->height()) / 2;
> +                // Ignore leading for ruby base so that ruby text is set closer to base.
> +                int halfLeading = curr->renderer()->parent()->isRubyBase() ? 0 :
> +                (usedFonts->at(i)->lineSpacing() - usedFonts->at(i)->ascent() - usedFonts->at(i)->descent()) / 2;

Just discard the changes to InlineFlowBox.cpp.  They aren't correct, and I landed a more comprehensive fix that takes care of the vertical spacing problem.

> WebCore/rendering/RenderRubyRun.cpp:62
> +void RenderRubyRun::setOverhangMargins(RenderObject* last, RenderObject* next)

I have a few questions about setOverhangMargins.

(1) What are we supposed to do about overhang at the start of a line? At the end of the line?  Does the Ruby traditionally spill out above the top or below the bottom of the line in vertical text in that case?  The current patch is going to let Ruby overhang at the start and end of lines.  Is that what we want?  It's pretty easy to detect if the overhang can fit or not, since you have all that info in findNextLineBreak.

(2) Shouldn't there be some kind of cap on the amount we're willing to overhang?  I thought I remembered reading that you shouldn't typically overhang by more than 1 fullwidth character.

(3) There's no consideration for font size differences or vertical position differences.  With this patch the Ruby will push right into adjacent text runs that use larger fonts.  While I think handling the vertical position could be a bit challenging and could be deferred to another patch, it's pretty trivial to just grab the font from the RenderStyles and not overhang if it is larger.

(4) I think you do need to handle the odd pixel and place it somewhere (we traditionally give the odd pixel to the start side) so that there isn't an extra pixel in the base run of text.

[Eric Mader]

Created attachment 78066 [details]
Detect ruby blocks in findNextLineBreak and take overhang into account when determining whether or not the ruby block fits on the ine.

[WebKit Review Bot]

Attachment 78066 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/css/html.css', u'WebCore/rendering/RenderBlockLineLayout.cpp', u'WebCore/rendering/RenderRubyRun.cpp', u'WebCore/rendering/RenderRubyRun.h']" exit_code: 1
WebCore/rendering/RenderBlockLineLayout.cpp:1451:  Declaration has space between type name and * in RenderObject *first  [whitespace/declaration] [3]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Mader]

Here's an updated version of RenderRubyRun::getOverhangMargins that checks font size:


void RenderRubyRun::getOverhangMargins(RenderObject* last, RenderObject* next, int& beforeMargin, int& afterMargin)
{
    RenderRubyText* text = rubyText();
    RenderRubyBase* base = rubyBase();
    int rubyLength = text? text->maxPreferredLogicalWidth() : 0;
    int baseLength = base? base->maxPreferredLogicalWidth() : 0;
    int baseFontSize = base? base->style()->fontSize() : 0;
    
    beforeMargin = afterMargin = 0;
    
    if (baseLength < rubyLength) {
        bool canOverhangLast = last && last->isText() && last->style()->fontSize() <= baseFontSize;
        bool canOverhangNext = next && next->isText() && next->style()->fontSize() <= baseFontSize;
        int overhang = rubyLength - baseLength;
        
        if (canOverhangLast)
            beforeMargin = - ((overhang + 1) / 2);
        
        if (canOverhangNext)
            afterMargin = - (overhang / 2);
    }
}

[Dave Hyatt]

Comment on attachment 78066 [details]
Detect ruby blocks in findNextLineBreak and take overhang into account when determining whether or not the ruby block fits on the ine.

Include changes in a patch please, rather than just posting them in comments.  It's easier to comment on the code if it's in the pretty-printed form.

I still think you need to cap the amount you're willing to overhang.

Also, the change to shrink the font-size to 50% has nothing to do with this bug, and we should just do that in a separate change.

[Dave Hyatt]

I also had this question in my previous review:

"(1) What are we supposed to do about overhang at the start of a line? At the end of the line?  Does the Ruby traditionally spill out above the top or below the bottom of the line in vertical text in that case?  The current patch is going to let Ruby overhang at the start and end of lines.  Is that what we want?  It's pretty easy to detect if the overhang can fit or not, since you have all that info in findNextLineBreak."

[Eric Mader]

(In reply to comment #9)
> I also had this question in my previous review:
> 
> "(1) What are we supposed to do about overhang at the start of a line? At the end of the line?  Does the Ruby traditionally spill out above the top or below the bottom of the line in vertical text in that case?  The current patch is going to let Ruby overhang at the start and end of lines.  Is that what we want?  It's pretty easy to detect if the overhang can fit or not, since you have all that info in findNextLineBreak."

The spec says that the ruby text should not hang into the top or bottom margins. My code checks to see if the ruby block is first on the line and will not permit overhang in that case (by passing 0 as the address of the last block).

To handle the bottom margin, my code does not take any overhang into account when deciding whether or not the ruby block fits on the line. The idea is that if the ruby text will not fit on the line, then the text it would overhang won't fit either.

[Eric Mader]

(In reply to comment #8)
> (From update of attachment 78066 [details])
> Include changes in a patch please, rather than just posting them in comments.  It's easier to comment on the code if it's in the pretty-printed form.

OK. I was in a time crunch and this was faster than re-generating the patch. I'll take the time to do it right from now on.

> I still think you need to cap the amount you're willing to overhang.

OK. I can cap it based on the point size of the base text without too  much trouble, I think.

> Also, the change to shrink the font-size to 50% has nothing to do with this bug, and we should just do that in a separate change.

My thinking was that changing the font-size to 50% reduced the cases where overhang would be required, but I'm happy to make this a separate change, especially since I also need to special-case the enforcement of the minimum point size for ruby text.

[Eric Mader]

Created attachment 79528 [details]
Updated patch per previous review comments. Limit overhang to 1/2 of the point size of the base text. Remove change to ruby text-size.

Patch should also prevent ruby text from overhanging into the margins.

[WebKit Review Bot]

Attachment 79528 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/RenderBlockLineLayout.cpp:1452:  Declaration has space between type name and * in RenderObject *first  [whitespace/declaration] [3]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 88179 [details]
Patch and layout tests

Fixed in r82903. <http://trac.webkit.org/changeset/82903>

[WebKit Review Bot]

http://trac.webkit.org/changeset/82903 might have broken Qt Linux Release
The following tests are not passing:
fast/ruby/overhang-horizontal.html
fast/ruby/overhang-vertical.html

(In reply to comment #16)
> http://trac.webkit.org/changeset/82903 might have broken Qt Linux Release
> The following tests are not passing:
> fast/ruby/overhang-horizontal.html
> fast/ruby/overhang-vertical.html

See message:[e-mail]%3E

[Ryosuke Niwa]

It seems like we need a rebaseline for Windows & GTK:
http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r82906%20(11218)/fast/blockflow/Kusa-Makura-background-canvas-pretty-diff.html
http://build.webkit.org/results/GTK%20Linux%2032-bit%20Release/r82906%20(12385)/results.html

[Roland Steiner]

For generated :before/:after content, this will be broken by the patch proposed in bug 55930. Do you have a suggestion how to consolidate both patches?