877890 - Placeholders should use the "order" and "-moz-box-ordinal-group" values from their OOF frame, when we're sorting them in a flex container / box frame

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Currently, setting the ordering properties[1] on an abspos child of a flex container (or an old-style XUL flexbox) will have no effect, because the nsPlaceholderFrame ends up being the thing that the container sorts, and the nsPlaceholderFrame has its own nsStyleContext that has the default ordering properties.

Intuitively, it seems like the nsPlaceholderFrame should copy these properties from its out-of-flow frame, either via inheritance (& making the nsPlaceholderFrame inherit from its OOF), or some other method.

This bug caused problems with one of the easy attempted solutions in bug 853415 [2], FWIW, and we also need it for "order" to be spec-compliant. (though the abspos-handling part of the flexbox spec is still labeled at-risk)

[1] "order" for new flexbox (display:flex), -moz-box-ordinal-group for old flexbox (display:-moz-box)

[2] In bug 853415, we had some abspos generated content from ":after", which was getting sorted to be the *second-to-last child* (which changed the layout in an undesirable way), due to another child having a large -moz-box-ordinal-group value. I suggested that we set -moz-box-ordinal-group to an even larger value on the ":after" content, but this bug here thwarted that & made it have no effect.

Comment 1

[Boris Zbarsky [:bzbarsky]]

The other option, I guess, is when getting the "order" whole sorting to get it from nsPlaceholderFrame::GetRealFrameFor(f) instead of f itself....

Comment 2

[Daniel Holbert [:dholbert]]

Oh, right -- I like that better. I forgot that we had that handy utility-function; that abstracts away the placeholder special-casing to make it more palatable than I was imagining.

So we'd need to swap out the frames for their "real frames" in:
* IsBoxOrdinalLEQ() in nsBoxFrame
 http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsBoxFrame.cpp#1886
* IsOrderLEQ() and IsOrderLEQWithDOMFallback() in nsFlexContainerFrame
 http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp#512
...and with that, we should be good.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1 [no tests yet]

This just has comment 2, coded up.

This compiles, but I haven't tested it (& need to write some unit tests).

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix v1a [no tests yet]

[fixed a minor comment typo]

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v1b [now with tests]

This includes two reftests, which test this via testing paint-order of absolutely positioned elements with ordinal values set to reverse their DOM order (which should reverse them in the frame tree, and hence should reverse their paint order).
The tests:
 * box-ordinal-with-out-of-flow-1.html fails before the patch, and passes afterward.

 * flexbox-paint-ordering-3.html fails before the patch, *and fails afterward*, because we currently wrap placeholders in anonymous flex items (as described in bug 874718 -- see the first comment there.  Basically, our code for handling abspos elements in a flex container is from and older revision of the spec).

So, I flagged flexbox-paint-ordering-3.html as "fails" with a comment pointing to bug 874718.

Comment 6

[Daniel Holbert [:dholbert]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=9f57c44c0523

Comment 7

[Daniel Holbert [:dholbert]]

[Try run is fully green so far (crash/mochi/reftests), and two platforms have completely finished, so I think this is good]

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 756784 [details] [diff] [review]
fix v1b [now with tests]

r=me

Comment 9

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6eb787f348de

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6eb787f348de