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-anchor] anchor-name should not leak out of a shadow tree #7916

Open
lilles opened this issue Oct 19, 2022 · 26 comments
Open

[css-anchor] anchor-name should not leak out of a shadow tree #7916

lilles opened this issue Oct 19, 2022 · 26 comments

Comments

@lilles
Copy link
Member

lilles commented Oct 19, 2022

The spec currently says: "find the first element el in tree order". It should be more specific about which tree.

The current implementation in Chromium looks walks the box tree, essentially the flat tree, which means it is leaking anchor-names out of shadow trees.

@lilles lilles changed the title [css-anchor-position] anchor-name should not leak out of a shadow tree [css-anchor] anchor-name should not leak out of a shadow tree Oct 19, 2022
@lilles
Copy link
Member Author

lilles commented Oct 19, 2022

I guess tree scoped names/references could be used here?

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 19, 2022
The current implementation leaks anchor-name out of shadow trees. That
is probably not what we want.

See w3c/csswg-drafts#7916

Change-Id: I2f79f68b5595749a6802b753395cb7ee535c5778
@tabatkins
Copy link
Member

Hm, yeah that needs clarification. Scoping to the document tree is indeed what you want, so the anchor-name is a tree-scoped reference. Otherwise two components would have to defensively uniquify their names to make sure they didn't overlap, which sucks.

tabatkins added a commit that referenced this issue Oct 19, 2022
@tabatkins
Copy link
Member

Done, please let me know if there's anything else that you think needs to be said here.

@lilles
Copy link
Member Author

lilles commented Oct 20, 2022

Tree-scoped names generally capture the tree-scope of the element where the property is specified, so if it's inherited it is still associated with the tree-scope of the element it is ultimately inherited from. The new spec text says it compares the tree-scopes of the elements the computed value is on, which is slightly different. Is that intentional? That is, what should happen if an anchor-name is explicitly inherit through a shadow root?

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 20, 2022
The current implementation leaks anchor-name out of shadow trees. That
is probably not what we want.

See w3c/csswg-drafts#7916

Change-Id: I2f79f68b5595749a6802b753395cb7ee535c5778
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 20, 2022
The current implementation leaks anchor-name out of shadow trees. That
is probably not what we want.

See w3c/csswg-drafts#7916

Change-Id: I2f79f68b5595749a6802b753395cb7ee535c5778
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 20, 2022
The current implementation leaks anchor-name out of shadow trees. That
is probably not what we want.

See w3c/csswg-drafts#7916

Change-Id: I2f79f68b5595749a6802b753395cb7ee535c5778
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 20, 2022
The current implementation leaks anchor-name out of shadow trees. That
is not according to spec.

See also w3c/csswg-drafts#7916

Bug: 1376917
Change-Id: I2f79f68b5595749a6802b753395cb7ee535c5778
@tabatkins
Copy link
Member

Ah, hm, you're right. My algo is wrong, it should be allowed for light dom to set ::part(foo) { position: absolute; top: anchor(--target top); }, with anchor-name: --target; set in the light dom (and without fear of accidentally hitting an anchor-name: --target in the shadow). I'll need to tweak things a little to rely on the tree-scoped bit more explicitly.

@tabatkins
Copy link
Member

In other words, I need to check that the tree-scoped reference and the tree-scoped name are scoped to the same tree, not that the querying element and target element are in the same tree.

aarongable pushed a commit to chromium/chromium that referenced this issue Oct 20, 2022
The current implementation leaks anchor-name out of shadow trees. That
is not according to spec.

See also w3c/csswg-drafts#7916

Bug: 1376917
Change-Id: I2f79f68b5595749a6802b753395cb7ee535c5778
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3966354
Reviewed-by: Xiaocheng Hu <[email protected]>
Commit-Queue: Rune Lillesveen <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1061842}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 20, 2022
The current implementation leaks anchor-name out of shadow trees. That
is not according to spec.

