1167930 - LogicalMargin::GetPhysicalMargin doesn't handle dir=rtl in vertical writing modes correctly

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jonathan Kew [:jfkthame]]

In the course of debugging some issues with position:absolute, I noticed that LogicalMargin::GetPhysicalMargin doesn't handle top/bottom margins correctly when bidi-rtl occurs in vertical writing mode.

This accounts for a couple of the current float-in-rtl reftest failures; see https://treeherder.mozilla.org/#/jobs?repo=try&revision=8beff9ab97bd.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Handle direction:rtl in vertical modes when converting a LogicalMargin to physical

Comment 2

[Jonathan Kew [:jfkthame]]

Turns out the patch here has an unfortunate side-effect on an example like

  data:text/html,<div style="writing-mode:vertical-lr;width:50px;height:50px;
                  border:1px solid blue;direction:rtl">
                  <div style="width:10px;height:10px;background:red">

where the small red square should appear at the bottom left of the blue square, but this patch moves it to the top left.

(I'm a bit surprised we don't already have a reftest that detects this, but the tests in bug 1147834 will run into it.)

I still think the fix to LogicalMargin::GetPhysicalMargin is correct, but there must be code elsewhere that is dependent on the current (incorrect) GetPhysicalMargin behavior.

Comment 3

[Jonathan Kew [:jfkthame]]

The problem here seems to be an aspect of bug 1131451. In nsBlockReflowContext, we don't take container height into account, and this means that we fail to handle dir=rtl correctly in vertical modes. In the example from comment 2, we're calculating the correct computed margins for the red square, but failing to take account of container height for vertical bidi means that we place it incorrectly.

So my current understanding is that this patch is OK, and the resulting test failures in bug 1147834 should be ascribed to bug 1131451 and simply marked as known-fail until we address that.

Comment 4

[Simon Montagu :smontagu]

Comment on attachment 8609823 [details] [diff] [review]
Handle direction:rtl in vertical modes when converting a LogicalMargin to physical

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

The patch looks correct, and I agree with your conclusions in comment 3

Comment 5

[Jonathan Kew [:jfkthame]]

Just to follow up a little more: Because of bug 1131451, I can reproduce errors involving top/bottom margins, vertical writing mode, and rtl direction either with or without this patch. The precise nature of the errors changes when we fix GetPhysicalMargin here, but the vertical/rtl combination is still fundamentally broken.

Example:

  data:text/html,<div style="writing-mode:vertical-lr;width:50px;height:50px;
                  border:1px solid blue;direction:rtl">
                  <div style="width:10px;height:10px;background:red;
                   margin-top:10px;margin-bottom:auto">

On current trunk, the red square is at top left (the margin-top gets lost). With this patch, it appears 10px up from the bottom left (the margin-top appears to have been mapped to the bottom, though what has really happened is that its inline-start margin, computed to 30px, has ended up above the red square instead of below it due to the failure to account for bidi and container-height when placing the block).

Comment 6

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b4c06ddd51d

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b4c06ddd51d