Closed Bug 1272721 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

48 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: pphagula, Assigned: dholbert)

References

()

Details

(Keywords: regression)

Attachments

(5 files, 1 obsolete file)

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.
OS: Unspecified → Mac OS X
Attached image firefox-bug.png (obsolete) —
This is the screenshot showing the issue
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
Flags: needinfo?(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.
Component: Untriaged → CSS Parsing and Computation
Product: Firefox → Core
Flags: needinfo?(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.
Flags: needinfo?(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.
[/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.)]
Summary: Firefox developer edition is interpreting `display: -webkit-box;` overriding `display: -moz-box;` incorreclty → On some particular site, it breaks Firefox's rendering to honor `display: -webkit-box;`
Screenshot showing difference between firefox and google chrome rendering of display: -webkit-box
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.
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.
Attachment #8752559 - Attachment is obsolete: true
--> setting needinfo=me to circle back to this tomorrow, to reduce the testcase further & better-diagnose the issue
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(pphagula) → needinfo?(dholbert)
Attached file testcase 2
Flags: needinfo?(dholbert)
Attached file 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.
Assignee: nobody → 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 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
Attachment #8756117 - Flags: review?(mats) → review+
(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!
(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 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/
Attachment #8756117 - Attachment description: MozReview Request: Bug 1272721: Fix backwards rtl-checking logic, in legacy (-webkit-box) codepath in nsFlexContainerFrame. r?mats → MozReview Request: Bug 1272721: Fix rtl-checking logic in legacy -webkit-box codepath, to actually reverse the correct axis under correct conditions. r=mats
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
Attachment #8756117 - Flags: review+ → review?(mats)
(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.])
(alternately, feel free to just leave a "r+" comment here, if mozreview doesn't cooperate. :))
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
Attachment #8756117 - Flags: review?(mats) → review+
Summary: On some particular site, it breaks Firefox's rendering to honor `display: -webkit-box;` → Flex item margins are mishandled inside `display: -webkit-box`, at dareenzo.github.io
(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.)
Blocks: 1262049
No longer blocks: 1257688
Keywords: regression
OS: Mac OS X → All
Hardware: Unspecified → All
Attachment #8755730 - Attachment description: chrome-vs-firefox-rendering.png → screenshot of chrome vs firefox rendering
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.
Attachment #8756117 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/99f8a5fcc34d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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.
Attachment #8756117 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.