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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix w/ reftest, v1

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 3

[Daniel Holbert [:dholbert]]

(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?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

yes.

Comment 5

[Daniel Holbert [:dholbert]]

Thanks! https://hg.mozilla.org/integration/mozilla-inbound/rev/2f30020727e9

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2f30020727e9