[Webkit-unassigned] [Bug 49334] Implement Default Ruby Overhang Behavior

[bugzilla-daemon at webkit.org]

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


Dave Hyatt <hyatt at apple.com> changed:

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




--- Comment #4 from Dave Hyatt <hyatt at apple.com>  2010-11-20 19:45:43 PST ---
(From update of attachment 74291)
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.

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