CSS Ruby jumps under certain vertical text situation

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: xmomdo, Assigned: xidorn)

Tracking

(Blocks 1 bug, {css3})

Trunk
mozilla39
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Component: Untriaged → Layout
Keywords: css3
OS: Windows 8.1 → All
Product: Firefox → Core
Hardware: x86_64 → All
(Assignee)

Comment 2

4 years ago
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)

Updated

4 years ago
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)
(Assignee)

Comment 4

4 years ago
Other UAs simply treat ruby like a inline-block, which is not what we want.
(Assignee)

Comment 5

4 years ago
BTW, it seems to me that Chrome doesn't justify CJK characters at all on Windows, either.
(Assignee)

Comment 6

4 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #8567622 - Flags: review?(jfkthame)
(Assignee)

Updated

4 years ago
Attachment #8567622 - Attachment is obsolete: true
Attachment #8567622 - Flags: review?(jfkthame)
(Assignee)

Comment 7

4 years ago
Posted patch patchSplinter Review
Attachment #8567623 - Flags: review?(jfkthame)
(Assignee)

Comment 8

4 years ago
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)
(Assignee)

Comment 13

4 years ago
This reftest seems to uncover some problem of ruby in B2G, let me check that first.
(Assignee)

Comment 14

4 years ago
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.
(Assignee)

Updated

4 years ago
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/e54975f3d51b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.