Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-contain] Is it ok that contain:layout is a CB for fixpos/abspos descendants but doesn't establish a stacking context? #2942

Closed
dbaron opened this issue Jul 20, 2018 · 16 comments

Comments

@dbaron
Copy link
Member

dbaron commented Jul 20, 2018

The containment spec for contain:layout:

  • says that contain:layout establishes a containing block for absolutely positioned and fixed positioned descendants, but
  • does not say that it establishes a stacking context.

This is the first feature in CSS that I'm aware of with this combination. I think it's worth some consideration of whether this combination is sensible. Do the painting rules for abspos and fixpos elements contained by such an element work in a reasonable way?

/cc @MReschenberg

@tabatkins
Copy link
Member

The combination wasn't on purpose; I just didn't realize that the two were tied together like that.

@dbaron
Copy link
Member Author

dbaron commented Jul 20, 2018

I'm not sure how tied together they are -- but if we're going to keep it this way, somebody needs to think it through.

@dholbert
Copy link
Member

dholbert commented Jul 20, 2018

RE how tied together they are:

Right now, the CSS painting order algorithm has three different mentions of a special category which gets special painting behavior at different parts of the algorithm:

positioned descendants and descendants which actually create a new stacking context

Fun fact: up until now, this category has basically been 100% equivalent to the set of all elements that establish a containing block for abs-pos descendants. (Please correct me if I'm wrong, but I'm pretty sure these are 100% equivalent.)
[EDIT: Actually, per @chrishtr's correction below, I think this category (is-positioned-or-forms-stacking-context) is a superset of the set of elements that establish a CB for abs pos descendants. opacity is one example of something in the former category but not the latter.]

However -- with the current contain:layout spec-text, these two categories will now be slightly disjoint. Specifically, contain:layout elements will be in the latter category (they establish a containing block for abs-pos descendants) but they might not be in the former category (they're not necessarily positioned themselves, nor do they create a stacking context).

So I think the questions here are:

  • In the CSS Painting Order algorithm, would we want contain:layout elements to be included or excluded from the pieces about "positioned descendants and descendants which actually create a new stacking context"? (And to what extent does this matter?)
  • Does this matter enough to create this new special-case (things which are abpsos containing blocks but are not themselves positioned nor do they form stacking contexts)?
@dbaron
Copy link
Member Author

dbaron commented Jul 21, 2018

And @dholbert wrote comments 13 through 16 of bug 1472919 that show that Gecko code currently assumes this combination doesn't exist, and behaves incorrectly as a result.

@frivoal
Copy link
Collaborator

frivoal commented Jul 21, 2018

As we're not trying to introduce any new complicated thing here, I'd be perfectly fine with creating a new stacking context as well even if it isn't strictly necessary, if that makes things simpler, which seems to be the case in Gecko.

Is that OK for blink? @eaenet @mrego

@chrishtr chrishtr added this to Backlog in High-priority issues via automation Jul 25, 2018
@chrishtr
Copy link
Contributor

positioned descendants and descendants which actually create a new stacking context

Fun fact: up until now, this category has basically been 100% equivalent to the set of all elements that establish a containing block for abs-pos descendants. (Please correct me if I'm wrong, but I'm pretty sure these are 100% equivalent.)

It's not true that containing block, positioning, and stacking context imply each other. Some of the stacking context triggers do not imply containing block for abs pos. Opacity is one example.

However, I think what you might have meant to say is that the set of element which are treated in the special category by the painting algorithm is positioned elements + stacking contexts, and that this is a good invariant to keep in order to avoid code complexity in the engines and reduce developer confusion. This is also what @dbaron proposed resolving in the first bullet of this comment: #2717 (comment). I added some notes in the end of that other issue regarding various kinds of replaced content that don't fit this model.

However, I think that making contain:layout a stacking context would be a good change, since I agree
that it would simplify implementations and be less confusing for developers. Let's resolve this, and
I will try to make the change happen in Chromium.

@dholbert
Copy link
Member

dholbert commented Jul 26, 2018

Some of the stacking context triggers do not imply containing block for abs pos. Opacity is one example.

Ah, thank you - that's a good counterexample. So the two groups aren't 100% equivalent as I'd initially stated, but rather: one group is a subset of the other. Let me restate my observed "rule" more carefully, with that in mind:

[Observed rule]: Up until now, if an element forms a containing block for abs pos descendants, then that always implies it is in this special "is positioned or forms stacking context" category that's referenced several times in the painting spec. (And there are a few other things in that category, too, like "opacity" as you noted.)

contain:layout would be the first (I think?) exception to this "rule" -- it forms an abs pos containing block, but it's not in the special "is positioned or forms stacking context" category.

However, I think that making contain:layout a stacking context would be a good change

Great! I'm glad we're in agreement. Thanks for chiming in.

@mrego
Copy link
Member

mrego commented Jul 26, 2018

I agree with the idea and I think it should be fine to implement it in Chromium.

BTW, I realized that despite paint containment is creating a stacking context in Chromium, it's doing it more than expected even for elements where paint containment doesn't apply. See https://crbug.com/868102

@frivoal
Copy link
Collaborator

frivoal commented Jul 28, 2018

Made a PR for this (#2962). Standing ready to merge if we resolve in favor.

@css-meeting-bot
Copy link
Member

The Working Group just discussed Is it ok that contain:layout is a CB for fixpos/abspos descendants but doesn't establish a stacking context?, and agreed to the following:

  • RESOLVED: Accept the PR that corrects this oversite
The full IRC log of that discussion <dael> Topic: Is it ok that contain:layout is a CB for fixpos/abspos descendants but doesn't establish a stacking context?
<dael> github: https://github.com//issues/2942
<dael> florian: contain layout sets the element to be a containing block for lots of things, but didn't say it establishes stacking. This would be a first time we would have something liek this that's not stacking, so let's do it
<dael> TabAtkins: And it wasn't intentional, accidental tying concepts and didn't realize
<dael> astearns: Not seeing obj in issue
<dael> astearns: Obj?
<dael> RESOLVED: Accept the PR that corrects this oversite
@dbaron
Copy link
Member Author

dbaron commented Aug 2, 2018

I updated my big stacking context test to reflect this resolution.

@frivoal
Copy link
Collaborator

frivoal commented Aug 2, 2018

@dbaron is this something you expect to extract individual test cases from and submit to wpt?

@chrishtr
Copy link
Contributor

chrishtr commented Aug 2, 2018

Yes we need testcases submitted to WPT, combined with changes to implementations to adopt
this resolution. I can get that done for Chrome. Can Gecko please implement in parallel?

@MReschenberg
Copy link

@mrego
Copy link
Member

mrego commented Aug 2, 2018

I have these tests from my experiments in Chromium: web-platform-tests/wpt#12269

@frivoal
Copy link
Collaborator

frivoal commented Sep 6, 2018

test cases available in wpt

aarongable pushed a commit to chromium/chromium that referenced this issue Sep 7, 2018
The CSSWG has recently resolved that layout containment
should create a stacking context like paint containment
(see issue w3c/csswg-drafts#2942).

This is the spec text about this
(https://drafts.csswg.org/css-contain/#containment-layout):
  "5. The element creates a stacking context."

The patch is just a simple change in
ComputedStyle::UpdateIsStackingContext() to add the new condition.

Apart from that TestExpectations file is updated
as we're now passing some tests from WPT.
We're also failing one test, but for an unrelated issue
(see bug #880802).

BUG=870157
TEST=external/wpt/css/css-contain/contain-layout-016.html
TEST=external/wpt/css/css-contain/contain-layout-018.html
TEST=external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/contain/contain-layout-stacking-context-001.html

Change-Id: Icdc102692801534364adf3fc0929192b886cdbc4
Reviewed-on: https://chromium-review.googlesource.com/1211922
Commit-Queue: Manuel Rego <rego@igalia.com>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589689}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment