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-3] container-name string vs ident #6405

Closed
frivoal opened this issue Jun 23, 2021 · 12 comments
Closed

[css-contain-3] container-name string vs ident #6405

frivoal opened this issue Jun 23, 2021 · 12 comments

Comments

@frivoal
Copy link
Collaborator

frivoal commented Jun 23, 2021

https://drafts.csswg.org/css-contain-3/#container-name accepts either a string or a custom ident, with identical semantics. We should probably stick to one of the two. Given that this is a name that originates in css and is used in css, I don't think we have a particular need to accomodate for names that don't fit a custom ident, so I'd suggest dropping strings.

@Loirooriol
Copy link
Contributor

For other named at-rules:

  • @color-profile: <dashed-ident> | device-cmyk
  • @counter-style: <custom-ident>
  • @custom-media: <dashed-ident>
  • @custom-selector: <dashed-ident>
  • @font-palette-values: <custom-ident>
  • @keyframes: <custom-ident> | <string>
  • @layer: <ident> [ '.' <ident> ]*
  • @property: <dashed-ident>
  • @scroll-timeline: <custom-ident> or <string>

@mirisuzanne
Copy link
Contributor

By accepting a string, we allow attribute patterns like:

[container-name] {
  container-name: attr(container-name);
}

I don't see that as a deal-breaker, but it does seem nice-to-have, and based on prior art?

@frivoal
Copy link
Collaborator Author

frivoal commented Jun 24, 2021

It's still not implemented anywhere, but using css-values-4 syntax, you could achieve that without needing strings:

[container-name] {
  container-name: attr(container-name ident);
}

@mirisuzanne mirisuzanne moved this from To do to In progress in Container Queries [css-contain] Feb 17, 2022
@mirisuzanne
Copy link
Contributor

I would love to see browsers support the full attr() syntax. Failing that, I like the flexibility our current syntax provides. Either way, this probably needs to be settled, as browsers seem eager to start shipping.

@tabatkins
Copy link
Member

Yeah, I'm fine with keeping it as it is right now (accepting both, with them living in the same namespace). It's not a pattern we'll want to continue for the indefinite future, but nothing is harmed, and some use-cases are helped, by allowing it right now.

@andruud
Copy link
Member

andruud commented Feb 17, 2022

living in the same namespace

That means any string that would match <custom-ident> when "ident-ified" by the computed-value process would need to be disallowed?

@tabatkins
Copy link
Member

I'm not sure what you mean by that. I mean that foo and "foo" should represent the same container name.

@andruud
Copy link
Member

andruud commented Feb 18, 2022

Yeah. No wonder, that didn't make much sense. :-)

that would match <custom-ident>

Should have been "that would NOT match ...".

I'm just saying that as currently specified, container-name: "initial" would compute to container-name: initial. At first I thought that was what you meant with "living in the same namespace". Need to use "Computed value: as specified", or deal with it some other way.

@tabatkins
Copy link
Member

We don't necessarily need to make "initial" invalid, but I think it would be a good idea, yeah. You can't construct a custom-ident with that value (unlike things like "1em" or "foo bar", which can be done with escapes).

@nt1m
Copy link
Member

nt1m commented Feb 19, 2022

I agree supporting both string and ident is not ideal in the long run. It's typically the type of thing that becomes a quirk on the web platform and small maintenance burden for the engine (which accumulate).

For the attr() use case, since vendors currently only implement attr() for ::before/::after atm, I would expect the generalization of attr() to also cover attr(container-name ident).

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed container name - string vs ident, and agreed to the following:

  • RESOLVED: Restrict container-name to just idents
