[webkit-reviews] review denied: [Bug 137421] Inline ruby does not get justified correctly : [Attachment 239280] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 137421: Inline ruby does not get justified correctly
https://bugs.webkit.org/show_bug.cgi?id=137421

Attachment 239280: Patch
https://bugs.webkit.org/attachment.cgi?id=239280&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=239280&action=review


r-

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:582
> +	   if (auto* rubyText = rubyRun.rubyText())
> +	       rubyText->setLogicalLeft(rubyText->logicalLeft() +
totalExpansion / 2);

Everything looks fine except for this part. You can't just move the ruby text
and assume that's all that was needed. Ruby text could change its size and
position based off the additional width gained by the ruby base. A few test
cases to consider that would illustrate problems with this approach: (a)
text-align: right on the ruby text, (b) justified ruby text, (c) ruby text
wider than the ruby base.

Here is what I would do instead. In this function:

(1) Compute the total expansion of the ruby base without changing any widths.
You already do this.
(2) Set the OVERRIDE WIDTH of the ruby run to its old width plus the
totalExpansion. This will cause it to lay out and size using this modified
width.
(3) Relayout the ruby run. It will justify the text properly because it will
use the override width and increase its size. The line layout will then re-run
and use the expanded space properly. The ruby text will be repositioned by ruby
layout and just do the right thing with the expanded width.