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] Should style() queries allow !important flag? #7413

Closed
lilles opened this issue Jun 24, 2022 · 15 comments
Closed

[css-contain-3] Should style() queries allow !important flag? #7413

lilles opened this issue Jun 24, 2022 · 15 comments

Comments

@lilles
Copy link
Member

lilles commented Jun 24, 2022

According to the current draft[1], the syntax for style features are the same as for declarations which means !important is allowed as part of the feature. I don't think that would have an effect, but would it still make sense to change the syntax to not allow it?

[1] https://drafts.csswg.org/css-contain-3/#style-container

@tabatkins
Copy link
Member

We should disallow, yeah.

@frivoal
Copy link
Collaborator

frivoal commented Aug 24, 2022

In the case of @supports,

A trailing !important on a declaration being tested is allowed, though it won’t change the validity of the declaration.

I suspect we may want to do the same here: allow the !important syntactically, but ignore it. That way, we're consistent about the allowed syntaxes in the places that let you give a declaration for the sake of checking something rather than for declaring something. That said, if people feel strongly about disallowing it, that seems ok too.

@fantasai
Copy link
Collaborator

Disagree in the case of @supports. You're doing a syntax "is it supported" check there, and !important is arguably part of it.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-contain-3] Should style() queries allow !important flag?.

The full IRC log of that discussion <astearns> topic: [css-contain-3] Should style() queries allow !important flag?
<astearns> github: https://github.com//issues/7413
<argyle> astearns: who's gonna take this one?
<argyle> florian: the syntax for style container queries, lets you specify a property declaration representing the computed value, and grammar wise, a prop dec can include !important. but that has no effect on th computed value
<argyle> florian: q is, shouuld we ban writing !Important or should be keep it even tho it has no effect?
<astearns> ack fantasai
<emilio> q+
<argyle> fantasai: i think we should disallow this becaues it represents a computed value, it has no concept of where it exists in the cascade
<florian> q+
<argyle> fantasai: i dont feel the same about @supports rules which have a siimilar sytnax, in that case, you're asking is this syntax supported and then it's fine to have !important be part of that. but if add another !something later, it'd return null
<astearns> ack emilio
<argyle> emilio: i think i disagree with fantasai because even tho it does rep the computed value, what you write is a specified value. you can query color: blue, and it's computed then compared, but
<argyle> emilio: it's a specified value right
<astearns> ack florian
<miriam> q+
<argyle> florian: i dont feel strongly, seems to me it's easier if we can copy/paste declarations from where they define and test them, to test. that might drag a !impportant along the way. if we allow it in @supports, could support it here as well. but i dont feel strongly
<astearns> agrees with the copy-paste argument
<astearns> ack miriam
<argyle> miriam: was thinking about resolving variables, custom properties in a query, but they won bring along !important. never mind, i'm fine either way
<argyle> florian: if you're using actual css var, your'e right, but if you're using server side processor that has something similar to variables, it might
<argyle> miriam: that's true
<argyle> astearns: we are out of time, is anyone else convinced byt he copy paste arg to not make a change to the sepc?
<argyle> astearns: or should we continue discussion int he issue?
<argyle> astearns: not going to get reslution to this, we will continue in the issue. bring it back to the agenda onde we have clarity
<masonf> Thank you Adam!

@astearns
Copy link
Member

I think it is very likely that authors will copy-paste values (or have them set by build-time variables) that will contain !important and be surprised or not notice that their style() query became invalid. So I think we should continue to allow them.

@astearns astearns removed the Agenda+ label Aug 24, 2022
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Sep 9, 2022
Change the implementation to accept !important in style feature
declarations. This is in line with the current spec draft, and also what
the current discussion in the spec issues[1] is leaning towards.

[1] w3c/csswg-drafts#7413

Bug: 1357573, 1302630
Change-Id: Ib08fbb01943f425aebae574f082d446161c5a544
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Sep 9, 2022
Change the implementation to accept !important in style feature
declarations. This is in line with the current spec draft, and also what
the current discussion in the spec issue[1] is leaning towards.

[1] w3c/csswg-drafts#7413

