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

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla24
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

6 years ago
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....
Assignee

Comment 2

6 years ago
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.
Assignee

Updated

6 years ago
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

Updated

6 years ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Flags: in-testsuite?
OS: Linux → All
Hardware: x86_64 → All
Assignee

Comment 3

6 years ago
Posted 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).
Assignee

Comment 4

6 years ago
Posted patch fix v1a [no tests yet] (obsolete) — Splinter Review
[fixed a minor comment typo]
Assignee

Updated

6 years ago
Attachment #756617 - Attachment is obsolete: true
Assignee

Comment 5

6 years ago
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)
Assignee

Updated

6 years ago
Blocks: 853415
Assignee

Comment 7

6 years ago
[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+
Assignee

Updated

6 years ago
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/6eb787f348de
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Assignee

Updated

a year ago
Blocks: 1431856
You need to log in before you can comment on or make changes to this bug.