Closed Bug 1135361 Opened 5 years ago Closed 5 years ago

CSS Ruby jumps under certain vertical text situation

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: xmomdo, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

(Keywords: css3)

Attachments

(3 files, 2 obsolete files)

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.
Component: Untriaged → Layout
Keywords: css3
OS: Windows 8.1 → All
Product: Firefox → Core
Hardware: x86_64 → All
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → quanxunzhen
FWIW, Chrome seems to just ignore the "text-align: justify" in the testcase (but in vertical text without ruby they do justify)
Other UAs simply treat ruby like a inline-block, which is not what we want.
BTW, it seems to me that Chrome doesn't justify CJK characters at all on Windows, either.
Attached patch patch (obsolete) — Splinter Review
Attachment #8567622 - Flags: review?(jfkthame)
Attachment #8567622 - Attachment is obsolete: true
Attachment #8567622 - Flags: review?(jfkthame)
Attached patch patchSplinter Review
Attachment #8567623 - Flags: review?(jfkthame)
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...
Blocks: css-ruby
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.
Attachment #8567623 - Flags: review?(jfkthame) → review+
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.
Attachment #8567883 - Flags: review?(quanxunzhen)
Attachment #8567875 - Attachment is obsolete: true
Attachment #8567875 - Flags: review?(quanxunzhen)
This reftest seems to uncover some problem of ruby in B2G, let me check that first.
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?
Attachment #8567883 - Flags: review?(quanxunzhen) → review+
(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.
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/e54975f3d51b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.