Bug 137421 - Inline ruby does not get justified correctly
Summary: Inline ruby does not get justified correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks: 144044
  Show dependency treegraph
 
Reported: 2014-10-04 01:01 PDT by Myles C. Maxfield
Modified: 2015-04-22 09:07 PDT (History)
14 users (show)

See Also:


Attachments
Patch (15.00 KB, patch)
2014-10-04 01:15 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (544.25 KB, application/zip)
2014-10-04 02:48 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (578.53 KB, application/zip)
2014-10-04 03:09 PDT, Build Bot
no flags Details
Patch (24.67 KB, patch)
2014-10-04 08:24 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (26.02 KB, patch)
2014-10-08 16:56 PDT, Myles C. Maxfield
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2014-10-04 01:01:38 PDT
Inline ruby does not get justified correctly
Comment 1 Myles C. Maxfield 2014-10-04 01:15:01 PDT
Created attachment 239272 [details]
Patch
Comment 2 Build Bot 2014-10-04 02:48:21 PDT
Comment on attachment 239272 [details]
Patch

Attachment 239272 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5689605747638272

New failing tests:
fast/ruby/bopomofo-rl.html
Comment 3 Build Bot 2014-10-04 02:48:25 PDT
Created attachment 239274 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 4 Build Bot 2014-10-04 03:09:31 PDT
Comment on attachment 239272 [details]
Patch

Attachment 239272 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5105455835643904

New failing tests:
fast/ruby/bopomofo-rl.html
Comment 5 Build Bot 2014-10-04 03:09:39 PDT
Created attachment 239275 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 6 Myles C. Maxfield 2014-10-04 08:24:41 PDT
Created attachment 239280 [details]
Patch
Comment 7 Dave Hyatt 2014-10-07 14:39:05 PDT
Comment on attachment 239280 [details]
Patch

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.
Comment 8 Myles C. Maxfield 2014-10-08 16:56:49 PDT
Created attachment 239503 [details]
Patch
Comment 9 Dave Hyatt 2014-10-08 17:54:13 PDT
Comment on attachment 239503 [details]
Patch

r=me
Comment 10 Myles C. Maxfield 2014-10-08 18:15:22 PDT
http://trac.webkit.org/changeset/174489
Comment 11 Myles C. Maxfield 2014-10-08 18:17:25 PDT
<rdar://problem/17352909>