WorryFree Computers   »   [go: up one dir, main page]

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

[selectors] Let :is() have better error-recovery behavior than normal Selectors #3264

Open
tabatkins opened this issue Oct 29, 2018 · 29 comments
Labels
selectors-4 Current Work

Comments

@tabatkins
Copy link
Member

This isn't captured in the current spec, but I remember earlier speculation that we could use :matches() as a way to get around the bad Selectors behavior of "a syntax error in one complex selector invalidates the whole sequence" that we're stuck with. Do we still think this is a worthwhile idea to pursue?

Spec-wise, what this would mean is defining the official syntax as :matches( <any-value> ), then split the result on top-level comma tokens, then attempt to parse each item as a <complex-selector>, and just ignore any invalid ones. (If all of them are invalid, the selector matches nothing.)

Then, if you're concerned about using a newer feature, you can just write your selectors like:

:matches( .foo || .bar, td.foo:first-child ) {
  /* styles for the bar cell */

An unfortunately, but relatively minor, tax for getting better error-recovery behavior. (If we do end up renaming it to :is(), it's even more minor.)

@tabatkins tabatkins added the selectors-4 Current Work label Oct 29, 2018
@CyberAP
Copy link
CyberAP commented Oct 29, 2018

Shouldn't this belong to @supports rule?
Just to imagine how bloated stylesheet would get if you have to use new selectors extensively. While with @supports you can only test it once and that's it.
Yes it doesn't cover selector check right now, but with Custom Selectors from CSS Extensions this should be possible.

@SelenIT
Copy link
Collaborator
SelenIT commented Oct 29, 2018

@CyberAP, the point is to avoid having to write the same rules twice (for new and all browsers).

For example, consider a menu that is by default transformed into a hamburger icon (or something), but should "uncollapse" if either hovered or has a link inside it focused via keyboard. For modern browsers, I can write

.menu:hover, .menu:focus-within { transform: none; }

...but in browsers that don't support :focus-within this would not apply at all, and I would have to write this rule twice, for each selector separately, in order to make at least the :hover part work there.

However, with the new proposal, the following hypothetical example

:is(.menu:hover, .menu:focus-within, :current(.menu)) { transform: none; }

would be able to work in browsers that already support :is() and these two pseudo-classes, but don't support :current() yet.

How can this be solved with @supports (even if extended to test for selectors)? For me, this looks like the opposite of what @supports does (combining the code for old and new browsers into one rule, rather than separating it into different rules). Would be happy to be proven wrong!

@CyberAP
Copy link
CyberAP commented Oct 31, 2018

@SelenIT, thanks for a great explanation! That does make sense and actually is a really great feature I would like to have.

@ExE-Boss
Copy link
Contributor
ExE-Boss commented Nov 2, 2018

You mean like #3082, except without the vendor prefix exemption?

@tabatkins
Copy link
Member Author

Yes, and safely; I'm pretty sure we can't do #3082.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Let :matches() have better error-recovery behavior than normal Selectors, and agreed to the following:

  • RESOLVED: Use media query style invalidation inside pseudo classes that accept selector lists
The full IRC log of that discussion <dael> Topic: Let :matches() have better error-recovery behavior than normal Selectors
<dael> github: https://github.com//issues/3264
<dael> Rossen: Is TabAtkins on?
<dael> fantasai: I can take it
<dael> fantasai: When we have a list of selectors :foo, :bar
<dael> fantasai: Browser doesn't rec :foo so the entire style rule is thrown out
<dael> fantasai: That's selectors invalidation.
<dael> fantasai: Other style of invalidation is media queries-like where you throw out a section. So if you have foo, screen we recognize screen
<dael> fantasai: Can't do MQ like because it would break the web for selectors. TabAtkins pointed out we have ability to do it within the new selectors in L4
<dael> fantasai: Issue proposes we adopt MQ-like invalidation inside the pseudoclasses that take selectors.
<dael> fantasai: :foo || :bar, [known selector] we'd only do the part we recognize.
<dael> fantasai: Makes a lot os sense to me together with @supports rule we added. Gives authors much better tools. If they want more granular they can use something else
<dael> Rossen: Sounds reasonable.
<dael> Rossen: Opinions?
<dael> myles: Thoughts from people that teach this? Seems like could be confusing
<dael> Rossen: leaverou or rachelandrew ?
<dbaron> Were you suggesting doing different things for :matches() vs. :is() ?
<tantek> agreed with myles, it could be confusing. would definitely want webdev teacher feedback on this.
<dael> fantasai: dbaron this was from before we renamed. Title should be is. Applies to is, not, has, nth-child, and maybe current
<dael> dbaron: One worry is some have been around for a while and might have compat issues
<dael> fantasai: :not with commas isn't widely supported.
<dael> fantasai: Not with a single argument that's invalid makes the whole thing invalid
<dael> florian: I think not with a comma is supported in Safari and Viviostyle
<dael> fantasai: Not is trickier because it's a negation
<dael> leaverou: Trying to decide error recovery or syntax?
<dael> fantasai: Error recovery
<dael> leaverou: Sounds amazing thing to do. It might be a little confusing, but it's worth it. In talks I only use webkit version and have to mention verbally it's just webkit. As an author I would love it
<dael> emilio: Doesn't solve unknown pseudo elements issue, right?
<dael> fantasai: No, sep.
<dael> emilio: I think that's biggest issue authors want to solve
<dael> fantasai: There's a rule for the webkit
<dael> emilio: Want to style a video control for webkit and edge it's not stanard. That's the biggest source of duplication
<dael> fantasai: This would solve, put it in all one :is
<dael> emilio: Syntax allow psuedo elements inside?
<bradk> I’m still concerned about :not. Can we find out how much authors have :-webkit and commas inside not?
<dael> fantasai: This is work for pseudo classes. Elements is next on agenda
<florian> I support this
<dael> Rossen: bradk point about not and his concern, can we address that now?
<dael> Rossen: Concern is the :not and defining how much authors have commas inside a not?
<dael> florian: It's safari only
<dael> bradk: Safari on iphone is used a lot. :not has been without commas for a while, but it's used in mobile web a lot. That's my suggestion, I'd like actual data
<dael> florian: This is hard data to get. Need to find usage that would break the page if it started working in a different way. That's a judgement call
<dael> Rossen: Another way to ask is if any Apple folks have an issue with this. If they feel comfortable with compat risk we can resolve
<dael> bradk: That solution would be okay
<dael> smfr: I don't think we have enough [missed]
<dael> smfr: I don't thinkw e have enough information to know. When we impl :not we got compat issues b/c people using it in wrong ways
<dael> smfr: No feeling for how common comma use is to know if it's risky
<dael> Rossen: Would you be okay with current proposal in the absense of this information?
<dael> smfr: I think so
<dael> Rossen: Anything else before we try and resolve?
<bradk> No strong objection
<dael> fantasai: Use media query style invalidation inside psuedo classes that accept selector lists
<dael> Rossen: Objections to ^?
<dael> emilio: Would like behavior for :not clarified
<dael> fantasai: Makes sense
<dael> RESOLVED: Use media query style invalidation inside pseudo classes that accept selector lists
<dael> Rossen: fantasai and TabAtkins will have clarification in spec

@fantasai
Copy link
Collaborator

emilio pointed out that we need to be careful about how we handle :not() here. It's not obvious it would work as intended if we just ignore unknown arguments, and also a single argument to :not() likely has Web-compat implications since it's already implemented.

A related question is :nth-child(); simply ignoring one of its arguments could throw off the count. There likely wouldn't be a Web-compat problem with it, though.

@fantasai fantasai changed the title [selectors] Let :matches() have better error-recovery behavior than normal Selectors [selectors] Let :is() have better error-recovery behavior than normal Selectors Nov 21, 2018
@ExE-Boss
Copy link
Contributor
ExE-Boss commented Nov 21, 2018

The thing about :not() is that only the single‑argument form is supported in major browsers, so we could probably apply it to the multi‑argument form and be fine.

Also, at least for unknown pseudo‑classes, we could have those match nothing, so :not(:unsupported, :unsupported-2) would match everything because :not(:is(:unsupported, :unsupported-2)) would match everything, as :is(:unsupported, :unsupported-2) would match nothing.

@ewilligers
Copy link
Contributor

I agree that we could minimize the Web-compat risk by saying that error recovery only applies if two or more selectors are supplied.

:nth-last-child(:nonsense) is invalid
:nth-child(:nonsense, :enabled) is :nth-child(:enabled)
:not(:nonsense, :gibberish) is *
:is(:nonsense, :gibberish) is :not(*).

This would have the following benefits:

  • :not(.) and :nth-child(.) and :nth-last-child(.) with a single selector have the same semantics as in Selectors 3.
  • We have error recovery for :is(...) and :where(...) and :has(...)
  • :not(...) and :is(:not(...)) and :not(:is(...)) are always equivalent, same for :nth-child and :nth-last-child

As a minor variation, preserving the same benefits, we could say that error recovery only applies if at least one of the selectors is valid. Then

:not(:nonsense, :gibberish) is invalid.

This might be easier to teach, and avoid developers' confusion when they see an introduced :not(*) in a Developer Tool that hasn't preserved raw text.

@ExE-Boss
Copy link
Contributor

As a minor variation, preserving the same benefits, we could say that error recovery only applies if at least one of the selectors is valid.

Then :not(:nonsense, :gibberish) is invalid.

That should probably only apply to :not(…), as :commonly-supported, :is(:rarely-supported) can be used to avoid invalidating the entire selector list if the :rarely-supported pseudo‑class isn’t supported by the current browser but :is(…) and :commonly-supported are supported by the current browser.

Also, it might be a good idea to still parse :not(:nonsense, :gibberish) as valid, but matching nothing.

aarongable pushed a commit to chromium/chromium that referenced this issue Nov 29, 2018
CSS Selectors 4 allows :not to accept selector lists.

There is a proposal for invalid complex selectors in the list to
be ignored (instead of causing the whole selector to fail parsing.

This would affect :is :where :nth-child :nth-last-child, :has and :not,
with the main compatibility risk being :not.

w3c/csswg-drafts#3264

We add use counters to estimate the Web compatibilty risk.

BUG=568705

Change-Id: I708d48d626a6e61cc8c1076d40d2c33bb19496e3
Reviewed-on: https://chromium-review.googlesource.com/c/1354751
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Eric Willigers <ericwilligers@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612177}
@ewilligers
Copy link
Contributor

For :not(), I added Blink use counters.

kCSSSelectorNotWithValidList
6% of pages have :not() with 2+ selectors, all valid, or a valid non-simple selector, e.g. :not(*, *) or :not(* + *)

kCSSSelectorNotWithInvalidList
4.5% of pages have :not() with no valid selectors, e.g. :not(:nonsense)

kCSSSelectorNotWithPartiallyValidList
0.09% of pages have :not() containing both valid and invalid selectors, e.g. :not(*, :nonsense)

@ewilligers
Copy link
Contributor

We haven't discussed what dev tools, etc., should show when the author provided :is(:nonsense). Do people prefer :is() or :is(:not(*))? This might not be a spec issue.

@ExE-Boss
Copy link
Contributor
ExE-Boss commented Feb 13, 2019

I think displaying it as :is(:nonsense ⚠️) might be the best option.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Error recovery for :is() and :not(), and agreed to the following:

  • RESOLVED: Drop individual invalid selectors from selector lists in *all* selector functions that take such lists, *except* for :not().
The full IRC log of that discussion <presenter> Topic: Error recovery for :is() and :not()
<astearns> github: https://github.com//issues/3264
<presenter> ewilligers: We resolved we'd like to have error-recovery for :is() and :where(), but unsure about :not().
<presenter> ewilligers: I have use-cases showing that :not() is used frequently, so we probably don't want to change the meaning.
<presenter> ewilligers: Do we want :nth-child() or :nth-last-child() have error-recovery?
<presenter> fantasai: nth-child should probably have the same error-recovery as :is()
<florian> fantasai: I think it would make more sense with compound, not complex selectors
<presenter> fantasai: The big question for nth-child was compound vs complex selectors.
<florian> fantasai: people want that
<presenter> fantasai: If that makes it easier, would be good. Things like zebra-striping non-hidden rows.
<presenter> ewilligers: I think webkit already shipped complex selectors in nth-child
<fantasai> s/good/good to get it implemented faster/
<fantasai> s/rows/rows is a major use case that's not currently solved otherwise/
<presenter> dydz: I'm looking in the WK code
<presenter> fantasai: Emilio asked why we need selectors in :nth-child()
<presenter> fantasai: Say we have a list of items, I want to color their backgrounds alternating
<presenter> fantasai: But some of them are hidden; they're display:none
<presenter> fantasai: Counting is based on sibling list. If you hide 3rd one, 2nd and 4th will both have the same color
<presenter> fantasai: So want to count after filtering
<presenter> emilio: Whew, that's gonna be slow. We have a lot of caching already to make :nth-of-type() fast.
<presenter> ewilligers: I confirmed that Safari supports :nth-child(even of div+div)
<presenter> fantasai: My main concern is if other impls are more likely to implement without complex selectors, getting even compound-selector interop would still be great.
<presenter> emilio: I think impl-wise, not supporting complex selectors is easier.
<presenter> TabAtkins: Unless we recursively pass down the compound-only restriction into :is()/etc, you're at full power anyway.
<presenter> fantasai: Maybe that's reasonable to have that sort of restriction.
<tantek> agreed. reasonable restriction
<presenter> emilio: Did we decide on how the new error recovery behavior serializes.
<fantasai> fantasai: We'll just say Safari supports L5 of selectors, and L4 doesn't accept combinators
<presenter> emilio: Did we decide about serialization of invalid selectors in :is()?
<presenter> TabAtkins: I didn't think we were doing anything in particular?
<presenter> ewilligers: I thought we'd just drop them.
<presenter> emilio: What if they're all invalid?
<presenter> ewilligers: You just get :is(). A little weird with :nth-child(even of), tho.
<presenter> ewilligers: What about parsing of weird stuff? Extra close brackets, what happens?
<presenter> TabAtkins: Syntax already handles brackets correctly. We just split the tokens on top-level commas, then interpret each chunk as a selector.
<presenter> fantasai: So back to the issue: :is() and :not() take a list of selectors
<presenter> fantasai: Currently any invalid selector invalidates the entire selector list.
<presenter> fantasai: Because we now have these functional notations that creates some scoping, within :is() we can just ignore in the invalid selector, but still process the rest.
<presenter> fantasai: So we're gonna resolve that within :is(), we ignore invalid selectors.
<presenter> ewilligers: And :where(), :nth-child(), :has(), etc.
<presenter> fantasai: But not in :not() - the whole thing will invalidate if any are any invalid.
<presenter> gregwhitworth: I think it's weird to have :not() work different. I get the %s making it hard tho.
<presenter> AmeliaBR: I do use :not() as an @supports for new selectors...
<fantasai> AmeliaBR: e.g. :not(:valid) means you support :valid, and this selector selects things that are not valid
<presenter> TabAtkins: I was going over the boolean logic for :not() invalidating just parts too this morning, and was getting really confused...
<presenter> RESOLVED: Drop individual invalid selectors from selector lists in *all* selector functions that take such lists, *except* for :not().

aarongable pushed a commit to chromium/chromium that referenced this issue Feb 26, 2019
This reverts the use counters added in
https://chromium-review.googlesource.com/c/chromium/src/+/1354751

The CSS Working Group was considering allowing :not() to
ignore selectors that are invalid or not accepted by the browser.

Use counts showed that this would affect a large percentage of pages
w3c/csswg-drafts#3264 (comment)
and so the decision was made to not change the behavior of :not
w3c/csswg-drafts#3264 (comment)

Data collection is no longer required.

BUG=568705

Change-Id: I38ddaee5339e19b70c1c13c9507a50ae27c354b0
Reviewed-on: https://chromium-review.googlesource.com/c/1487753
Commit-Queue: Emil A Eklund <eae@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635559}
SergioOpenPeer pushed a commit to webrtc-uwp/chromium-tools that referenced this issue May 10, 2019
CSS Selectors 4 allows :not to accept selector lists.

There is a proposal for invalid complex selectors in the list to
be ignored (instead of causing the whole selector to fail parsing.

This would affect :is :where :nth-child :nth-last-child, :has and :not,
with the main compatibility risk being :not.

w3c/csswg-drafts#3264

We add use counters to estimate the Web compatibilty risk.

BUG=568705

Change-Id: I708d48d626a6e61cc8c1076d40d2c33bb19496e3
Reviewed-on: https://chromium-review.googlesource.com/c/1354751
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Eric Willigers <ericwilligers@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#612177}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 7f276c9a8d885ab388eb17cf4e00f54974d46715
SergioOpenPeer pushed a commit to webrtc-uwp/chromium-tools that referenced this issue May 10, 2019
This reverts the use counters added in
https://chromium-review.googlesource.com/c/chromium/src/+/1354751

The CSS Working Group was considering allowing :not() to
ignore selectors that are invalid or not accepted by the browser.

Use counts showed that this would affect a large percentage of pages
w3c/csswg-drafts#3264 (comment)
and so the decision was made to not change the behavior of :not
w3c/csswg-drafts#3264 (comment)

Data collection is no longer required.

BUG=568705

Change-Id: I38ddaee5339e19b70c1c13c9507a50ae27c354b0
Reviewed-on: https://chromium-review.googlesource.com/c/1487753
Commit-Queue: Emil A Eklund <eae@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#635559}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 6b9a6186caaf4b814b297d2e3f7fe3d22c3b6eff
@ExE-Boss
Copy link
Contributor

What should happen if someone were to write:

:is(h1, h3), :is(h2:maybe-unsupported) { font-family: sans-serif }

I believe that it should be treated as if the following was written in browsers that don't support :maybe‑unsupported, but support :is(…):

:is(h1, h3) { font-family: sans-serif }

@pygy
Copy link
pygy commented Feb 6, 2020

I'm all for this, but it will interact surprisingly with selector nesting, since a, b { & c, &d {}} becomes :is(a, b) c, :is(a, b) d{}.

It makes nesting forgetgiveful except at the last level... So c or d resulting in a parsing error will cause the rejection of the last nesting level.

@danburzo
Copy link

Drive-by comment: seems that in Firefox 78+, and the upcoming Safari 14 (currently in the Technology Preview stage), the :is() and :where() pseudo-classes become invalid when an invalid selector is included in the list. For example, document.querySelectorAll('div:is(div, :bogus)') throws in both.

@frivoal
Copy link
Collaborator
frivoal commented Aug 24, 2020

Agenda+ to look into the problem raised by #3264 (comment), and to consider whether compat is starting to paint us into a corner here.

@emilio
Copy link
Collaborator
emilio commented Aug 24, 2020

I'm happy to implement this in Gecko if people define in the spec how to deal with the empty-selector behavior. Like, should :is(:nonsense) serialize just as :is()? Should that be special-cased to never match?

Also, it may be a bit confusing if people write stuff like :is(::before, ::after), and it gets turned to :is() instead of being rejected as invalid...

@emilio
Copy link
Collaborator
emilio commented Aug 24, 2020

Then, of course :is() should be parseable too...

@ewilligers
Copy link
Contributor

should :is(:nonsense) serialize just as :is()? Should that be special-cased to never match?

Then, of course :is() should be parseable too...

Yes, I was going to serialize :is(:nonsense) as :is() in dev tools, and treat :is() as a valid selector that never matches.

Is serialization of selectors web exposed, i.e. testable in WPTs?

@Loirooriol
Copy link
Contributor

@ewilligers Sure, include /css/support/parsing-testcommon.js and then

test_valid_selector(":is(:nonsense)", ":is()");

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [selectors] Let :is() have better error-recovery behavior than normal Selectors.

The full IRC log of that discussion <dael> Topic: [selectors] Let :is() have better error-recovery behavior than normal Selectors
<dael> github: https://github.com//issues/3264
<dael> ericwilligers: We resolved to have better error recovery but WK shipped without and FF is about to. Is it too late or do we still want error recovery?
<dael> TabAtkins: If possible I'd like to have it. If we have to oh well, but it's bad behavior without
<dael> ericwilligers: Who wants to do spec update?
<dael> ericwilligers: We resolved on issue but spec isn't updated
<dael> TabAtkins: I'll do it if we agree today to proceed
<dael> Rossen_: We're wanting to hold previous resolution that requires error recovery and make that edit into spec? Or are we trying to relax the error recovery and not proceed with edits?
<dael> ericwilligers: [missed]
<dael> ericwilligers: I'm propsing the first, that we go ahead and make edit.
<dael> Rossen_: You're proposing add it to spec and pester impl to do that
<dael> ericwilligers: Yes
<astearns> notes that Emilio says he's OK implementing it
<dael> Rossen_: Alright. Any thoughts on that? Are we still committed and have TabAtkins add edits?
<dael> TabAtkins: We'd need FF or WK dev to be okay doing change
<dael> Rossen_: emilio is signaling he's fine with change. Only WK is going to be required to update. Anyone from WK on?
<dael> smfr: If emilio is fine changing we're fine changing
<dael> Rossen_: We have a resolution to do error recovery. Do we need resolution to have TabAtkins edit it in?
<dael> Rossen_: We'll close no change but to put in the required edits
<dael> Rossen_: Thank you

@astearns astearns removed the Agenda+ label Sep 2, 2020
@emilio
Copy link
Collaborator
emilio commented Sep 13, 2020

https://bugzilla.mozilla.org/show_bug.cgi?id=1664718 is the Gecko bug for this (sorry for the lag, busy month!). I plan to enable it on Nightly and beta for now to collect potential breakage, but will enable after a couple releases everywhere if there's no issue.

@tabatkins
Copy link
Member Author

Just verifying as I write the spec - in #3264 (comment) @ewilligers suggested that we handle "single selector, which is invalid" differently from the rest of the cases, since that has legacy implications, but it looks like we're no longer doing that?

That is, :is(:unknown) is valid but matches nothing, just like :is() and :is(:unknown1, :unknown2)?

tabatkins added a commit that referenced this issue Sep 14, 2020
… and :where(). (Per WG resolution, we dont' use it in :not().) #3264
@emilio
Copy link
Collaborator
emilio commented Sep 14, 2020

Yes, that's what I implemented, fwiw. I don't see why "single invalid selector" should be different.

@tabatkins
Copy link
Member Author

Cool, then please make sure the spec edits linked above are satisfactory; they apply to :is(), :where(), and :has(), but not :not(), per resolutions earlier in the thread.

@emilio
Copy link
Collaborator
emilio commented Sep 14, 2020

They look good. I'd probably note that there's an interesting side-effect of this behavior which I'm not sure it's great, which is that the specificity of :is(:unknown, div), will change when a browser starts supporting :unknown. I don't see a great way to avoid that though.

@tabatkins
Copy link
Member Author

Yeah, unless I distinguish between "parsable, just using unknown selectors" and "lol wtf is this", I can't handle that.

And even then, a "parsable but unknown" selector might have complex specificity behavior, so we actually can't even predict that. So it's just something we'll have to accept.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
selectors-4 Current Work
Projects
None yet
Development

No branches or pull requests