1131013 - bidi reordering for dir=rtl does not work as expected in vertical writing modes

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Jonathan Kew [:jfkthame]]

AFAICT, we're failing to reorder inline frames into the proper visual order when dir=rtl and vertical writing mode is in effect.

Testcase:

  data:text/html;charset=utf-8,<div style="writing-mode:vertical-rl;
    height:150px;width:100px;background:yellow;" dir=rtl>א foo ב <b>bar ג</b>

which should render the same as the dir=ltr example:

  data:text/html;charset=utf-8,<div style="writing-mode:vertical-rl;
    height:150px;width:100px;text-align:end;background:yellow;"><b>ג bar</b> ב foo א

Comment 1

[Jonathan Kew [:jfkthame]]

Inline bidi within vertical writing mode is not one of the primary use cases for vertical text; it's a relatively esoteric combination.

We suspect the work needed to fix this is closely related to the work for text-orientation:sideways-left, as they both involve inline flow that runs vertically upwards, against the (physical) vertical coordinate system we use. As such, I think we should not treat this as blocking the initial release of vertical writing mode support (where we're not supporting sideways-left, either).

(A quick fix would still be great, though!)

Comment 2

[Simon Montagu :smontagu]

Attachment: Quick fix

So by fixing up the inline coordinate calculation just in nsBidiPresUtils::RepositionFrame, we can fix both this and bug 1127488. I think that's a sufficient win to be worth the slight hackiness involved. (I filed bug 1131451 on a comprehensive fix for vertical writing modes with dir=rtl).

Comment 3

[Simon Montagu :smontagu]

Attachment: Reftest

The test from comment 0 as a reftest (with added border to catch a bug that I perpetrated in the first version of the patch).

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 8561953 [details] [diff] [review]
Quick fix

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

s/text frames/inline frames/ in the commit message, as this doesn't only apply to text.

Looks OK for now, until we do a more complete job. One suggestion is to use nsSize fields for container size in nsBlockReflowState and nsLineLayout, rather than separate width and height fields; I think this'll make things just a little tidier.

::: layout/base/nsBidiPresUtils.cpp
@@ +1479,5 @@
>    }
>  
> +  // LogicalRect doesn't correctly calculate the vertical position
> +  // in vertical writing modes with right-to-left direction (Bug 1131451).
> +  // This does the correct calculation ad hoc pending the fix for that.;

No need to terminate the comment with a semicolon. ;-)

@@ +1481,5 @@
> +  // LogicalRect doesn't correctly calculate the vertical position
> +  // in vertical writing modes with right-to-left direction (Bug 1131451).
> +  // This does the correct calculation ad hoc pending the fix for that.;
> +  nsRect rect = aFrame->GetRect();
> +  nscoord lineSize = aContainerWM.IsVertical()

Might lineLength be better than lineSize, given that we normally use Size for types/variables that hold measurements in both dimensions? No big deal, though; I'm sure we do things like this elsewhere too.

::: layout/generic/nsBlockReflowState.h
@@ +207,5 @@
>    }
>  
>    // Physical width. Use only for physical <-> logical coordinate conversion.
>    nscoord mContainerWidth;
> +  nscoord mContainerHeight;

Make this "nsSize mContainerSize;" (and update the comment).

::: layout/generic/nsLineLayout.h
@@ +56,5 @@
>                         bool aImpactedByFloats,
>                         bool aIsTopOfPage,
>                         mozilla::WritingMode aWritingMode,
> +                       nscoord aContainerWidth,
> +                       nscoord aContainerHeight);

Make these a single const nsSize& parameter, and update callers accordingly.

@@ +585,5 @@
>    nscoord mTrimmableISize;
>  
>    // Physical width. Use only for physical <-> logical coordinate conversion.
>    nscoord mContainerWidth;
> +  nscoord mContainerHeight;

Make this "nsSize mContainerSize;" (and update the comment).

Comment 5

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/efec1f21b4a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/31f86276a98b

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/efec1f21b4a1
https://hg.mozilla.org/mozilla-central/rev/31f86276a98b

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: followup - make the vertical-bidi reftest more robust across platform/font differences

It turns out the reftest here is rather fragile, depending on the line-height metrics (ascent/descent) of the fonts that end up getting used. E.g. see the Linux failure in https://treeherder.mozilla.org/#/jobs?repo=try&revision=022592cb986f. Here's a patch to make it less fragile; with this it seems to pass across all platforms. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5cbdb06ab2b.

Comment 8

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/225deeffbc54

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/225deeffbc54