See also w3c/csswg-drafts#7916

Bug: 1376917
Change-Id: I2f79f68b5595749a6802b753395cb7ee535c5778
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3966354
Reviewed-by: Xiaocheng Hu <[email protected]>
Commit-Queue: Rune Lillesveen <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1061842}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 20, 2022
The current implementation leaks anchor-name out of shadow trees. That
is not according to spec.

See also w3c/csswg-drafts#7916

Bug: 1376917
Change-Id: I2f79f68b5595749a6802b753395cb7ee535c5778
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3966354
Reviewed-by: Xiaocheng Hu <[email protected]>
Commit-Queue: Rune Lillesveen <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1061842}
@tabatkins
Copy link
Member

Hm, I think it's still wrong actually. References are meant to search their own tree first, then search their host tree if they didn't find anything, etc, so an anchor(--foo) in a shadow tree should be able to find an anchor-name: --foo in the light DOM (but not in reverse). See the last paragraph of the tree-scoped references section.

So the currently specced behavior is just a little too restrictive. I probably need to define a callable algo in Scoping that makes this Just Work Correctly, considering I wrote the Scoping spec and still did this wrong.

@xiaochengh
Copy link
Contributor

So the currently specced behavior is just a little too restrictive

Do you mean the currently specced behavior is the expected behavior or not?

I think the current behavior is already current, and we just need to restrict the current tree-scoped reference spec's inheritance behavior to names defined by at-rules only.

My understanding of that spec is: it was designed with only at-rules in mind. In particular, the "inheritance" behavior makes sense to me only for at-rules. For other names, it seems better to just restrict them to the same tree scope.

(Btw, I think we have the same issue for counter names)

@tabatkins
Copy link
Member

Do you mean the currently specced behavior is the expected behavior or not?

Not. Per the Scoping spec, we should expect anchor(--foo ...) in a shadow tree to be capable of seeing anchor-name: --foo in the light tree, if nothing in the shadow tree defines that name already.

My understanding of that spec is: it was designed with only at-rules in mind. In particular, the "inheritance" behavior makes sense to me only for at-rules. For other names, it seems better to just restrict them to the same tree scope.

It was def written with at-rules foremost in mind, but I think the reasoning for the "inheritance" still makes decent sense - it means components can anchor to things outside of themselves, but without polluting namespaces that don't expect it.

Like, it'd probably be fine if we ended up restricting tree-scoped names created by properties rather than at-rules to be same-tree only, but I don't think there's a good reason to do so from a theoretical standpoint.

@xiaochengh
Copy link
Contributor

but I think the reasoning for the "inheritance" still makes decent sense - it means components can anchor to things outside of themselves, but without polluting namespaces that don't expect it.

Agreed that this should be achieved, but inheritance is not needed for this purpose. We can define anchor names in :host and ::part rules.

Do we have cases where :host and ::part don't suffice, or other scenarios that require inheritance?

If not, I think inheritance is doing more risk than good as it breaks shadow DOM encapsulation, and it's complicated to implement without good use cases.

@lilles
Copy link
Member Author

lilles commented Oct 21, 2022

I find it a bit hard to wrap my head around the inheritance part since both ends can be inherited across trees. You can explicitly inherit an anchor-name from a slot or via a shadow root, but you could also inherit the inset property anchor() function across a shadow boundary.

@tabatkins
Copy link
Member

Yup, just remember that the important part is that we care about the trees that the styles come from, not what trees the elements are in that actually use the styles.

Then the "name inheritance" is a convenience feature, intentionally leaking names defined in one tree into descendant trees, taking metaphorical advantage of the existing info-leak that inheritance represents.

If not, I think inheritance is doing more risk than good as it breaks shadow DOM encapsulation, and it's complicated to implement without good use cases.

If it's a significant complication I'm okay with dropping it and leaving the spec as is (and modifying the advice in Scoping to match up). The cases are reasonably minimal, yeah.

