1044198 - "margin-left" renders on the wrong side, on an element with "direction:ltr" inside of a "direction:rtl" container

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[skeate]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140716183446

Steps to reproduce:

CSS Transition on margin-left with direction: ltr and a parent element direction: rtl

See this JSFiddle: http://jsfiddle.net/FdC45/4/


Actual results:

clicking "panel 0" causes the box to disappear, then two to slide in from the left.


Expected results:

one box should slide in from behind panel 0

remove direction: ltr; from .panel to see expected behavior

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

I don't think transitions are part of the problem here.

The problem (the difference in Chrome's rendering vs. Firefox's rendering at least) boils down to what happens when you insert <div class="panel">.

In Firefox, the existing panel gets pushed offscreen. (to the left)
In Chrome, it does not. I'm not yet sure which is correct -- need to poke at the testcase a bit more first.

Here's a reduced testcase, after the second panel is inserted, with a less-severe "margin-left" value. This demonstrates the difference between Firefox Nightly (which renders the panel partly-hidden) vs. Chrome (which does not).

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (only tangentially related; uses "unicode-bidi" and "display:inline")

Chrome renders the first two lines of this testcase as follows:
 div:
   FIRST SECOND
 span:
   FIRST SECOND



Firefox renders the first two lines like:
 div:
   SECOND FIRST
 span:
   FIRST SECOND

The reason we differ on <div> vs. <span> seems to be because our UA stylesheet for html.css, which has a rule for 'div' that has "unicode-bidi: -moz-isolate".  If I add that styling to the span in my testcase, then it matches the div rendering (showing SECOND on the left), as showed in the third "bonus" line.  This styling (with -webkit prefix) makes Chrome shift SECOND to the left as well, interestingly.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 3

(Sorry, testcase 2 isn't as relevant to the original jsfiddle as I thought it was; the jsfiddle uses 'inline-block', and unicode-bidi doesn't seem to have the same effect there as it does on 'inline'. So tweaks to that property don't affect the rendering of the original example.)

I think this new testcase (#3) gets to the root of the real difference here.  When an element in an RTL flow has a "margin-left" value *and* has "direction: ltr", we seem to place the "margin-left" on the wrong side.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (updated, now w/ extraneous div removed from first line

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 4 (further reduced & using positive margin)

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

one-day mozilla-central regression range is:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=44ae8462d6ab&tochange=46041cc216fd

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Presumably a regression from
https://hg.mozilla.org/mozilla-central/rev/dab8e3865967

Comment 8

[Daniel Holbert [:dholbert]]

Yup, I got the same regression-range, and mozregression narrowed it to that regression using inbound builds:
Last good revision: f05bca38aa4d
First bad revision: dab8e3865967
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f05bca38aa4d&tochange=dab8e3865967

So, definitely regressed by bug 789096, back in March.

(Incidentally, looks like some more patches appear to have landed on that bug yesterday; I wonder if they impacted this at all. Either way, this would probably benefit from a look-see by smontagu, unless dbaron or someone else is already on top of debugging this.)

Comment 9

[Daniel Holbert [:dholbert]]

(er, "narrowed it to that revision")

Comment 10

[Simon Montagu :smontagu]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> The reason we differ on <div> vs. <span> seems to be because our UA
> stylesheet for html.css, which has a rule for 'div' that has "unicode-bidi:
> -moz-isolate".

I'm a little surprised that Chrome still doesn't do that. Our behaviour is certainly correct by spec: see 

https://www.w3.org/Bugs/Public/show_bug.cgi?id=10815
https://bugzilla.mozilla.org/show_bug.cgi?id=676245

and https://bugs.webkit.org/show_bug.cgi?id=65617

Comment 11

[Simon Montagu :smontagu]

Attachment: Testcase 4 as reftest

Do you think this is sufficient as a reftest, or are the cases in testcase 3 different enough that they need testing as well?

Comment 12

[Simon Montagu :smontagu]

Attachment: Patch

Because we are walking through child frames in the containing frame's writing direction, we need to convert the margin's to the container's writing mode. I got this right in nsLineLayout::ApplyStartMargin https://bugzilla.mozilla.org/attachment.cgi?id=8389172&action=diff#a/layout/generic/nsLineLayout.cpp_sec17 but wrong here.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=216523b9e6e6

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8463056 [details] [diff] [review]
Testcase 4 as reftest

(In reply to Simon Montagu :smontagu from comment #11)
> Do you think this is sufficient as a reftest, or are the cases in testcase 3
> different enough that they need testing as well?

The negative-margin situation from testcase 3 ("margin-left: -30px *and direction:ltr* on the blue element") might be worth including in the reftest (e.g. as a second line, after a <br><br>) since that's the scenario that the reporter was actually exercising, and it's a scenario where we were pushing content offscreen.

r=me either way; this is probably sufficient on its own, too.

Comment 14

[Daniel Holbert [:dholbert]]

RE unicode-bidi:
(In reply to Simon Montagu :smontagu from comment #10)
> I'm a little surprised that Chrome still doesn't do that. Our behaviour is
> certainly correct by spec: see 
[...]
> and https://bugs.webkit.org/show_bug.cgi?id=65617

Looks like https://code.google.com/p/chromium/issues/detail?id=296863 is the blink bug (migrated from the webkit bug).

(Anyway -- fortunately, that behavior wasn't actually implicated here, in the original testcase.)

Comment 15

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0fe08bd921f
https://hg.mozilla.org/integration/mozilla-inbound/rev/fedb725da886

Comment 16

[Daniel Holbert [:dholbert]]

We're relatively early in the release cycle. Given that this is a recent regression, it's probably worth getting this backported to Aurora33, I imagine?  Maybe even Beta32, depending on how comfortable we are with the safety/correctness here. Then, current release (31) would be the only affected version.

Comment 17

[Simon Montagu :smontagu]

Yes, I'll ask aurora approval after the patch has baked on central for a few days. Do we know of any real-world sites affected by this?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b0fe08bd921f
https://hg.mozilla.org/mozilla-central/rev/fedb725da886

Comment 19

[Simon Montagu :smontagu]

Comment on attachment 8463062 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: bug 789096
[User impact if declined]: could be misplaced inlines making sites messy and hard to read (but there are no reports of instances in the wild)
[Describe test coverage new/current, TBPL]: reftests attached to bug, checked in on mozilla-central
[Risks and why]: minimal risk 
[String/UUID change made/needed]: N/A

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8463056 [details] [diff] [review]
Testcase 4 as reftest

[Triage Comment]
I think that having the test in aurora is a good thing.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5ce3895d8dd8