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

RESOLVED FIXED in Firefox 41

Status

()

Core
Layout
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

(Blocks: 1 bug)

Trunk
mozilla41
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8609823 [details] [diff] [review]
Handle direction:rtl in vertical modes when converting a LogicalMargin to physical
Attachment #8609823 - Flags: review?(smontagu)
(Assignee)

Updated

3 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Blocks: 1079151
(Assignee)

Comment 2

3 years ago
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.
(Assignee)

Comment 3

3 years ago
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 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
Attachment #8609823 - Flags: review?(smontagu) → review+
(Assignee)

Comment 5

3 years ago
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).
Blocks: 1169420
https://hg.mozilla.org/mozilla-central/rev/8b4c06ddd51d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.