[webkit-reviews] review denied: [Bug 49334] Implement Default Ruby Overhang Behavior : [Attachment 74291] Review changes plus change to ignore leading for ruby base text.

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Eric Mader <emader at apple.com>'s request
for review:
Bug 49334: Implement Default Ruby Overhang Behavior
https://bugs.webkit.org/show_bug.cgi?id=49334

Attachment 74291: Review changes plus change to ignore leading for ruby base
text.
https://bugs.webkit.org/attachment.cgi?id=74291&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
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.