Closed Bug 1345873 Opened 7 years ago Closed 7 years ago

flex items aren't sorted according to "order", if they're separated by an abspos sibling

Categories

(Core :: Layout, defect)

52 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox52 --- wontfix
firefox-esr52 53+ fixed
firefox53 + fixed
firefox54 + fixed
firefox55 + fixed

People

(Reporter: johnny.is.first, Assigned: dholbert)

References

Details

(4 keywords, Whiteboard: [fixed in mozilla-central by bug 812687. Fixing on branches via more targeted band-aid])

Attachments

(6 files, 3 obsolete files)

Attached file Flexbox.htm
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36

Steps to reproduce:

Create a flebox layout like 

<div style=display:flex>
  <div style=order:2>A</div>
  <div style=position:absolute></div>
  <div style=order:1>M</div>
</div>


Actual results:

it displays: AM


Expected results:

According W3C (https://www.w3.org/TR/css-flexbox-1/#abspos-items) it should display: MA
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b48820f4c87aeaa2186c401c1bafe6661d6b7e31&tochange=751ba58e708fc28961efc6ff0c2b053f4306b08f

Regressed by Bug 1269045, I guess:
part 1: Adjust flex item "order"-sorting code to treat placeholder frames as <= anything they're compared against, including each other. r=mats
Blocks: 1269045
Component: Untriaged → Layout
Flags: needinfo?(dholbert)
Keywords: regression, testcase
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All
NB: someone needs to find a better title, IMHO. :)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Broken flexbox layout → flex items aren't sorted according to "order", if they're separated by an abspos sibling
(In reply to Loic from comment #1)
> Regressed by Bug 1269045, I guess:
> part 1: Adjust flex item "order"-sorting code to treat placeholder frames as
> <= anything they're compared against, including each other. r=mats

Indeed -- this is an embarrassing typo. This is a bug in the LEQ function changes here:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/86fbc1fb2818#l1.32

That change made IsOrderLEQ return true *if either arg is a placeholder*. That's not quite what I wanted.  (So in this bug's testcase, IsOrderLEQ doesn't reshuffle anything -- it gets called for the first flex-item & the placeholder, and it returns true; and then it gets called for the placeholder and the second flex-item, and it returns true again. And so it doesn't see that there's any reordering needed.)

My intent was to make IsOrderLEQ return val always indicate *that placeholders were smaller*. So the logic needs to be a bit more subtle -- something like:
 if first frame is placeholder: return true
 if second frame is placeholder: return false
...I think.
[Tracking Requested - why for this release]: Causing potential site layout breakage.
I'm going to be adding a new "order" mochitest based on the existing ones here, and I noticed some needed-cleanup while writing the new test -- so I'm spinning off the old-test-cleanup into a preliminary test-only helper patch, and then the main patch will just copy-with-modifications.

Not bothering to request review on this test-tweak since it's test-only and minor. But feedback is also welcome.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Note:
 - the code-comment changes (in nsFlexContainer.cpp) are just deindenting the existing comment.
 - the code changes are just implementing the pseudocode at the end of comment 3.
(In reply to Kohei Yoshino [:kohei] from comment #4)
> [Tracking Requested - why for this release]: Causing potential site layout breakage.
> tracking-firefox52: --- → ?

(If we ship a 52.0.* dot-release, I'd be very much interested in getting this fix included in that dot-release.)
Comment on attachment 8846122 [details] [diff] [review]
part 1: Make flex-item sorting code *actually* sort placeholders to the start of the child list, as intended

r=mats (sorry for missing this in my original review)

A couple of suggestions for enhancing the tests (feel free to ignore):
> +  // (Note: "order" doesn't influence position or paint order for abspos
> +  // children - only for (in-flow) flex items.)

It would be nice if the test verified that.  Perhaps you could set
a random value between 1..3 on the abs.pos. elements or something?

Both test_flexbox_order_abspos.html and test_flexbox_order.html says:
> * Note: The items in this testcase don't overlap, so this testcase does _not_
> * test paint ordering.  It only tests horizontal ordering in a flex container.

It seems it would be fairly easy to enhance these tests to do that.
Just add a child in each of the current children and make it slightly overflow
and set background:inherit on it?
Attachment #8846122 - Flags: review?(mats) → review+
(In reply to Mats Palmgren (:mats) from comment #9)
> r=mats (sorry for missing this in my original review)

No worries; my code-comment was accidentally misleading about what the code was doing. :) I take full responsibility.

> > +  // (Note: "order" doesn't influence position or paint order for abspos
> > +  // children - only for (in-flow) flex items.)
> 
> It would be nice if the test verified that.  Perhaps you could set
> a random value between 1..3 on the abs.pos. elements or something?

The new test does already verify this, by setting "order" values on the abs.pos elements.

The first two lines below that comment are testing two abspos flex children with different "order" values, and it asserts that those two cases look the same.  Those lines are:
   [ 1, 2, 3, "ab", "ABc"],
   [ 2, 1, 3, "ab", "ABc"],

Here, "a" and "b" are both abspos (from the "ab" string) and in the first testcase "a" has the smaller order, and in the second testcase "b" has the smaller order.  And we're asserting that they both look the same, like the "ABc" reference case, which is just <refA abs/><refB abs/><refC/>, basically [which has <refB> stacked on top of <refA> in the upper-left corner of the flex container].

If we *did* let order influence paint-order for abspos children, then the second testcase there would instead match a hypothetical "BAc" reference case [with <refA> stacked on top of <refB> instead of the other way around].

> > * Note: The items in this testcase don't overlap, so this testcase does _not_
> > * test paint ordering.  It only tests horizontal ordering in a flex container.
> 
> It seems it would be fairly easy to enhance these tests to do that.
> Just add a child in each of the current children and make it slightly
> overflow
> and set background:inherit on it?

I'll think about that, but for now I won't bother, since it's not related to this change & I want to keep this narrowly focused & get it in ASAP.  (And we do have reftests for "order" & paint-ordering elsewhere, as I recall.)
Hmm, the Try run from comment 11 shows a VTT issue related to whether or not something is enabled. It's hard to tell exactly what's going on since VTT stuff happens under the hood, but I have a theory about the cause, and my theory means this isn't as trivial to fix as I'd thought. :-/

So -- this patch *does* achieve what I was originally aiming to achieve in https://hg.mozilla.org/integration/mozilla-inbound/rev/86fbc1fb2818 -- sorting abspos placeholders to the start of the child list.  BUT, that causes some fallout that I hadn't thought of.  That'll influence the focus order (the "Tab"-key traversal order), making all of the abspos flex items always get focused first, once any sorting has happened (i.e. once any frame has had "order" set on it).  And that might actually cause more (or as much) breakage in the wild than the bug that I'm fixing here... hard to say.

The fix for that is to fix bug 812687, I think -- probably in the same way that we addressed that for grids (by making the sorting temporary & repeating it on each reflow).  And that would be a big enough change that we probably shouldn't take it in a Firefox 52 dot release after all.  (Though we perhaps could justify uplifting it to 53 beta, if it's ready soon enough & if it helps us address this regression in Firefox 52.)
(In reply to Daniel Holbert [:dholbert] from comment #13)
> Hmm, the Try run from comment 11 shows a VTT issue related to whether or not
> something is enabled.

Sorry, that wasn't a very helpful description -- I meant to say "...related to elements & selection".  [The connection to selection is what made me wonder about focus/tab-order.]

Sample Try log for this failure: https://treeherder.mozilla.org/logviewer.html#?job_id=83148587&repo=try
I wouldn't worry about tab-order/selection too much, that's probably already
quite weird when 'order' is used, in all browsers.  I think you should go ahead
and land this because a correct rendering order is more important.
That makes sense to me. (I do need to update the test to avoid the failure in comment 14, before I land, though.)
Flags: needinfo?(dholbert)
Turns out the Video/VTT test failure in comment 14 is a legitimate rendering/interaction issue, not just a focus-handling thing with "order".  It's a symptom of a broader flaw in the premise of this whole "sort placeholders to the start" plan in the first place. :-/   (from Bug 1269045 part 1)

So, I don't think we can (or should) take the current fix here after all.

In Bug 1269045 part 1, the major premise was: abspos children will all stack on top of other children, so their frametree ordering relative to the other children doesn't matter -- we can just shift them all to the start, and we'll still paint the same thing.

This is true in simple cases, but it's not true if some of the flex items have their own abspos children. In particular, if we have the following pseudo-HTML (with "position" only being set on the abspos things)...
 <flex container>
   <item><abspos A/></item>
   <abspos B/>
 </flex container>
...then "abspos B" SHOULD be painted on top of (and cover up) "abspos A" [assuming they overlap], because abspos things are drawn in document order.  But bug 1269045 part 1 (fixed up with my patch here) would have us shuffle <abspos B> to the start of the frame tree, and then it would draw before abspos A and would get covered up incorrectly.

So, I tend to think we really need to:
 (a) hope that not too many sites are affected by this bug here (seems likely or at least possible, given that we got no reports while 52 was in prerelease and haven't heard anything since, and given that flex + abspos hasn't worked interoperably until ~52 anyway)
 (b) Revert Bug 1269045 part 1 and address the thing it was meaning to solve via a proper fix for bug 812687 instead.
 (c) hopefully do that in time to ship in 53.
Here's a reduced testcase from the <video> mochitest that was failing.

With the patch applied, this testcase is broken as follows:
- if you click the "CC" button, the subtitle menu appears [as it should] BUT it's underneath the video's transparent overlay, so it looks grayed out and you can't actually click on any of its menu items. (These clicks/mouse-events get blocked by the video's transparent overlay, and they're interpreted as clicks on the video itself. They cause the video to pause or play, or to fullscreen if you double-click.)
Here's a further-reduced testcase which is kind of like my pseudo-HTML from comment 18.

EXPECTED RENDERING:
 - No red should be visible. Only the "Front" lime div.
ACTUAL RENDERING, with this bug's current patch applied:
 - Red div is visible (and its "Cover Me!" text), superimposed on top of the lime div.
Flags: needinfo?(dholbert)
Flags: needinfo?(dholbert)
Blocks: 1349864
[Tracking Requested - why for this release]: It's too late for 52.0.2 (coming tomorrow) but should be tracked for 53 as this may break page layout.
Add "Absolutely-Positioned Flex Children" in MDN Browser compatibility: https://developer.mozilla.org/en-US/docs/Web/CSS/order#Browser_compatibility
We could still consider a last minute patch for 53 uplift. Is this likely to affect many existing pages or do we not know?
I've no idea how many pages could be affected, but this bug affects our site (ulozto.net / uloz.to) quite a lot (users complain about it) and we would definitely welcome to have it fixed in 53...
Same issue here: https://www.zurich.ch/en/private-customers/product-overview
even if the issue does not happen very often - if it happens - it completely breaks the layout. So I'd be very happy to see this fixed (or reverted) in 53
Depends on: 812687
Comment on attachment 8846122 [details] [diff] [review]
part 1: Make flex-item sorting code *actually* sort placeholders to the start of the child list, as intended

I'm obsoleting the patch here, since it causes other problems per comment 13.

I'll be fixing this by fixing bug 812687 instead.
Attachment #8846122 - Attachment is obsolete: true
No longer blocks: 1349864
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #24)
> We could still consider a last minute patch for 53 uplift.

I've just landed patches that fix this over on bug 812687, and I requested uplift approval over there, if it's not too late.  (Don't be too scared away by the multi-patch stack -- only one of the patches there actually changes behavior. The rest are renaming/moving existing code, with no behavior change, in the service of letting the main patch share some existing known-good code.)

> Is this likely to
> affect many existing pages or do we not know?

I don't know how many pages might be affected -- probably not too many, since we only got reports after we'd shipped this in release.  But per comment 25 & comment 26, it breaks the layout pretty badly, for the pages that are affected.
Flags: needinfo?(dholbert)
OK, so this is now FIXED on trunk via bug 812687 having landed. (Shipping in tomorrow's nightly, I think.)

In the meantime, though, I realized there's a simpler band-aid here. The original patch here was fixing the vision of Bug 1269045 part 1 (sorting all placeholders to the start), and we discovered that was problematic. But I think we can actually do something similarly-simple & not-problematic -- just treat all placeholders as having the default "order", so that we can sort reordered stuff *around* them, without unnecessarily adjusting their own spot in the child list.

This is likely-safe because it's basically what we did before bug 1269045 -- at that point, placeholders were all wrapped in anonymous flex items, which were unstylable & hence had the default value for "order".

Clearly we don't need such a patch on Nightly (since per bug 812687 we won't be sorting the frame list at all there), but we could take it as an alternate backport on Aurora/Beta.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Note: the testcase included in this patch is the same as the one in the previous version, so it doesn't need re-review. The (small) code change is the only part that needs a look.
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=667391f9bec16cb88648b765c6c0aec9b9b653b7

I've locally verified that the patch passes its included mochitest, and that it doesn't break the attached "breaktests" (which are derived from the test failure in comment 18).
Comment on attachment 8855439 [details] [diff] [review]
part 1 v2: treat placeholders as having default order/box-ordinal-group value

To keep things moving, I'm going to optimistically request uplift approval here, on the assumption that Mats will r+ this (though I won't land anywhere until it's got r+).

This uplift-request is a more targeted alternative to the one in bug 812687 comment 66.

Approval Request Comment
[Feature/Bug causing the regression]:  bug 1269045, which was really just a helper-bug for bug 874718 (a large spec change about handling abspos children in a css flexbox).
[User impact if declined]: Broken layout on any websites that have a css flexbox with "order"-reordered children as well as abspos children. (We've received several reports of such sites being broken -- this bug & its dupes.)
[Is this code covered by automated tests?]:  Yes. We have fairly comprehensive flexbox tests, and I've included an additional test in this patch to verify our new behavior.
[Has the fix been verified in Nightly?]: I've verified it in a local Nightly build from yesterday.  Actual-nightly (as of today or tomorrow) doesn't exhibit the bug anymore, because it's got a different fix (bug 812687) that replaces all of this code with something better.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Not very risky.
[Why is the change risky/not risky?]:  This effectively restores the behavior we had before this regressed -- treating abspos placeholders as having the default value of the ordering properties.  (The regressing patch somewhat-arbitrarily made us do something different, because I thought it didn't matter, but it turns out it does.)
[String changes made/needed]: None.
Attachment #8855439 - Flags: approval-mozilla-beta?
Attachment #8855439 - Flags: approval-mozilla-aurora?
If this bug gets uplift approval, we should probably take this on all branches, rather than uplifting bug 812687.

(IMO bug 812687 is safe to uplift at least as far as Aurora & probably safe for beta/ESR52 as well -- but it'd be good to have extra testing for this patch here if we're going to ship it in 52, so might as well just use this as the uplift for all branches in that spirit.)
Comment on attachment 8855439 [details] [diff] [review]
part 1 v2: treat placeholders as having default order/box-ordinal-group value

r=mats
Attachment #8855439 - Flags: review?(mats) → review+
(In reply to Daniel Holbert [:dholbert] from comment #35)
> (IMO bug 812687 is safe to uplift at least as far as Aurora & probably safe
> for beta/ESR52 as well -- but it'd be good to have extra testing for this
> patch here if we're going to ship it in 52, so might as well just use this
> as the uplift for all branches in that spirit.)

Right, I think this patch is slightly less risky than bug 812687,
although that patch isn't that risky either given that Grid has
been using that code for quite a while (it's shipping in v52).
So landing this patch on both Aurora/Beta initially sounds good
to me, to get some extra testing from our Aurora users.  We could
then backout this and uplift bug 812687 instead on Aurora after
a few weeks if we decide we want to take that one cycle earlier.
That makes sense to me, yeah.

I'll be reposting the patch with one minor tweak -- restoring the fix from bug 1221112, which was removed as part of the regressing patch here (in bug 1269045) and which we need to restore now that we're sorting placeholders again.  (i.e. we need to call nsPlaceholderFrame::GetRealFrameFor when looking up the pseudo-tag to compare, during the DOM-fallback LEQ sorting function)

Without that, we trip the assertion from bug 1221112 during that bug's crashtest (as revealed by the try run in comment 33).

I won't bother requesting re-review for that change, since that exact change was previously reviewed (in bug 1221112) and the tweak is just restoring that piece of code which we need again now that placeholders are participating in sorting again.
(Transferring approval request to updated version of patch. See approval request in comment 34.)
Attachment #8855439 - Attachment is obsolete: true
Attachment #8855439 - Flags: approval-mozilla-beta?
Attachment #8855439 - Flags: approval-mozilla-aurora?
Attachment #8855529 - Flags: review+
Attachment #8855529 - Flags: approval-mozilla-beta?
Attachment #8855529 - Flags: approval-mozilla-aurora?
Comment on attachment 8855529 [details] [diff] [review]
part 1 v3: treat placeholders as having default order/box-ordinal-group value [r=mats]

We should take this on ESR52, also.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a targeted fix for an edge-case layout regression (which was a new regression in Firefox 52).
User impact if declined: Broken layout on some sites; see comment 34.
Fix Landed on Version: Hopefully the fix will be landing in Firefox 53 beta & 54 aurora. So hopefully it'll be shipping in Firefox 53.
Risk to taking this patch (and alternatives if risky): Not very risky. (see comment 34 for more)
String or UUID changes made by this patch:  None.
Attachment #8855529 - Flags: approval-mozilla-esr52?
Whiteboard: [fixed in mozilla-central by bug 812687. Fixing on branches via more targeted band-aid]
Updated Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5eb9cb271ba40f0e10bd608c6187b711efa8b5d2
(Using "-p all -u all" for maximum sensitivity to catch any unforseen issues, since backouts on branches are extra-annoying.)
Comment on attachment 8855529 [details] [diff] [review]
part 1 v3: treat placeholders as having default order/box-ordinal-group value [r=mats]

Let's uplift this smaller fix for aurora and beta. Thanks dholbert!
Attachment #8855529 - Flags: approval-mozilla-beta?
Attachment #8855529 - Flags: approval-mozilla-beta+
Attachment #8855529 - Flags: approval-mozilla-aurora?
Attachment #8855529 - Flags: approval-mozilla-aurora+
Attached patch [wrong file] (obsolete) — Splinter Review
There are some unimportant contextual changes that prevent "part 1 v3" from applying directly on mozilla-beta. Here's a rebased version that does apply cleanly there.
Attachment #8855538 - Attachment description: part1 v3, rebased for mozilla-beta → [wrong file]
Attachment #8855538 - Attachment is obsolete: true
Setting qe-verify- based on Daniel's assessment on manual testing needs (see Comment 34) and the fact that this fix has automated coverage.
Flags: qe-verify-
Great, our site (ulozto.net) is looking well again in latest Nightly, thanx!
Comment on attachment 8855529 [details] [diff] [review]
part 1 v3: treat placeholders as having default order/box-ordinal-group value [r=mats]

fix this regression in esr52
Attachment #8855529 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
(In reply to Marek Raida from comment #48)
> Great, our site (ulozto.net) is looking well again in latest Nightly, thanx!

Hooray, thanks for letting us know!

BTW, Nightly's actually fixed via the more-involved patch stack over on bug 812687, rather than the patches here. If you get a chance, would you mind testing latest Developer Edition 54 to validate that the patches from this bug here fixed things for you?  (The patches landed there in Comment 45.)
https://www.mozilla.org/en-US/firefox/developer/

Thank you!

(In reply to Julien Cristau [:jcristau] from comment #49)
> fix this regression in esr52

Thanks for the approval+. I've uplifted the beta patches to ESR52:
remote:   https://hg.mozilla.org/releases/mozilla-esr52/rev/75cea353ad787814158fbbb0805c15711f327f74
remote:   https://hg.mozilla.org/releases/mozilla-esr52/rev/b08ef5a82f89190b9daaacd05b07bf63adbbdb31
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: