Closed Bug 1131013 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: jfkthame, Assigned: smontagu)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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 א
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!)
Attached patch Quick fixSplinter Review
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).
Assignee: nobody → smontagu
Attachment #8561953 - Flags: review?(jfkthame)
Attached patch ReftestSplinter Review
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).
Attachment #8561956 - Flags: review?(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).
Attachment #8561953 - Flags: review?(jfkthame) → review+
Attachment #8561956 - Flags: review?(jfkthame) → review+
Blocks: 1127488
https://hg.mozilla.org/mozilla-central/rev/efec1f21b4a1
https://hg.mozilla.org/mozilla-central/rev/31f86276a98b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Attachment #8565696 - Flags: review?(smontagu)
Blocks: 1134598
Attachment #8565696 - Flags: review?(smontagu) → review+
You need to log in before you can comment on or make changes to this bug.