w3c / csswg-drafts

CSS Working Group Editor Drafts
https://drafts.csswg.org/
Other
4.48k stars 661 forks source link

[css-contain] Style containment for counters #9212

Open nt1m opened 1 year ago

nt1m commented 1 year ago

While looking at: https://github.com/web-platform-tests/wpt/blob/efde03a7b7/css/css-contain/counter-scoping-004.html

It seemed to me that this WPT was incorrect, and that Blink & WebKit's behaviors were correct here.

Since ::before (where we read the counter) is inside the style containment tree (it's a tree abiding pseudo element), counter-increment on the scoping element shouldn't have any effect on the counter inside tree.

I believe that is the intention of the spec as well:

https://drafts.csswg.org/css-contain-2/#property-scoped-to-a-sub-tree

If scoped to a sub-tree, it’s the same, except the scoping element itself is counted as "outside" the tree [...] When considering the effects of the scoped property on elements inside the subtree, the element at the base of the subtree is treated as if it was the root of the document.

To me that essentially means we take all the elements inside the subtree excluding the scoping element, and put that in its own document. Meaning the counter should always be "0" where we read the counter, since the counter is never incremented (since the element that increments it is outside of that "document").

@emilio Can you describe what your interpretation was?

Loirooriol commented 1 year ago

https://drafts.csswg.org/css-contain-2/#example-6932a400

the counter() and counters() value of the content property is not itself scoped, and can refer to counters established outside of the subtree

So the ::before can see the outer instance of the counter instead of 0.

That said, there are nested .list-item children that will use counter-increment and thus create a new instance of the counter. Where does this happen? See discussion in #5175

The example in the spec says

As counter-increment is scoped to an element’s subtree, the first use of it within the subtree acts as if the named counter were set to 0 at the scoping element

So with that interpretation the ::before of the .list-item wrapper should see 0 indeed.

[0] A1
   [1] B1
   [2] B2
[2] A2
[3] A3

However I think that's wrong, the new counter should be instantiated on the element that tries to modify it (which is what all browsers do in simple cases).

So then I agree with Firefox:

[1] A1
   [1] B1
   [2] B2
[2] A2
[3] A3

In fact, when using counters(), Blink behaves like Firefox

[1] A1
   [1.1] B1
   [1.2] B2
[2] A2
[3] A3
danielsakhapov commented 1 year ago

Agree with:

However I think that's wrong, the new counter should be instantiated on the element that tries to modify it (which is what all browsers do in simple cases).

New Blink implementation behaves like Firefox in this case for counter().

danielsakhapov commented 1 year ago

Also, these two tests need spec clarification:

https://github.com/web-platform-tests/wpt/blob/master/css/css-contain/contain-style-counters-004.html and https://github.com/web-platform-tests/wpt/blob/master/css/css-contain/contain-style-counters-005.html

for the 004 the question is - why we don't have 13 as the result, as it doesn't seem different from the spec example: https://drafts.csswg.org/css-contain-2/#example-6932a400. (btw, removing <span>s will produce 13 in Firefox, but they should not change the result, since we still can leave the style containment scope, even the s are there? Or it has to do something with the first counter-increment acting like it was set to 0 on scope element?).

for the 005 the question is - since we have counters(), shouldn't it access counters established outside the style containment scope? E.g. the first div with class increment will create a counter, that is inherited by the first div with class contain. Inside this style containment div the first div with class increment will create another counter. So, when later using counters() on ::before of that div, shouldn't counters also leave the style containment scope and produce 11 as a result? Basically, it's the same as the same example https://drafts.csswg.org/css-contain-2/#example-6932a400?

Loirooriol commented 1 year ago

I do think 004 should produce 13, which is what I get on Blink if I use counters(counter-of-span, ".").

And also agree for 005, on Blink I get both counters if the outer instance is instantiated on the <body>. I don't think that instantiating it in a previous sibling instead of the parent should affect the outcome.

fantasai commented 1 year ago

To me that essentially means we take all the elements inside the subtree excluding the scoping element, and put that in its own document. Meaning the counter should always be "0" where we read the counter, since the counter is never incremented (since the element that increments it is outside of that "document").

From an authoring perspective, this mental model makes sense to me...

But I think this gets to the fundamental question of, what is style containment? Does it only prevent content within the contained subtree from affecting content outside it, or does it also prevent styling from outside the subtree from affecting content inside it?

For example, what happens with timeline names? If an ancestor of the contained element declares a named timeline, can content inside the subtree reference it? Counters should be consistent with that.

What are the use cases for style containment, and which mental model are we targetting here?

Loirooriol commented 1 year ago

See prior discussion in #2483. It was decided that the model would be: contents inside containment can read counters from outside of the containment, but can't modify them (attempting to do so instantiates a new counter).

Does it only prevent content within the contained subtree from affecting content outside it, or does it also prevent styling from outside the subtree from affecting content inside it?

The former. @tabatkins said "the read doesn't cause alteration so it should work as normal". I think this is also consistent with e.g. size containment, which prevents the contents from affecting the size of the element, but not the other way round.

If an ancestor of the contained element declares a named timeline, can content inside the subtree reference it?

I would guess so, for the same reason

What are the use cases for style containment

Container queries, see #6213.

css-meeting-bot commented 1 year ago

The CSS Working Group just discussed [css-contain] Style containment for counters, and agreed to the following:

The full IRC log of that discussion <emeyer> astearns: I assume the resolution we want here is “yes”?
<emeyer> ntim: Right now, I found style container behavior for counters to be quite unintuitive
<emeyer> …Spec says you should scope counter-increement to the subtree that is contained
<florian> q?
<emeyer> …Further effects are unclear
<emeyer> fantasai: At a higher level, it brings up the question of what style containers are useful for
<emeyer> …There’s two ways to look at it
<florian> q+
<oriol> q+
<emeyer> …One is that style containment prevents stuff inside a subtree from affecting stuff outside it
<emeyer> …Another is that the protection goes both ways, so that also stuff outside can’t affect things inside the subtree
<emeyer> …I think the two-way is a little easier for authors to understand
<vmpstr> q+
<emeyer> …I’m not sure if there were good reasons to have one-way containment or not
<astearns> ack oriol
<emeyer> oriol: This was discussed in 2018 in Berlin; then the idea was that we should allow content inside containment shoudl not read values from the outside
<emeyer> …I think this is reasonable; with things like size containment, we prevent the size of the element from depending on (missed)
<emeyer> …Having containment work both ways doesn’t seem needed for container queries
<emeyer> …I guess it’s reasonable for an author to use a counter in the headers of different sections, and maybe they want a section to be a container query
<emeyer> …I think it would be strange to no longer be able to use those as counters
<emeyer> …I think the current way this is specified may be better
<emeyer> …If people want to have the containment work both ways, that’s also a posisbility, but maybe too restrictive
<emeyer> TabAtkins: I don’t have a strong opinion, but Oriol’s summary of my reasoning is correct
<miriam> +1 I don't like making cq-required containment *more invasive* than necessary
<ntim> q+
<astearns> ack florian
<emeyer> florian: Containment in general is meant to be one way
<emeyer> …So, not designed to deliberately isolate parts of the document
<emeyer> …Updating a subtree shouldn’t update the whole page, was the goal
<astearns> ack vmpstr
<emeyer> …As Oriol said, if you aren’t using counters, it makes sense for them to update into the subtree
<emeyer> vmpstr: Content-visibilty: auto allows to skip styling and rendering updates if not needed
<emeyer> …I would like this to remain a one-way barrier
<astearns> ack ntim
<emeyer> ntim: I could go either way, but the one-way containment is harder to implement than two-way
<florian> s/ shouldn’t update the whole page/ shouldn’t dirty the whole page
<emeyer> …What’s important is use cases: if we were to expand style containment beyond making container queries work, if we were to use for names of anchor position, which behavior would make more sense?
<emeyer> astearns: Oriol, you mentioned if we go with one-way containment, the spec would need changes to make that more clear?
<emeyer> oriol: Yes, right now when counters are modified, they create a new instance of the counter
<bramus> present-
<emeyer> …The question is where the counter get instantiated
<emeyer> …Browsers make the new instance in the element that tries to modify the counter, which I agree with
<emeyer> …In complex cases, browsers have several bugs, but in simple cases they all agree that new instances are created
<emeyer> TabAtkins: The example I wrote is just wrong
<argyle> 🧠💨
<emeyer> astearns: I hear a slight preference for one-way containment
<emeyer> fantasai: I think Oriol was fairly convincing
<florian> my pref is rather strong…
<emeyer> astearns: Tim, would you be okay despite the implementation difficulty?
<dbaron> (I suppose the other option is that the inside of the container essentially operates on a clone of the counter state as of its start.)
<emeyer> ntim: I guess it’s okay
<fantasai> dbaron, I think that would be more confusing
<emeyer> astearns: So this is a “no change to spec, fix example” situation; are there tests that need to be updated?
<chrishtr> I think one-way is better for authors
<emeyer> TabAtkins: I need to check them either way and update if necessary
<emeyer> ntim: I would want the spec to be more clear
<emeyer> …Right now it says the scoped property is as if scoped to its own document
<emeyer> …It’s unclear the extent of the effects of the property
<emeyer> …If it’s like you isolate the counters in their own document, they would all be zero
<sakhapov_> q+
<emeyer> TabAtkins: The counter-* properties are scoped, but the counter() function is not scoped
<emeyer> …Using the counter() function does not interact with style scoping in any way
<emeyer> …So it should not be zeroed just because an ancestor was style-scoped
<emeyer> astearns: Explicitly saying in normative text that counter() is not affected, then
<emeyer> florian: The spec says what’s affected, and counter() is not on the list
<emeyer> astearns: Fair
<astearns> ack sakhapov_
<emeyer> sakhapov_: I think the problem is counter-increment acts as if the named counter is set to zero
<emeyer> TabAtkins: Yeah, the example is wrong and I need to fix that
<emeyer> sakhapov_: But the HTML code is correct, so maybe move this
<astearns> ack dbaron
<emeyer> dbaron: I want to mention a slightly different model for one-way containment, which is essentially you could do the containment by acting as though the counter state gets cloned into the contained subtree
<emeyer> …That way, an increment inside the subtree would happen in ways that wouldn’t leak back out
<astearns> ack fantasai
<emeyer> fantasai: I thought that was the original proposal and it’s terribly confusing, let’s not do that
<emeyer> …I like the suggestion that you instantiate a new counter, so inside the subtree you can do your own thing
<emeyer> astearns: Let’s resolve to fix the example and re-affirm the one-way containment of counters
<emeyer> …Objections?
<emeyer> (silence)
<ntim> florian, TabAtkins: "the effects of the property on that element" is confusing, because counter-increment does affect what's read from the counter() function
<emeyer> RESOLVED: fix the example and re-affirm the one-way containment of counters
<ntim> Hoping the wording can be clarified a bit for that.
<emeyer> astearns: Are we taking David’s suggestion?
<fantasai> s/counters/counters by instantiation of new counters/
<astearns> q?
<emeyer> sakhapov_: I don’t see how different from the current behavior that would be
<emeyer> dbaron: Counter resets would act the same, but incrememnts would be different
<emeyer> …If you were using the counters() function, and you incremented both outside and inside the subtree, the difference is that one would give you 3.1 and the other would give you 4
<emeyer> sakhapov_: Each counter reset creates a new counter?
<emeyer> dbaron: The idea is that the clone operation wouldn’t be deep, you’d only need to clone the most-nested counter
<emeyer> sakhapov_: Do you inherit the reset?
<emeyer> dbaron: Either way, the counters() function would look all the way up?
<emeyer> TabAtkins: David’s proposal means things inside the subtree won’t modify counters outside the subtree
<emeyer> florian: This is maybe more convenient and less confusing
<emeyer> …David’s proposal leads to people asking why counters seem to reset
<emeyer> fantasai: I agree it’s confusing and we shouldn’t do it
<emeyer> astearns: I’m slightly against the cloning proposal from an authoring perspective
<emeyer> …David, if you want to pursue further, you could open an issue
<emeyer> fantasai: I object to going that direction
<emeyer> florian: If we were to consider it, we need to assess compatibility baggage
<emeyer> astearns: We should resolve on whether we need to add a note, and update the WPT
<emeyer> RESOLVED: Add a clarifying note about the coutner function and review the WPT tests
vmpstr commented 1 year ago

I was a little confused about the end of the discussion (and @dbaron's proposal), specifically:

…If you were using the counters() function, and you incremented both outside and inside the subtree, the difference is that one would give you 3.1 and the other would give you 4

I tried with

<style>
div::before {
  counter-increment: foo;
  content: counters(foo, ".");
}
</style>

<div>
  <div style="contain: style"> 
    <div></div>
    <div></div>
  </div>
  <div> 
    <div></div>
    <div></div>
  </div>
</div>

And I see, on Chrome and Firefox:

1
1
2
3
2
3
4

That's what I'd expect. But it seemed like the discussion was about this being something like 1.1 1.2 inside the style containment, or did I misunderstand?

Loirooriol commented 1 year ago

That's probably because implementations are buggy? You can see new counter instances in this simpler case:

<style>
div::before {
  content: counters(foo, ".", decimal);
}
</style>
<div style="counter-increment: foo">
  <div style="contain: style">
    <div style="counter-increment: foo"></div>
  </div>
</div>
1
1
1.1
tabatkins commented 1 year ago

Rendering of Chrome's handling of @vmpstr's code, with a little bit of styling to show off the nesting and containment better: Screenshot 2023-09-28 121434

Yeah, this is just buggy, it reflects neither the current spec nor dbaron's proposal. It looks like it does "two-way" containment, so the stuff inside the containment can't see the outside counters at all.

The current spec should render as:

1
  1.1 (the style-contained element)
    1.2
    1.3
  2
    3
    4

The elements inside the scope can see the foo counter, but aren't allowed to modify it, so they create a nested foo counter instead.

Dbarons' proposal should render as:

1
  2 (the style-contained  element)
    3
    4
  2
    3
    4

The elements inside the scope instead see a copy of the counters coming from the style-scoped element. They're thus allowed to modify the foo counter as normal, but since they're only modifying a copy, the elements outside of the scope don't see their modifications.


And for completeness, without any containment at all it should look like this (and impls do indeed match this):

1
  2 (the element that used to be style-contained)
    3
    4
  5
    6
    7

So we see the differences here. Under both the current spec and dbaron's proposal, the elements following the containment act identically, pretending the contained subtree doesn't exist. In dbaron's proposal, however, the stuff inside the subtree also acts identically to how it worked without containment.

(Edited to fix the numbering since Oriol pointed out the increment was happening on the ::before, not on the div.)

Loirooriol commented 1 year ago

@tabatkins Your expected behavior should actually be for this CSS:

div { counter-increment: foo }
div::before { content: counters(foo, ".") }

However, the testcase was incrementing on the pseudo-element, not the element itself, and the pseudo-element is in the subtree, so I think it should actually produce

1 (body > div::before)
  1.1 (body > div > :first-child::before)
    1.2 (body > div > :first-child > :first-child::before)
    1.3 (body > div > :first-child > :last-child::before)
  2 (body > div > :last-child::before)
    3 (body > div > :last-child > :first-child::before)
    4 (body > div > :last-child > :last-child::before)
tabatkins commented 1 year ago

Ohhhh, I didn't notice the increment was on the ::before. That does indeed change things, yeah, so that explains the first thing I listed as buggy. False alarm, it's correct. I'll edit.

tabatkins commented 1 year ago

Okay, example fixed and made more comprehensive - I just reused the example we're talking about here, but with the counter-increment on the div itself so I could also illustrate the "scoped to subtree" aspect.

danielsakhapov commented 1 year ago

Can you guys have a look at https://github.com/web-platform-tests/wpt/pull/41799 and https://github.com/web-platform-tests/wpt/pull/41820 for a review that it's ok?

Loirooriol commented 1 year ago

Mm, actually, due to #5477, I think the example in the spec should produce

1
  2
    2.1
    2.1
  3
    4
    5

that's because the 2nd item inside the containment will not inherit the nested counter from the previous sibling, because the parent element already has a counter with the same name.

But I already added #5477 to the agenda to revert that resolution, so it may be fine to leave the example as-is?

tabatkins commented 1 year ago

Oooh, I didn't pay proper attention to #5477. Yeah, let's deal with it there.

vmpstr commented 1 year ago

The fact that both Chrome and Firefox render the case I had incorrectly but consistent with each other makes me a little worried about "just fixing the implementation" as it might cause rendering changes on existing sites that rely on this behavior.

frivoal commented 1 year ago

I don't know if this is necessarily the same problem as #5477. Another way to get to 2.1 / 2.2 rather than 2.1 / 2.1 in the example would be to say something like:

The counter-increment and counter-set properties must be scoped to the element’s sub-tree and create a new counter at the root of the scoped sub-tree when a counter with the same name is inherited from outside.

Loirooriol commented 1 year ago

But that's a problem if there is some counters() beforehand, because at first it could just read the outer counter, but if later we create a new instance, then it would be affecting counters() retroactively.

frivoal commented 1 year ago

I'm not sure I'd call that "retroactive", as my perception of the ordering is that first you resolve all the counter-increment, counter-set, counter reset properties everywhere, and then you figure out what the counter() or counter(s) functions give you. But point taken, that behavior would not be overly intuitive.

frivoal commented 4 months ago

State of this issue: the edits we resolved on are in place, but once the edits for #5477 are landed and once #5175 is resolved and edited in, we need to do a round of review all all related tests, and possibly write a few more.