Closed Bug 807897 Opened 7 years ago Closed 7 years ago

Setting "z-index" should convert a flex item into a stacking context (even for "position:static")

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file)

If a flex item has its z-index set (to anything other than "auto"), that needs to make it create a stacking context, even if it's "position:static".
  http://dev.w3.org/csswg/css3-flexbox/#painting

I've got some code to do this already; patch coming soon.  This blocks bug 783474, because that bug's patch stacks on top of this bug's patch.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #677676 - Flags: review?(dbaron)
Comment on attachment 677676 [details] [diff] [review]
fix w/ reftest, v1

Maybe you should change nsIFrame::IsPseudoStackingContextFromStyle as well (to check IsFlexItem() && zindex != auto), though really that would just be for completeness, since it doesn't matter to any of the current callers.

I'm going to mark this r=dbaron, though I should perhaps have transferred review to roc initially.  I've at least cc:ed him.
Attachment #677676 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] from comment #2)
> Comment on attachment 677676 [details] [diff] [review]
> fix w/ reftest, v1
> 
> Maybe you should change nsIFrame::IsPseudoStackingContextFromStyle as well
> (to check IsFlexItem() && zindex != auto), though really that would just be
> for completeness, since it doesn't matter to any of the current callers.

I'm hesitant to do that, for 3 reasons:

 (a) as you say, it doesn't matter to the current callers. (it's only called in table-layout)

 (b) the documentation for that method says "looking only at style", whereas IsFlexItem() looks at the frame-tree. (though maybe that doesn't matter since it's cheap...  I could easily change the documentation to say "and the frame tree" I suppose.)

 (c) IsPseudoStackingContextFromStyle() appears to already be incomplete -- it's missing e.g. checks for SVG effects, CSS transforms, and some extra fanciness for animated opacity that we check in nsIFrame::HasOpacity().  (All of those things end up toggling this line in BuildDisplayListForChild():
> 2205     // If you change this, also change IsPseudoStackingContextFromStyle()
> 2206     pseudoStackingContext = true;
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp?mark=2196-2206#2196

...and per that comment at line 2205, I suspect those things *should* be checked in IsPseudoStackingContextFromStyle(), but they aren't currently).


So -- are you OK with this landing as-is, without that change?
(In reply to Daniel Holbert [:dholbert] from comment #3)
> So -- are you OK with this landing as-is, without that change?

yes.
https://hg.mozilla.org/mozilla-central/rev/2f30020727e9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.