https://drafts.csswg.org/css-contain/#containment-layout says: 4. The element acts as a containing block for absolutely positioned and fixed positioned descendants. but based on the rather complicated test at https://dbaron.org/css/test/2018/stacking-context-z-order , when I enable layout.css.contain.enabled it seems like we don't do this for contain:layout. (Feel free to ask me for help understanding the test if it's not clear.)

The contain:layout implementation is still in process re: 1463593, so (if I'm understanding your test cases correctly) it seems reasonable that contain:layout doesn't create a containing block just yet. For what its worth, it looks like contain:paint and contain:style both pass on my nightly build with the pref enabled, and Yusuf is working on borrowing code from those implementations to create the same functionality for contain:layout. When those patches land I can come back to this and make sure things look as they should.

Could you explain what the expected functionality is for contain:size? The spec [1] doesn't have any specifications about establishing a stacking context, so I'm a little confused about why it appears on your testing list. Does either one of the following specified properties indicate the need for a stacking context? 1. When laying out the containing element, it must be treated as having no contents. After layout of the element is complete, its contents must then be laid out into the containing element’s resolved size. Replaced elements must be treated as having an intrinsic width and height of 0. 2. Elements with size containment are monolithic (See CSS Fragmentation Module Level 3 §possible-breaks). [1] https://drafts.csswg.org/css-contain/#containment-size

(In reply to Morgan Reschenberg [:morgan] from comment #1) > The contain:layout implementation is still in process re: 1463593, so (if > I'm understanding your test cases correctly) it seems reasonable that > contain:layout doesn't create a containing block just yet. For what its > worth, it looks like contain:paint and contain:style both pass on my nightly > build with the pref enabled, and Yusuf is working on borrowing code from > those implementations to create the same functionality for contain:layout. > When those patches land I can come back to this and make sure things look as > they should. We just talked with Morgan about this, and here are few notes just for clarification: - Contain:style does not care about the stacking context (and appropriately not given place in the test in comment 0), thus it is negligible for this test cases purpose. The reason both strict and content passes is probably because contain:paint. - Contain:style is still in progress and not pushed. - Hopefully, I want to take on few tasks from the contain:layout (bug 1463593) as soon as I have the chance, but I am currently occupied with contain:style, and can't estimate an exact finish date on that.

(In reply to Morgan Reschenberg [:morgan] from comment #2) > Could you explain what the expected functionality is for contain:size? The > spec [1] doesn't have any specifications about establishing a stacking > context, so I'm a little confused about why it appears on your testing list. That's the bit about the testcase being a bit confusing. The red background behind 'contain:size' in the first column means that it's not actually expected to establish a stacking context, and the lack of a green square next to it means it's not expected to establish a containing block for absolutely positioned or fixed positioned elements. So for contain:size the expected results are red-red-red-green-red. Perhaps not the best testcase, but it's complicated...

and if I understand correctly, for the "contain:layout" row, the expected results are red-red-red-green-green (green in the final column), and this bug here covers getting that final column correct.

Comment on attachment 8993761 [details] Bug 1472919 - Establish stacking context, containing block, independent formatting context for contain:layout. https://reviewboard.mozilla.org/r/258460/#review265486 ::: layout/style/nsStyleStruct.h:2343 (Diff revision 1) > + bool IsContainLayout() const { > + // Note: The spec for size containment says it should > + // have no effect on non-atomic, inline-level boxes. We s/size/layout/

Do the three columns after "establishes a stacking context" test stacking contexts in different cases, or do they test formatting contexts? I made the same changes as Yusuf made for contain:paint (save for adding IsContainLayout() to IsStackingContext here: https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/layout/generic/nsFrame.cpp#10947), but I get red-green-green-green-green as my results. If those are all stacking context cases, let me know and I'll take another look at the overlap between stacking contexts and formatting contexts.

(In reply to Morgan Reschenberg [:morgan] from comment #8) > Do the three columns after "establishes a stacking context" test stacking > contexts in different cases, or do they test formatting contexts? The three columns after "establishes a stacking context" test a painting order condition that, per the consensus in https://github.com/w3c/csswg-drafts/issues/2717 , should also all be green as a result of establishing a stacking context. > I made the same changes as Yusuf made for contain:paint (save for adding > IsContainLayout() to IsStackingContext here: > https://searchfox.org/mozilla-central/rev/ > 8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/layout/generic/nsFrame.cpp#10947), > but I get red-green-green-green-green as my results. If those are all > stacking context cases, let me know and I'll take another look at the > overlap between stacking contexts and formatting contexts. I would expect red-red-red-green-green for the contain:layout line, since contain:layout should *not* establish a stacking context.

(Though it's possible there's some other effect that 'contain:layout' triggers that changes the tests in a way that I didn't consider, but I don't immediately see one.)

Comment on attachment 8993761 [details] Bug 1472919 - Establish stacking context, containing block, independent formatting context for contain:layout. https://reviewboard.mozilla.org/r/258460/#review265494 ::: layout/style/nsStyleStruct.h:2348 (Diff revision 1) > + bool IsContainLayout() const { > + // Note: The spec for size containment says it should > + // have no effect on non-atomic, inline-level boxes. We > + // don't check for these here because we don't know > + // what type of element is involved. Callers are > + // responsible for checking if the box in question is Neither of the callers you've added does this. Should they?

I also just reported https://github.com/w3c/csswg-drafts/issues/2942 which may be an issue here. (I'd be interested to know why you got the results you did on the test, actually.)

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #9) > I would expect red-red-red-green-green for the contain:layout line, since > contain:layout should *not* establish a stacking context. As it stands right now, Morgan's patch gives me red-green-green-green-green for contain:layout. If I revert the one-liner HasFixedPosContainingBlockStyleInternal tweak, then I instead get red-red-red-green-red. So it seems we've got some painting code that (probably indirectly) uses on HasFixedPosContainingBlockStyleInternal to control the stacking/painting order between the contain:layout element and its siblings.

(In reply to Daniel Holbert [:dholbert] from comment #13) > So it seems we've got some painting code that (probably indirectly) uses on > HasFixedPosContainingBlockStyleInternal to control the stacking/painting > order between the contain:layout element and its siblings. It's specifically due to the following logic in nsIFrame::BuildDisplayListForChild, where we check IsAbsPosContainingBlock (which will now return true for our contain:layout thing) -- we use that return value to inform our decision about whether we're a pseudo-stacking-context. And it seems like that pseudo-stacking-context decision is the key thing here for those boxes. > const bool isPositioned = > disp->IsAbsPosContainingBlock(child); > > [...] > if (... || isPositioned || > ...) { > pseudoStackingContext = true; https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/layout/generic/nsFrame.cpp#3670-3671,3677,3681 If I edit that ^^ code to *avoid* the pseudoStackingContext = true assignment for contain:layout things, then we match dbaron's expected red-red-red-green-green rendering. (specifically, I replaced "isPositioned" with "(isPositioned && !disp->IsContainLayout())" in the logic guarding the pseudoStackinContext assignment.) This isn't a change we should actually make -- I'm just mentioning it for illustration to point out that this is the key piece that matters here.

At the bottom of that function, the "are we a pseudo-stacking-context" question comes into play here: > if (isPositioned || isStackingContext || > (aFlags & DISPLAY_CHILD_FORCE_STACKING_CONTEXT)) { > // Genuine stacking contexts, and positioned pseudo-stacking-contexts, > // go in this level. > if (!list.IsEmpty()) { > nsDisplayItem* item = WrapInWrapList(aBuilder, child, &list, wrapListASR, canSkipWrapList); > if (isSVG) { > aLists.Content()->AppendToTop(item); > } else { > aLists.PositionedDescendants()->AppendToTop(item); https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/layout/generic/nsFrame.cpp#3824,3833 ...and inside of this clause, we append the contents of a local variable "list", which was populated from "pseudoStack" above, in the case that we're a pseudo-stacking context. (Note that the variable "isPositioned" is true here for contain:layout things, because really it just means "are we an abspos containing block", per the code quoted in comment 14. Its variable-name isn't super accurate right now.) If I start with Morgan's patch and modify this above-quoted "isSVG" clause to "isSVG || disp->IsContainLayout()", then I get dbaron's expected red-red-red-green-green results. So the problem here is that we're appending our not-really-positioned contain:layout stuff to the "PositionedDescendants()" list (which puts it at a different place in the stacking order than we're expecting). And this is because we incorrectly think (at least from variable naming & comments) that IsAbsPosContainingBlock()==true corresponds to being positioned.

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #12) > (I'd be interested to know why you got the results you did on the test, > actually.) Hopefully comment 14 and 15 make this clearer. The bottom line here is that this display list code semi-hackily treats IsAbsPosContainingBlock() as a signal that the frame is "positioned". Up until now, this has been basically accurate, because IsAbsPosContainingBlock() only returns true if the frame is indeed positioned (via 'position') OR if it has some other style that makes it a stacking context, which is a strictly-stronger requirement for our purposes, and in which case we don't care that it's technically not positioned. Put another way: contain:layout is the first CSS feature that... - makes a frame an abs-pos containing block... - without making the frame be considered "positioned"... - *and* without making the frame a stacking context (This is a slightly more specific statement than what dbaron expressed in https://github.com/w3c/csswg-drafts/issues/2942#issue-343223685 )

Pending consensus at: https://github.com/w3c/csswg-drafts/issues/2942

Comment on attachment 8993761 [details] Bug 1472919 - Establish stacking context, containing block, independent formatting context for contain:layout. https://reviewboard.mozilla.org/r/258460/#review266192 ::: layout/style/nsStyleStruct.h:2348 (Diff revision 1) > + bool IsContainLayout() const { > + // Note: The spec for size containment says it should > + // have no effect on non-atomic, inline-level boxes. We > + // don't check for these here because we don't know > + // what type of element is involved. Callers are > + // responsible for checking if the box in question is I don't think they need to do this becuase: the first call to IsContainLayout() is in nsBlockFrame, which [based on the IsAtomicElement() method](https://searchfox.org/mozilla-central/rev/d160ac574a5798010eda4e33e91ee628aa268a33/layout/generic/TextOverflow.cpp#75), shouldn't apply to block frames. The second call is in nsStyleDisplay::HasFixedPosContainingBlockStyleInternal, which doesn't have direct access to a frame much like the IsContainLayout() method itself (Please re-open if you disagree)

Currently passing tests from w3 suite (http://test.csswg.org/harness/test/css-contain-1_dev/section/3.2/), and looks like the standing patch, which establishes both an independent formatting context and a stacking context, is in line with where the discussion is going on the linked github issue. Should I write additional tests for this part of contain:layout that also test the stacking context part? I can also go through the contain:paint tests which test stacking/formatting contexts and make copies for contain:layout to ensure they match.

(In reply to Morgan Reschenberg [:morgan] from comment #20) > looks like the standing patch, which establishes both an independent > formatting context and a stacking context I don't think the current patch *quite* establishes a stacking context, does it? Per comment 13, the first column is "red" for contain:layout. (The other columns indicate that it does some other things that are associated with stacking contexts, but it's not yet a full stacking context.) To fully make a stacking context, it looks like you need to add a check for this to nsIFrame::IsVisuallyAtomic alongside the existing IsContainPaint check there (from bug 1463599). > Should I write additional > tests for this part of contain:layout that also test the stacking context > part? You should write test for that, but probably best to avoid putting them in w3c-css/submitted until there's been a CSSWG resolution and/or spec update. Maybe create layout/reftests/css-contain and drop them there (at least for the stacking context ones), and we can move them into the submitted folder down the line when the spec is updated (assuming this proposal is accepted). > I can also go through the contain:paint tests which test > stacking/formatting contexts and make copies for contain:layout to ensure > they match. That sounds great.

Comment on attachment 8993761 [details] Bug 1472919 - Establish stacking context, containing block, independent formatting context for contain:layout. https://reviewboard.mozilla.org/r/258460/#review266570 I think the fixed-pos and abs-pos containing block bits still need work (that is, I disagree with comment 18), so marking as review- again. (Sorry about that.) See comments below. I wish we had a way to make sure we notice if there are future cases where we need to do the formatting context thing for future layout types, since it's not clear that the current code fixes things forever... but I don't see a good way to deal with that so I'm not requiring you to do anything about it. ::: layout/generic/nsBlockFrame.cpp:7060 (Diff revision 2) > - // If the box has contain: paint (or contain: strict), then it should also > - // establish a formatting context. > + // If the box has contain: paint, contain:layout, or contain:strict, > + // then it should also establish a formatting context. The (or contain:strict) should probably still be a parenthetical since 'contain:strict' is defined to be the combined behavior of 'contain: size layout style paint'. ::: layout/generic/nsBlockFrame.cpp:7067 (Diff revision 2) > if (StyleDisplay()->mDisplay == mozilla::StyleDisplay::FlowRoot || > (GetParent() && StyleVisibility()->mWritingMode != > GetParent()->StyleVisibility()->mWritingMode) || > - StyleDisplay()->IsContainPaint()) { > + StyleDisplay()->IsContainPaint() || > + StyleDisplay()->IsContainLayout()) { > AddStateBits(NS_BLOCK_FORMATTING_CONTEXT_STATE_BITS); I wish we could have a mechanism to catch future cases where we might need to do the same thing for other formatting context types, but I don't see how. ::: layout/style/nsStyleStructInlines.h:164 (Diff revision 2) > { > // NOTE: Any CSS properties that influence the output of this function > // should have the FIXPOS_CB flag set on them. > NS_ASSERTION(aStyle.ThreadsafeStyleDisplay() == this, "unexpected aStyle"); > > - if (IsContainPaint()) { > + if (IsContainPaint() || IsContainLayout()) { So I think this is testably wrong in that it will cause non-replaced inline elements to become abs-pos and fixed-pos containing blocks. You should add a test to web-platform-tests (via w3c-css/submitted is fine) that shows this. Instead, I think you should abstract the combination of this test *and* a test for not being a non-replaced inline *and* the existing calls to HasTransform and HasPerspective into a single function and then call that from both IsAbsPosContainingBlock and IsFixedPosContainingBlock.

(In reply to Daniel Holbert [:dholbert] (PTO 7/26 - 7/30) from comment #21) > You should write test for that, but probably best to avoid putting them in > w3c-css/submitted until there's been a CSSWG resolution and/or spec update. > Maybe create layout/reftests/css-contain and drop them there (at least for > the stacking context ones), and we can move them into the submitted folder > down the line when the spec is updated (assuming this proposal is accepted). I'd just put them in w3c-css/submitted so we don't forget about them, but add a link rel="help" pointing to the github issue in the csswg-drafts repo in addition to the one pointing to the spec.

Firefox 63.0a1 buildID=20180730221422 fails http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-cell-001.html http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-cell-002.html Reference file: http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-layout-cell-001-ref.html 'contain: layout' applies to 'table-cell' elements; in both tests, the div#contain (and td#contain in 002) should act as the containing block for div#abs-pos .

Comment on attachment 8993761 [details] Bug 1472919 - Establish stacking context, containing block, independent formatting context for contain:layout. https://reviewboard.mozilla.org/r/258460/#review266570 > So I think this is testably wrong in that it will cause non-replaced inline elements to become abs-pos and fixed-pos containing blocks. You should add a test to web-platform-tests (via w3c-css/submitted is fine) that shows this. > > Instead, I think you should abstract the combination of this test *and* a test for not being a non-replaced inline *and* the existing calls to HasTransform and HasPerspective into a single function and then call that from both IsAbsPosContainingBlock and IsFixedPosContainingBlock. Could you say a little more about this? From what I understand: the spec says contain:layout shouldn't apply to non-atomic, inline boxes and you're concerned that with this code it would cause them to become abs-pos and fixed-pos containing blocks. So this might be one of those cases where we have to do that percursory check in addition to IsContainLayout() to see we're not also one of those special boxes (FWIW, looks like contain:paint also shouldn't apply to this type; I'll look and see if there's a test already for the same functionality with contain:paint). I don't immediately see where the calls for HasTransform()/HasPerspective() that you mention are (I see the fuctions above this one, and the calls in IsFixedPosContainingBlockForAppropriateFrame() below, though). Why are these important for fixing this problem and why do we want to abstract them into a single function?

So HasFixedPosContainingBlockStyleInternal is a helper function for IsFixedPosContainingBlockForAppropriateFrame and for IsFixedPosContainingBlock, which are the publicly-exposed methods (the "ForAppropriateFrame" variant being used only in CalcStyleDifference, which can be pessimistic). Likewise for the AbsPos variants. There are HasTransform and HasPerspective calls in three places: IsFixedPosContainingBlockForAppropriateFrame, IsFixedPosContainingBlock, and IsAbsPosContainingBlock. I think you should patch the latter two and *probably* leave the third alone (since contain already causes frame reconstructs).

But also see bug 1479859 which I just filed.

The CSS WG just resolved https://github.com/w3c/csswg-drafts/issues/2942 to say that it should also establish a stacking context.

Ignore review request, just rebasing and pushing tests that currently pass.

I'll fix the issue in comment 25 and comment 26 in bug 1479859. It turns out (a) there's already a web-platform-test (b) we were passing it because we weren't actually calling IsAbsoluteContainingBlock for inlines (c) there were some other bugs to fix, like the one in bug 1479859 comment 0, and (d) the patches to do all of those things are pretty inter-related.

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #31) > I'll fix the issue in comment 25 and comment 26 in bug 1479859. It turns > out (a) there's already a web-platform-test (b) we were passing it because > we weren't actually calling IsAbsoluteContainingBlock for inlines (c) there > were some other bugs to fix, like the one in bug 1479859 comment 0, and (d) > the patches to do all of those things are pretty inter-related. Sounds good, I'll hold on to the partial patch I have and rebase it when your changes land.

Comment on attachment 8993761 [details] Bug 1472919 - Establish stacking context, containing block, independent formatting context for contain:layout. https://reviewboard.mozilla.org/r/258460/#review266570 > Could you say a little more about this? > From what I understand: the spec says contain:layout shouldn't apply to non-atomic, inline boxes and you're concerned that with this code it would cause them to become abs-pos and fixed-pos containing blocks. So this might be one of those cases where we have to do that percursory check in addition to IsContainLayout() to see we're not also one of those special boxes (FWIW, looks like contain:paint also shouldn't apply to this type; I'll look and see if there's a test already for the same functionality with contain:paint). > > I don't immediately see where the calls for HasTransform()/HasPerspective() that you mention are (I see the fuctions above this one, and the calls in IsFixedPosContainingBlockForAppropriateFrame() below, though). Why are these important for fixing this problem and why do we want to abstract them into a single function? I'm looking into this still to write a test similar to Yusuf's contain-paint-ignored-cases tests, probably something like https://searchfox.org/mozilla-central/rev/0f4d50ff5211e8dd85e119ef683d04b63062ed90/layout/reftests/w3c-css/submitted/contain/contain-paint-ignored-cases-no-principal-box-001.html

The two tests added to that revision ensure contain:layout doesn't generate a stacking context (001) or become a fixed/abspos containing block (002).

* when no principal box is generated (ie. in the case of non-atomic, inline level objects)

Comment on attachment 8993761 [details] Bug 1472919 - Establish stacking context, containing block, independent formatting context for contain:layout. https://reviewboard.mozilla.org/r/258460/#review269140 So if the tests were started as copies from other tests, then you should record them as such in version control. (see "hg help cp", especially about "hg cp --after".) You should probably also add your name as an additional author since you made modifications to them. Otherwise this looks fine. I'd be somewhat interested in looking at the patch again once I can see the diffs relative to the existing tests that these are copied from, but I'm willing to mark review+ at this point.

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #38) > Comment on attachment 8993761 [details] > Bug 1472919 - Establish stacking context, containing block, independent > formatting context for contain:layout. > > https://reviewboard.mozilla.org/r/258460/#review269140 > > So if the tests were started as copies from other tests, then you should > record them as such in version control. (see "hg help cp", especially about > "hg cp --after".) You should probably also add your name as an additional > author since you made modifications to them. Updated files that were copies with 'hg cp <src> <dst> --after' and amended the commit, but I didn't see any changes in 'hg status' or mozreview. One of the files I added was a combination of two sources, so I separated it back out to make the testing (and chain of changes) clearer. This one [1] did show up in mozreview as 'copied from <orig>'. If there is another change I need to make to get the others to show up, let me know. I'll hold off on adding checkin-needed until I hear from you. Thank you for reviewing! [1] contain-layout-ignored-cases-no-principal-box-003.html

Comment on attachment 8993761 [details] Bug 1472919 - Establish stacking context, containing block, independent formatting context for contain:layout. https://reviewboard.mozilla.org/r/258460/#review269258 ::: layout/reftests/w3c-css/submitted/contain/reftest.list:30 (Diff revision 8) > +== contain-layout-stacking-context-001.html contain-paint-stacking-context-001-ref.html > +== contain-layout-formatting-context-float-001.html contain-paint-formatting-context-float-001-ref.html > +== contain-layout-formatting-context-margin-001.html contain-layout-formatting-context-margin-001-ref.html > == contain-size-table-caption-001.html contain-size-table-caption-001-ref.html > +== contain-layout-containing-block-fixed-001.html contain-paint-containing-block-fixed-001-ref.html > +== contain-layout-containing-block-absolute-001.html contain-paint-containing-block-absolute-001-ref.html > +== contain-layout-ignored-cases-no-principal-box-001.html contain-paint-ignored-cases-no-principal-box-001-ref.html > +== contain-layout-ignored-cases-no-principal-box-002.html contain-layout-ignored-cases-no-principal-box-002-ref.html > +== contain-layout-ignored-cases-no-principal-box-003.html contain-layout-ignored-cases-no-principal-box-003-ref.html Nit: looks like this isn't quite inserting in the right order (there's a contain-size test interspersed in the middle of the contain-layout tests). Relatedly: you should also pull from autoland [to get your final landed ink-overflow patch from Bug 1476495] and rebase your changes on top of that, to be sure this patch [and particularly its reftest.list tweaks] will apply cleanly. I don't see that bug's reftest.list lines (for e.g. "contain-layout-overflow-001.html") shown in the reftest.list patch-context here, so I suspect this won't land cleanly right now until you've done that rebasing.

Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9a6728f46269 Establish stacking context, containing block, independent formatting context for contain:layout. r=dbaron

web-platform-tests PR at https://github.com/web-platform-tests/wpt/pull/12418

Marking this as qe-verify -, per comment 48.