@lilles
Copy link
Member Author

lilles commented Oct 21, 2022

This is similar to container-name for container queries, right? We don't make that one tree scoped? If you inherit container-name explicitly from a slot to a slotted element, we still find that container with a query in the slotted element's tree.

@xiaochengh
Copy link
Contributor

From my experience dealing with @keyframes, @counter-style and @font-face, name inheritance is quite complicated to support.

I don't really see it as a convenience feature, but a workaround for backward-compatibility since browser support for tree-scoped names (for existing at-rules) have been always bad/none, and people have been relying on global at-rules. Also there are some other new features that explicitly reject inheritance for better encapsulation, like scoped custom element registries.

I'll +1 to leaving this spec as-is and modify Scoping instead.

@tabatkins
Copy link
Member

If you inherit container-name explicitly from a slot to a slotted element, we still find that container with a query in the slotted element's tree.

What happens if you set container-name on a ::part()? (Given the way CQ and container-name interact, I think the answer should be "literally nothing happens".

I'll +1 to leaving this spec as-is and modify Scoping instead.

Your arguments are pretty reasonable. K, I'll leave the spec as-is and modify the advice in Scoping a bit.

@lilles
Copy link
Member Author

lilles commented Oct 24, 2022

If you inherit container-name explicitly from a slot to a slotted element, we still find that container with a query in the slotted element's tree.

What happens if you set container-name on a ::part()? (Given the way CQ and container-name interact, I think the answer should be "literally nothing happens".

I don't understand what you mean. Do you have an example?

@lilles
Copy link
Member Author

lilles commented Oct 24, 2022

So, my point about container queries is that the container queries spec is not making container-name a tree scoped name which means the text below is red (in both Safari and Chrome - I've checked Safari by using a non-declarative shadow dom):

<!doctype html>
<style>
  #slotted {
    width: 200px;
    container-type: inline-size;
  }
  @container shadow (width = 200px) {
    span { color: red; }
  }
</style>
<div id="host">
  <template shadowroot="open">
    <style>
      ::slotted(#slotted) {
        container-name: shadow;
      }
    </style>
    <slot></slot>
  </template>
  <div id="slotted">
    <span>Red?</span>
  </div>
</div>

I guess this can be seen as a leak and an argument that container-name should be a tree-scoped name.

My point is that anchor-name and container-name are so similar they should behave the same?

@tabatkins
Copy link
Member

My example would be something like, uh:

<div id="host">
  <template shadowroot="open">
    <div part=foo>
      <div id=child part=bar></div>
    </div>
    <style>
      :host {
        container-name: shadow;
        container-type: inline-size;
        width: 200px;
      }
      #child { color: red; }
      @container shadow (width = 200px) {
        #child { color: green; }
      }
    </style>
  </template>
</div>
<style>
#host::part(foo) {
  container-name: shadow;
  container-type: inline-size:
  width: 150px;
}
</style>

That is, the shadow establishes some CQ container and queries it, but a light-DOM stylesheet sets the same container name/type on a different element. Does this work, intercepting the CQ and making it evaluate false?

Then if the outer page does

@container shadow (width = 150px) {
  #host::part(bar) { color: blue; }
}

does it match?

I think the most reasonable answer is that both CQs should be true, referring to different container elements, because the @container foo is a tree-scoped reference and container-name is a tree-scoped name, so the CQ inside the shadow resolves against the host element, while the one in the light resolves against the part=foo element. Anything else exposes internal shadow details.

(Letting the light-dom CQ resolve to anything inside the shadow also reveals internal details, namely that part=foo is an ancestor of part=bar, but I think that's somewhat unavoidable.)

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 7, 2022
…ame across trees, a=testonly

Automatic update from web-platform-tests
[css-anchor] Add some tests for anchor-name across trees

The current implementation leaks anchor-name out of shadow trees. That
is not according to spec.

See also w3c/csswg-drafts#7916

Bug: 1376917
Change-Id: I2f79f68b5595749a6802b753395cb7ee535c5778
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3966354
Reviewed-by: Xiaocheng Hu <[email protected]>
Commit-Queue: Rune Lillesveen <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1061842}

--

wpt-commits: f249b439de786da11083591131a5bd6729740e11
wpt-pr: 36551
@lilles
Copy link
Member Author

lilles commented Nov 8, 2022

My example would be something like, uh:

<div id="host">
  <template shadowroot="open">
    <div part=foo>
      <div id=child part=bar></div>
    </div>
    <style>
      :host {
        container-name: shadow;
        container-type: inline-size;
        width: 200px;
      }
      #child { color: red; }
      @container shadow (width = 200px) {
        #child { color: green; }
      }
    </style>
  </template>
</div>
<style>
#host::part(foo) {
  container-name: shadow;
  container-type: inline-size:
  width: 150px;
}
</style>

That is, the shadow establishes some CQ container and queries it, but a light-DOM stylesheet sets the same container name/type on a different element. Does this work, intercepting the CQ and making it evaluate false?

Yes, the part rule will make this case go red because we don't use tree-scoped names for container-name because the spec doesn't say anything about it, we just look up shadow-including ancestors of the element being matched.

Then if the outer page does

@container shadow (width = 150px) {
  #host::part(bar) { color: blue; }
}

does it match?

Yes, the container queries try to match the same container in both cases.

I think the most reasonable answer is that both CQs should be true, referring to different container elements, because the @container foo is a tree-scoped reference and container-name is a tree-scoped name, so the CQ inside the shadow resolves against the host element, while the one in the light resolves against the part=foo element. Anything else exposes internal shadow details.

That might be the best behavior, but the spec does not mention container-name as a tree-scoped name at all.

(Letting the light-dom CQ resolve to anything inside the shadow also reveals internal details, namely that part=foo is an ancestor of part=bar, but I think that's somewhat unavoidable.)

We should open an issue against the css-contain spec for this.

@lilles
Copy link
Member Author

lilles commented Nov 8, 2022

What makes this super complex in the case of anchor-name and container-name compared to at-rules like keyframes is that we have values both on the name and reference side which can be associated with a different tree scope than the scope of the element that the computed value is associated with.

@lilles
Copy link
Member Author

lilles commented Nov 8, 2022

Hm, so given that css-scoping-1 says names/references must be tree-scoped names, perhaps the anchor positioning and container queries specs don't strictly need to say anything about it.

@lilles
Copy link
Member Author

lilles commented Nov 8, 2022

I'm utterly confused about the container queries case and need to go back to study it. There is the pseudo element thing affecting ::part() that I didn't think about in my comments above: #5984 (comment)

jamienicol pushed a commit to jamienicol/gecko that referenced this issue Nov 11, 2022
…ame across trees, a=testonly

Automatic update from web-platform-tests
[css-anchor] Add some tests for anchor-name across trees

The current implementation leaks anchor-name out of shadow trees. That
is not according to spec.

See also w3c/csswg-drafts#7916

Bug: 1376917
Change-Id: I2f79f68b5595749a6802b753395cb7ee535c5778
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3966354
Reviewed-by: Xiaocheng Hu <[email protected]>
Commit-Queue: Rune Lillesveen <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1061842}

--

wpt-commits: f249b439de786da11083591131a5bd6729740e11
wpt-pr: 36551
@andruud
Copy link
Member

andruud commented Nov 28, 2022

Hm, so given that css-scoping-1 says names/references must be tree-scoped names, perhaps the anchor positioning and container queries specs don't strictly need to say anything about it.

Some spec, somewhere, needs clarification, because the current text in css-scoping-1 can't easily be applied to name-defining things that aren't "global" .

@tabatkins seems to have agreed with that earlier:

Container names don't need to be tree-scoped; they're not globally registered/visible, but are instead solely looked up via an ancestor search.

If we've changed our mind on that, should also change the spec. :-)

@tabatkins
Copy link
Member

Ah, thanks for reminding me about #5984!

So, argh, this is tough. In 5984 we decided on using a shadow-including ancestor search, and to jump straight to a pseudo's originating element when starting that search. This means that a ::part() in a CQ won't see any containers on elements in the shadow (which is good); instead it'll start looking at the host element. (The ::part() will be able to see a container name set on the host element from within the shadow, tho, so there's still some shadow-leakage right at the boundary. But boundary leakage on the host is unavoidable so it's okay.) (And you can set a container-name using ::part(), which'll be visible to CQs inside the shadow, so there's leakage the other way too. But since light-DOM styles can't use a container-name set on a ::part(), there's no reason to ever set that property except to purposely screw with a shadow, so that's probably okay.)

My previous example's behavior under the 5984 decision

So in my example, the `part=foo` element in the shadow *does* get a `container-name` from the light DOM styles, which is visible to the CQ in the shadow (possibly inadvertently). This causes the CQ in the shadow to fail, since the nearest "shadow" container for #child is 150px wide, not 200px. The subsequent light-DOM CQ *also* fails, since it starts searching at the host element, not the part in the shadow tree, and thus sees a "shadow" container that is 200px wide, not 150px.

But this worked because we're doing this stuff at selector-resolution time, where we know that we're resolving a pseudo-element reference. Is it still possible to do when the search is initiated due to a property value rather than a selector? That's well after we're done with Selectors.

@tabatkins tabatkins reopened this Nov 29, 2022
@andruud
Copy link
Member

andruud commented Nov 30, 2022

The ::part() will be able to see a container name set on the host element from within the shadow, tho, so there's still some shadow-leakage right at the boundary. But boundary leakage on the host is unavoidable so it's okay.

You are probably saying it's unavoidable more generally, but at least for the specific case you mention there, that is avoidable if container names were tree-scoped?

But this worked because we're doing this stuff at selector-resolution time, where we know that we're resolving a pseudo-element reference. Is it still possible to do when the search is initiated due to a property value rather than a selector? That's well after we're done with Selectors.

I think you're right. The fact that a value "originates" from a pseudo-element-rule is long gone by the time we're ready to apply the effects of that value. We'd have to invent some new concept which would retain that fact. And at that point it seems preferable to re-use tree-scoped references.

Not that I love tree-scoped references so much (they're expensive: we have to hold an entire TreeScope pointer next to each name, and lookup of things generally becomes more complicated), but trying to apply the 5984 CQ behavior to situations that can't be resolved selector-matching-time (e.g. anchor names, timeline names) seems like a worse alternative in practice.

@tabatkins
Copy link
Member

that is avoidable if container names were tree-scoped?

Right, we could additionally add tree-scoping and avoid some additional leakage. But also some amount of boundary leakage is still unavoidable, since if both light and shadow set a name on the host one of them will win and cause the other to lose. ^_^ Tree-scoped would prevent ::part(foo) { container-name: foo; } from messing with the shadow content, too, tho as I note there's no use-case for doing so right now except messing with the shadow so that's probably not very important.

If they were tree-scoped, my example would result in the CQ in the shadow succeeding (it would skip the container-name set from the light and find the expected CQ on the host) and the CQ in the light failing (it starts its search for container names on the host, and fails to find any container names at all).

I think you're right. The fact that a value "originates" from a pseudo-element-rule is long gone by the time we're ready to apply the effects of that value. We'd have to invent some new concept which would retain that fact. And at that point it seems preferable to re-use tree-scoped references.

Ok, thanks for confirming my intuition. I'll get something written more explicitly in Scoping defining the behavior of tree-scoped names established by properties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants