Closed Bug 877890 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
The other option, I guess, is when getting the "order" whole sorting to get it from nsPlaceholderFrame::GetRealFrameFor(f) instead of f itself....
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.
Summary: Placeholders should steal (through inheritance?) the "order" and "-moz-box-ordinal-group" values from their OOF frame → 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
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Flags: in-testsuite?
OS: Linux → All
Hardware: x86_64 → All
Attached patch fix v1 [no tests yet] (obsolete) — Splinter Review
This just has comment 2, coded up.

This compiles, but I haven't tested it (& need to write some unit tests).
Attached patch fix v1a [no tests yet] (obsolete) — Splinter Review
[fixed a minor comment typo]
Attachment #756617 - Attachment is obsolete: true
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.
Attachment #756620 - Attachment is obsolete: true
Attachment #756784 - Flags: review?(bzbarsky)
Blocks: 853415
[Try run is fully green so far (crash/mochi/reftests), and two platforms have completely finished, so I think this is good]
Comment on attachment 756784 [details] [diff] [review]
fix v1b [now with tests]

r=me
Attachment #756784 - Flags: review?(bzbarsky) → review+
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/6eb787f348de
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Blocks: 1431856
You need to log in before you can comment on or make changes to this bug.