1135361 - CSS Ruby jumps under certain vertical text situation

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Momdo Nakamura]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150122214805

Steps to reproduce:

Ruby texts jumps into previous line under following situation:

1) Set vertical text mode
2) Set box height (optionally)
3) Apply text-align:justify
4) Add ruby annotation

i.e., following HTML fragments for example:

<p style="writing-mode:vertical-rl; height:200px; text-align:justify;">
<ruby>祇園精舎<rt>ぎおんしょうじゃ</rt></ruby>の鐘の声、
<ruby>諸行無常<rt>しょぎょうむじょう</rt></ruby>の響きあり。
<ruby>沙羅双樹<rt>さらそうじゅ</rt></ruby>の花の色、
<ruby>盛者必衰<rt>じょうしゃひっすい</rt></ruby>の理をあらはす。<!-- correct rendering -->
</p>


Actual results:

Ruby texts overlap preceding line, except for the last ruby texts.


Expected results:

All ruby texts are placed nearby ruby bases.

Comment 1

[Simon Montagu :smontagu]

Attachment: Reporter's testcase

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

The problem seems to from interaction between justification, vertical text, and ruby.

In the testcase, if we add "-moz-text-align-last: justify" to the stylesheet, the last line won't have correct rendering either.

Comment 3

[Simon Montagu :smontagu]

FWIW, Chrome seems to just ignore the "text-align: justify" in the testcase (but in vertical text without ruby they do justify)

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Other UAs simply treat ruby like a inline-block, which is not what we want.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

BTW, it seems to me that Chrome doesn't justify CJK characters at all on Windows, either.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8567623 [details] [diff] [review]
patch

Review of attachment 8567623 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsLineLayout.cpp
@@ +1124,5 @@
>            mLineAtStart = false;
>          }
>          if (nsGkAtoms::rubyFrame == frameType) {
>            mHasRuby = true;
> +          SyncAnnotationBounds(pfd);

This call is moved here because the metrics of the ruby frame have not been available in the previous location.

@@ +2806,2 @@
>    while (pfd) {
> +    nscoord containerWidth = pfd->mFrame->GetParent()->GetRect().Width();

Ruby text frames' container is not the ruby frame, but the ruby text container, which is not identical for each level. But if the frame is ruby text container, it can be optimized to only compute once.

::: layout/generic/nsRubyTextContainerFrame.cpp
@@ +160,5 @@
>          nsIFrame* child = e.get();
>          LogicalPoint pos = child->GetLogicalPosition(lineWM, containerWidth);
>          pos.B(lineWM) += deltaBCoord;
>          // Relative positioning hasn't happened yet.
> +        // So MovePositionBy should not be used here.

A comment fix...

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7150f46ce752

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 8567623 [details] [diff] [review]
patch

Review of attachment 8567623 [details] [diff] [review]:
-----------------------------------------------------------------

This looks OK to me.

I think we should have a reftest that covers this. I've put together a possible test that I'll attach in a moment; also pushed a tryserver run to check whether it passes safely across all platforms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa27c631c2e7.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: Reftest for ruby positioning in justified vertical text

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: Reftest for ruby positioning in justified vertical text

This is the version of the test I really meant to attach; and the correct try run is https://tbpl.mozilla.org/?tree=Try&rev=6c37a39541ca. Sorry about the churn.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

This reftest seems to uncover some problem of ruby in B2G, let me check that first.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8567883 [details] [diff] [review]
Reftest for ruby positioning in justified vertical text

Review of attachment 8567883 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

This test fails in B2G, but it doesn't seem to be caused by how this test is constructed. Hence it should be good to track that failure in a separate bug.

Could you file that bug and then land the test?

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e54975f3d51b

Comment 16

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aff54769dd05

Comment 17

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #14)
> Could you file that bug and then land the test?

Filed bug 1136067 for the B2G issue.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e54975f3d51b

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/aff54769dd05