The full IRC log of that discussion <TabAtkins> Topic: container name - string vs ident
<TabAtkins> github: https://github.com//issues/6405
<TabAtkins> miriam: In the current spec we allow container names to be either a string or an ident.
<TabAtkins> miriam: Advantage of string is you can use attr() to take from the element and use it for a container name.
<TabAtkins> miriam: That could also be possible if browsers implement the extensions to attr() that let you specify the type, attr(foo ident)
<TabAtkins> miriam: Florian suggested just accepting ident would be easier
<astearns> ack fantasai
<TabAtkins> miriam: Don't have a strong opinion here. Like the flexibility, but would like browsers to implement attr() stuff
<TabAtkins> fantasai: Currently attr() is not only restricted to strings, but also just to the content property, so it'll require extensions regardless.
<bkardell_> kinda +1 to that
<TabAtkins> fantasai: So I think we can keep it ident-only, and when we extend attr() we can make sure it allows for idents as well.
<bkardell_> q+
<astearns> ack bkardell_
<TabAtkins> astearns: Not sure it follows we'd get attr() in this property and with 'ident' behavior at same time, but I suppose we'd have to either change attr() or change this property to allow strings.
<TabAtkins> bkardell_: +1 to fantasai about starting with ident.
<TabAtkins> bkardell_: I think attr() is a need in lots of use-cases way beyond this, feels bad to make an exception to solve one case.
<TabAtkins> q+
<astearns> ack TabAtkins
<Rossen_> q
<fantasai> TabAtkins: I think I'm the main defender of keeping string, but fantasai makes a good point that attr() isn't allowed anywhere at this point, so we'll fix it when we fix it, and I'm fine with ident for now
<TabAtkins> astearns: So hearing strong arguments for restricting to ident. Anyone wanna argue for strings?
<TabAtkins> astearns: Proposed resolution is to restrict to idents. We'll see if we need to add strings later, or if attr() is sufficient.
<bkardell_> s/ feels bad to make an exception to solve one case/ feels bad to make an exception to solve one case as a first option. It might be a thing we need to do, but it feels bad to start assuming that
<TabAtkins> RESOLVED: Restrict container-name to just idents

@mirisuzanne mirisuzanne moved this from In progress to Needs Edits in Container Queries [css-contain] Feb 23, 2022
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 23, 2022
Add tests for strings being invalid as container-name.

Resolution:

w3c/csswg-drafts#6405

Bug: 1145970
Change-Id: I4bdfa66a672e3fec08cf1300f0f50dddf60a8325
aarongable pushed a commit to chromium/chromium that referenced this issue Feb 24, 2022
Add tests for strings being invalid as container-name.

Resolution:

w3c/csswg-drafts#6405

Bug: 1145970
Change-Id: I4bdfa66a672e3fec08cf1300f0f50dddf60a8325
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3483674
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#974637}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 24, 2022
Add tests for strings being invalid as container-name.

Resolution:

w3c/csswg-drafts#6405

Bug: 1145970
Change-Id: I4bdfa66a672e3fec08cf1300f0f50dddf60a8325
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3483674
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#974637}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 24, 2022
Add tests for strings being invalid as container-name.

Resolution:

w3c/csswg-drafts#6405

Bug: 1145970
Change-Id: I4bdfa66a672e3fec08cf1300f0f50dddf60a8325
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3483674
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#974637}
@mirisuzanne mirisuzanne moved this from Needs Edits to Done in Container Queries [css-contain] Feb 24, 2022
DanielRyanSmith pushed a commit to DanielRyanSmith/wpt that referenced this issue Feb 28, 2022
Add tests for strings being invalid as container-name.

Resolution:

w3c/csswg-drafts#6405

Bug: 1145970
Change-Id: I4bdfa66a672e3fec08cf1300f0f50dddf60a8325
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3483674
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#974637}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 22, 2022
…w idents as container-name, a=testonly

Automatic update from web-platform-tests
[@container] CSSWG resolved to only allow idents as container-name

Add tests for strings being invalid as container-name.

Resolution:

w3c/csswg-drafts#6405

Bug: 1145970
Change-Id: I4bdfa66a672e3fec08cf1300f0f50dddf60a8325
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3483674
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#974637}

--

wpt-commits: aac1ad269ad1683170b8a09f9b9490293e23c2ba
wpt-pr: 32953
aosmond pushed a commit to aosmond/gecko that referenced this issue Mar 26, 2022
…w idents as container-name, a=testonly

Automatic update from web-platform-tests
[@container] CSSWG resolved to only allow idents as container-name

Add tests for strings being invalid as container-name.

Resolution:

w3c/csswg-drafts#6405

Bug: 1145970
Change-Id: I4bdfa66a672e3fec08cf1300f0f50dddf60a8325
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3483674
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#974637}

--

wpt-commits: aac1ad269ad1683170b8a09f9b9490293e23c2ba
wpt-pr: 32953
@yisibl
Copy link
Contributor

yisibl commented Mar 27, 2022

@font-palette-values:

Update: @font-palette-values changed to <dashed-ident>

Add

@font-feature-values: <dashed-ident> | <string>

mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
Add tests for strings being invalid as container-name.

Resolution:

w3c/csswg-drafts#6405

Bug: 1145970
Change-Id: I4bdfa66a672e3fec08cf1300f0f50dddf60a8325
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3483674
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#974637}
NOKEYCHECK=True
GitOrigin-RevId: 7a942e0782a4477197d6e23c7c5d060f2582c2bf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

8 participants