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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Comment 2

[Jonathan Kew [: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.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

(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...

Comment 4

[Jonathan Kew [:jfkthame]]

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.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

(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 :)

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: ruby with vertical-rl screenshot on Linux

I didn't see any problem in either Windows or Linux. Maybe some bug has been fixed since comment 4?

Comment 8

[Jonathan Kew [:jfkthame]]

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.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

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.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

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

This doesn't seem to be correct...

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

It should work fine now, even on Windows :)

Comment 12

[Jonathan Kew [: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.)

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=83085fbd2d71

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 15

[Carsten Book [:Tomcat]]

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