Closed
Bug 877890
Opened 12 years ago
Closed 12 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)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(1 file, 2 obsolete files)
10.22 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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•12 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•12 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•12 years ago
|
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Flags: in-testsuite?
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Comment 3•12 years ago
|
||
This just has comment 2, coded up.
This compiles, but I haven't tested it (& need to write some unit tests).
Assignee | ||
Comment 4•12 years ago
|
||
[fixed a minor comment typo]
Assignee | ||
Updated•12 years ago
|
Attachment #756617 -
Attachment is obsolete: true
Assignee | ||
Comment 5•12 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 | ||
Comment 6•12 years ago
|
||
Assignee | ||
Comment 7•12 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 8•12 years ago
|
||
Comment on attachment 756784 [details] [diff] [review]
fix v1b [now with tests]
r=me
Attachment #756784 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 9•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Flags: in-testsuite? → in-testsuite+
Comment 10•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in
before you can comment on or make changes to this bug.
Description
•