Bug: 1357573, 1302630
Change-Id: Ib08fbb01943f425aebae574f082d446161c5a544
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Sep 9, 2022
Change the implementation to accept !important in style feature
declarations. This is in line with the current spec draft, and also what
the current discussion in the spec issue[1] is leaning towards.

[1] w3c/csswg-drafts#7413

Bug: 1357573, 1302630
Change-Id: Ib08fbb01943f425aebae574f082d446161c5a544
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3883945
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1045038}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Sep 9, 2022
Change the implementation to accept !important in style feature
declarations. This is in line with the current spec draft, and also what
the current discussion in the spec issue[1] is leaning towards.

[1] w3c/csswg-drafts#7413

Bug: 1357573, 1302630
Change-Id: Ib08fbb01943f425aebae574f082d446161c5a544
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3883945
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1045038}
@lilles
Copy link
Member Author

lilles commented Sep 9, 2022

FWIW, I'm fine with closing this as no change. I have changed our implementation to allow !important and adjusted the WPTs accordingly to match the current spec.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 24, 2022
…) queries, a=testonly

Automatic update from web-platform-tests
[@container] Accept !important in style() queries

Change the implementation to accept !important in style feature
declarations. This is in line with the current spec draft, and also what
the current discussion in the spec issue[1] is leaning towards.

[1] w3c/csswg-drafts#7413

Bug: 1357573, 1302630
Change-Id: Ib08fbb01943f425aebae574f082d446161c5a544
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3883945
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1045038}

--

wpt-commits: 375d9e8867b967793a119dfd111b89ed467f0757
wpt-pr: 35842
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Sep 27, 2022
…) queries, a=testonly

Automatic update from web-platform-tests
[@container] Accept !important in style() queries

Change the implementation to accept !important in style feature
declarations. This is in line with the current spec draft, and also what
the current discussion in the spec issue[1] is leaning towards.

[1] w3c/csswg-drafts#7413

Bug: 1357573, 1302630
Change-Id: Ib08fbb01943f425aebae574f082d446161c5a544
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3883945
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1045038}

--

wpt-commits: 375d9e8867b967793a119dfd111b89ed467f0757
wpt-pr: 35842
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
Change the implementation to accept !important in style feature
declarations. This is in line with the current spec draft, and also what
the current discussion in the spec issue[1] is leaning towards.

[1] w3c/csswg-drafts#7413

Bug: 1357573, 1302630
Change-Id: Ib08fbb01943f425aebae574f082d446161c5a544
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3883945
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1045038}
NOKEYCHECK=True
GitOrigin-RevId: ad5afaf41372a8933f32e4746feb1c734b02a67c
@mirisuzanne
Copy link
Contributor

Sorry for that noise. I was clicking badly. Meanwhile, I agree with the general (though incomplete) consensus here that !important should be allowed and ignored. Proposing we close this wontfix.

@lilles
Copy link
Member Author

lilles commented Oct 31, 2022

I agree that we should just close this. I have updated our impl to match the current spec and allow !important in style() query values.

@lilles lilles added the Async Resolution: Proposed Candidate for auto-resolve with stated time limit label Dec 16, 2022
@lilles
Copy link
Member Author

lilles commented Dec 16, 2022

Proposed resolution: no change

@astearns
Copy link
Member

@tabatkins @fantasai would you be OK with a resolution of no change here?

@LeaVerou
Copy link
Member

Sorry for that noise. I was clicking badly. Meanwhile, I agree with the general (though incomplete) consensus here that !important should be allowed and ignored. Proposing we close this wontfix.

Another vote for this.

@tabatkins
Copy link
Member

I have no strong opinion, so I'm fine with an async resolution in either direction.

@fantasai
Copy link
Collaborator

Ditto @tabatkins. I personally think it doesn't belong as part of style() syntax, but if there's a convenience to allowing it I don't see it as problematic.

@astearns
Copy link
Member

Since this has consensus here in the issue we likely do not need to spend meeting time on it.

The CSSWG will automatically accept this resolution one week from now if no objections are raised here. Anyone can add an emoji to this comment to express support. If you do not support this resolution, please add a new comment.

Proposed Resolution: No Change

@astearns astearns removed the Agenda+ label Dec 22, 2022
@astearns
Copy link
Member

astearns commented Jan 3, 2023

RESOLVED: No change

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

8 participants