1272721 - Flex item margins are mishandled inside `display: -webkit-box`, at dareenzo.github.io

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[pphagula]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160512004026

Steps to reproduce:

I've setup a style-sheet for a flexbox layout with the rules bellow
```
display: flexbox;
display: -ms-flexbox;
display: -moz-box;
display: -o-box;
display: -webkit-box;
```

on firefox developer edition version 48.0a2 (2016-05-12)


Actual results:

Firefox overrode the css rule specific for it and used the rule for webkit. And while using the webkit rule, didn't display the page correctly, thus leading me to consider it is not  interpreting the rule incorrectly.

But I believe it should have not done the override at all

Note that in a webkit browser like google chrome and safari the page display the right way.


Expected results:

Firefox should have used to css rule specific for it and disregarded the rest of them.

Comment 1

[pphagula]

Attachment: firefox-bug.png

This is the screenshot showing the issue

Comment 2

[ovidiu boca[:Ovidiu]]

Hi,
Please attached a test case where you can see the issue. Also it will help us if you can retest this issue with FF Nightly 49.0a1, you can download it from here: https://nightly.mozilla.org/. Thanks

Comment 3

[pphagula]

Hi ovidiu,

I can't make a test case as I'm not a firefox core developer.

I did the test though and the behaviour is still the same.

Cheers.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to pphagula from comment #3)
> I can't make a test case as I'm not a firefox core developer.

(By "test case", he just meant a simple HTML file that we can load in the browser and perform your steps on to see the problem)

(In reply to pphagula from comment #0)
> Firefox overrode the css rule specific for it and used the rule for webkit.

Yes, we honor "display: -webkit-box" starting in newer versions of Firefox (including DevEdition 48, though it probably won't ship to release until official Firefox 49).

See http://www.otsukare.info/2016/01/04/webkit-resolved-fixed for more, including the motivation. Unfortunately, there are tons of sites out there (particularly mobile sites) that rely on "display: -webkit-box" to render correctly (and don't provide any sort of useful fallback, or provide broken untested fallback), so we've been forced to implement "display: -webkit-box" in order to be able to render such sites.

> And while using the webkit rule, didn't display the page correctly, thus
> leading me to consider it is not  interpreting the rule incorrectly.

This is the interesting part!! I'd love to see how we're not displaying the page correctly.  Could you post a link to a version of your page which Firefox DevEdition is misrendering? I'd love to take a look and see what we're doing differently from Safari/Chrome.  In theory, our -webkit-box implementation should behave pretty much like theirs.

Comment 5

[Daniel Holbert [:dholbert]]

(I guess that "webkit-resolved-fixed" blog post doesn't specifically mention "-webkit-box", but its side-by-side screenshot is from an example site that depends on "-webkit-box" for rendering.)

A few other notes for improving your CSS, while I'm sidetracking:

(1) Your CSS from comment 0 includes "display: flexbox", but that is **not actually a thing** that any browser supports. That *was* the planned keyword for modern CSS flexbox, but in 2012 it was shortened to "display: flex".  (The prefixed implementation in IE does use "-ms-flexbox" because it was written before 2012 and they had content that depended on that spelling.  But the final unprefixed implementations in all browsers use "display: flex", not "display: flexbox".)

(2) Best practice for CSS is to put the modern unprefixed style at the *end*, not at the *beginning*. Later CSS stomps on earlier CSS, when they conflict, and you want the modern latest-and-greatest to stomp on the old-and-crufty.

So: you probably want to have "display:flex" at the *end* of your list in comment 0, instead of having "display:flexbox" at the *beginning*.

Anyway, this is all tangential to the issue you were reporting here -- in theory we'd still like to be able to render your content just like Chrome does, even without having these ^ nits addressed. So, I'm still very interested to see an example page that demonstrates the rendering problems that you mentioned in comment 0.

Comment 6

[Daniel Holbert [:dholbert]]

[/me updating summary to reflect the real bug here -- the fact that rendering reportedly breaks somehow.  (previously the summary indicated that honoring "display: -webkit-box" was a bug, but that is us behaving as-intended.)]

Comment 7

[pphagula]

Attachment: screenshot of chrome vs firefox rendering

Screenshot showing difference between firefox and google chrome rendering of display: -webkit-box

Comment 8

[pphagula]

Hi Daniel,

First of all, thanks for the tips.

A URL you can use to see the issue is my personal blog: https://dareenzo.github.io. The front page should be enough.

You may also look at the attached screenshot above to see the rendering difference between Google Chrome and Firefox of the referred URL. Note that I didn't change the markup as per your recommendations on [comment 6] yet.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (not very reduced)

Thanks! I can reproduce the bug at that site. (similar rendering to your screenshot)

I'm attaching a partially-reduced copy of the HTML/CSS as a testcase here, so that we can still trigger the bug with this testcase & investigate even if you change the CSS to work around this on your side.

Comment 10

[Daniel Holbert [:dholbert]]

--> setting needinfo=me to circle back to this tomorrow, to reduce the testcase further & better-diagnose the issue

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: testcase 3

Looks like the margin on the flex item is not being applied, as shown by this testcase.

(We actually place the margin on the wrong side, due to some flipped logic in one of my patches on bug 1257688.)

Patch coming later today.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: MozReview Request: Bug 1272721: Fix rtl-checking logic in legacy -webkit-box codepath, to actually reverse the correct axis under correct conditions. r=mats

Review commit: https://reviewboard.mozilla.org/r/54958/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54958/

Comment 14

[Daniel Holbert [:dholbert]]

I didn't catch this through other testing because the member-variable in question here ("mMainAxis") is pretty much only used for applying to flex items, at this point.  There are other member-variables that control the direction of flex item flow (with respect to the writing mode), and those are set properly.  

Only this one member-variable (which I need to remove in a refactoring anyway) was getting set incorrectly, due to a flipped "if" check.

The patch includes a reftest which fails in current Nightly due to this bug, but passes with the fix.

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 8756117 [details]
MozReview Request: Bug 1272721: Fix rtl-checking logic in legacy -webkit-box codepath, to actually reverse the correct axis under correct conditions. r=mats

https://reviewboard.mozilla.org/r/54958/#review51640

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> I didn't catch this through other testing because the member-variable in
> question here ("mMainAxis") is pretty much only used for applying to flex
> items, at this point.

(Sorry, I left out a key word there -- meant to say "...applying *margin* to flex items". :))

Thanks for the review!

Comment 17

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d26bc1ac4bc

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> The patch includes a reftest which fails in current Nightly due to this bug,
> but passes with the fix.

Sorry, the reftest didn't actually pass (as shown in my Try run).  I'd briefly compared it visually and I'd run it locally, but I must have missed the reftest-failure output!

Anyway - the included reftest's Try failure reveals that my logic here was still broken in cases where the flex container is "column-oriented".  

My previous patch made "direction:rtl" reverse the main axis, but that was wrong. Really, "direction:rtl" reverses *the container's inline axis*, which is the main axis if we're row-oriented, or the cross-axis if we're column-oriented.

So, I'm posting an updated patch with that slightly-more-subtle logic. This time I definitely ran the reftest locally, and it does pass. :)

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8756117 [details]
MozReview Request: Bug 1272721: Fix rtl-checking logic in legacy -webkit-box codepath, to actually reverse the correct axis under correct conditions. r=mats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54958/diff/1-2/

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8756117 [details]
MozReview Request: Bug 1272721: Fix rtl-checking logic in legacy -webkit-box codepath, to actually reverse the correct axis under correct conditions. r=mats

(Resetting review state of the bugzilla attachment, which I'm told is the current way to re-request review in MozReview.)

You only need to bother looking at the interdiff, which is https://reviewboard.mozilla.org/r/54958/diff/1-2

Comment 21

[Daniel Holbert [:dholbert]]

(Note that MozReview's UI still shows the patch as having r+, but smacleod in #mozreview tells me you'll still be able to indicate an r+ for the updated patch just as you otherwise would, via the "finish review" tab; and that will update the bugzilla state. [He also says there are improvements coming soon to make this clearer/simpler.])

Comment 22

[Daniel Holbert [:dholbert]]

(alternately, feel free to just leave a "r+" comment here, if mozreview doesn't cooperate. :))

Comment 23

[Mats Palmgren (inactive)]

Comment on attachment 8756117 [details]
MozReview Request: Bug 1272721: Fix rtl-checking logic in legacy -webkit-box codepath, to actually reverse the correct axis under correct conditions. r=mats

https://reviewboard.mozilla.org/r/54958/#review51892

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99f8a5fcc34d

Comment 25

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> (We actually place the margin on the wrong side, due to some flipped logic
> in one of my patches on bug 1257688.)

(Wrong bug number there -- Bug 1262049 "part 5" is where this regressed, actually. That's what added the code that I'm fixing up here.)

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 8756117 [details]
MozReview Request: Bug 1272721: Fix rtl-checking logic in legacy -webkit-box codepath, to actually reverse the correct axis under correct conditions. r=mats

Requesting approval to uplift to Aurora 48.

Approval Request Comment
[Feature/regressing bug #]: bug 1262049

[User impact if declined]:
 * For DevEdition 48 users (like this bug's reporter), this patch averts incorrect-layout (& content overlapping as a result) on sites that use margins inside of a "display:-webkit-box" container.  See attached screenshot.

 * For Beta 48 users early on in the release cycle, this will similarly avert incorrect-layout & content overlapping.

 * For Beta 48 users later in the release cycle, and for Firefox 48 Release users, there's zero impact, since this feature will be disabled for them. (See bug 1259345 comment 16.)

[Describe test coverage new/current, TreeHerder]: Reftest included; passes on Try. We have other reftest coverage for -webkit-box rendering, too. (and for modern flexbox, which it relies on)

[Risks and why]: Very low risk. This fixes some very-broken logic to be strictly better.  At worst, this could cause different layout breakage (but I don't think it will.  Moreover, for Firefox 48 Release users, this won't have any effect at all because this feature will be disabled for them (see bug 1259345 comment 16), so it's zero-risk from their perspective.

[String/UUID change made/needed]: None.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/99f8a5fcc34d

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8756117 [details]
MozReview Request: Bug 1272721: Fix rtl-checking logic in legacy -webkit-box codepath, to actually reverse the correct axis under correct conditions. r=mats

Polish a new feature, taking it.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7aa34c1c5398