Closed Bug 1115262 Opened 5 years ago Closed 5 years ago

Ruby text is not positioned properly with vertical-rl writing mode

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

No description provided.
Attached patch patch (obsolete) — Splinter Review
Attachment #8541116 - Flags: review?(jfkthame)
Comment on attachment 8541116 [details] [diff] [review]
patch

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

This doesn't seem to apply to my current tree -- it must depend on another pending patch to nsRubyTextContainer to introduce/maintain the mLineSize member -- so I haven't actually tested it, but it looks like a reasonable thing to do. r+ if it makes things work. :)

::: layout/generic/nsRubyBaseContainerFrame.cpp
@@ +326,5 @@
>  
>      LogicalMargin borderPadding = reflowState->ComputedLogicalBorderPadding();
> +    // If the writing mode is vertical-rl, the horizontal position of
> +    // rt frames will be updated when reflowing this text container.
> +    nscoord containerWidth = borderPadding.LeftRight(lineWM);

Given that the position will be set later, I think you could simply use a containerWidth of zero during the reflow here....

::: layout/generic/nsRubyTextContainerFrame.cpp
@@ +79,5 @@
>    aDesiredSize.SetSize(lineWM, mLineSize);
> +
> +  if (lineWM.IsVerticalRL()) {
> +    LogicalMargin borderPadding = aReflowState.ComputedLogicalBorderPadding();
> +    nscoord oldWidth = borderPadding.LeftRight(lineWM);

....and then oldWidth here would also be zero, so you can just eliminate it.
Attachment #8541116 - Flags: review?(jfkthame) → review+
(In reply to Jonathan Kew (:jfkthame) from comment #2)
> Comment on attachment 8541116 [details] [diff] [review]
> patch
> 
> Review of attachment 8541116 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This doesn't seem to apply to my current tree -- it must depend on another
> pending patch to nsRubyTextContainer to introduce/maintain the mLineSize
> member -- so I haven't actually tested it, but it looks like a reasonable
> thing to do. r+ if it makes things work. :)

This part of code, hmmm, is touched by both bug 1055665 and bug 1108429. They both have been pushed to m-i but haven't been merged to m-c. So, hopefully, you can apply this patch on top of the m-c tomorrow :)

> ::: layout/generic/nsRubyBaseContainerFrame.cpp
> @@ +326,5 @@
> >  
> >      LogicalMargin borderPadding = reflowState->ComputedLogicalBorderPadding();
> > +    // If the writing mode is vertical-rl, the horizontal position of
> > +    // rt frames will be updated when reflowing this text container.
> > +    nscoord containerWidth = borderPadding.LeftRight(lineWM);
> 
> Given that the position will be set later, I think you could simply use a
> containerWidth of zero during the reflow here....
> 
> ::: layout/generic/nsRubyTextContainerFrame.cpp
> @@ +79,5 @@
> >    aDesiredSize.SetSize(lineWM, mLineSize);
> > +
> > +  if (lineWM.IsVerticalRL()) {
> > +    LogicalMargin borderPadding = aReflowState.ComputedLogicalBorderPadding();
> > +    nscoord oldWidth = borderPadding.LeftRight(lineWM);
> 
> ....and then oldWidth here would also be zero, so you can just eliminate it.

I agree. Especially given that the border and padding code might be broken in other places for ruby boxes...
Hmm, in my testing this doesn't look quite right: it places the ruby glyphs immediately *outside* (to the right of) the <rtc> that should contain them, as shown by its border. (This is when testing on OS X, where bug 1107960 does not appear to be an issue; on some platforms, that could be confusing things further.)

Resetting to r? for now, to remind me to look into it again.
Attachment #8541116 - Flags: review+ → review?(jfkthame)
Summary: Ruby test is not positioned properly with vertical-rl writing mode → Ruby text is not positioned properly with vertical-rl writing mode
(In reply to Jonathan Kew (:jfkthame) from comment #4)
> Hmm, in my testing this doesn't look quite right: it places the ruby glyphs
> immediately *outside* (to the right of) the <rtc> that should contain them,
> as shown by its border. (This is when testing on OS X, where bug 1107960
> does not appear to be an issue; on some platforms, that could be confusing
> things further.)

Hmm... it is possible that it is positioned correctly on Windows is the side effect of bug 1107960 :)
Attached patch patch (obsolete) — Splinter Review
Attachment #8541116 - Attachment is obsolete: true
Attachment #8541116 - Flags: review?(jfkthame)
Attachment #8544218 - Flags: review?(jfkthame)
I didn't see any problem in either Windows or Linux. Maybe some bug has been fixed since comment 4?
Blocks: enable-css-ruby
No longer blocks: css-ruby
I'm looking at http://people.mozilla.org/~jkew/ruby-demo.html with this patch, and still see problems with the positioning in the vertical-rl case.

Note that I've just pushed a number of patches to inbound that should improve the handling of font metrics in vertical mode. Let's re-test this once those are safely merged to central, and see how things look across various platforms.
Attached patch patch (obsolete) — Splinter Review
Fix one more place, then it works correctly with your testcase on Mac :)

But it still doesn't work quite good on Windows. :(
I guess there are still some problems on Windows on vertical text, especially something related to font metrics.
Attachment #8544218 - Attachment is obsolete: true
Attachment #8544218 - Flags: review?(jfkthame)
Attachment #8546400 - Flags: review?(jfkthame)
Comment on attachment 8546400 [details] [diff] [review]
patch

This doesn't seem to be correct...
Attachment #8546400 - Flags: review?(jfkthame)
Attached patch patchSplinter Review
It should work fine now, even on Windows :)
Attachment #8546400 - Attachment is obsolete: true
Attachment #8546428 - Flags: review?(jfkthame)
Comment on attachment 8546428 [details] [diff] [review]
patch

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

Yes, this appears to fix the vertical-rl problems. Some remaining issues I'm seeing are not vertical-rl-specific, but relate more generally to borders on ruby elements, which I guess will be covered in bug 1055667.

(Testcase at http://people.mozilla.org/~jkew/ruby-demo.html has been updated to allow experimentation with borders; provided they're set to zero, the ruby text is positioned as expected in all writing modes.)
Attachment #8546428 - Flags: review?(jfkthame) → review+
https://hg.mozilla.org/mozilla-central/rev/a638073e104d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.