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

[WebComponents] Custom state pseudo class #428

Closed
3 of 5 tasks
tkent-google opened this issue Oct 8, 2019 · 40 comments
Closed
3 of 5 tasks

[WebComponents] Custom state pseudo class #428

tkent-google opened this issue Oct 8, 2019 · 40 comments
Assignees
Labels

Comments

@tkent-google
Copy link

tkent-google commented Oct 8, 2019

こんにちはTAG!

I'm requesting a TAG review of:

Further details:

We recommend the explainer to be in Markdown. On top of the usual information expected in the explainer, it is strongly recommended to add:

You should also know that...

N/A

We'd prefer the TAG provide feedback as (please select one):

  • open issues in our GitHub repo for each point of feedback
  • open a single issue in our GitHub repo for the entire review
  • leave review feedback as a comment in this issue and @-notify [@rakina, @domenic, @tkent-google]
@plinss
Copy link
Member

plinss commented Oct 9, 2019

Personally I'm very happy to see this moving forward, custom states visible as pseudo classes is something that I've wanted for a long time.

I'm concerned however that this design seems to only allow boolean states. While boolean states should be simple for authors to use, I can see many use cases for other types, at the least strings and numbers.

I accept that other values opens up a number of issues that may not want to be addressed in version 1 of this proposal (e.g. do we allow for more than simple value matching, substrings, numeric comparisons?), but at the least it would be good if the api surface allowed for other types in the future. Perhaps the states attribute should be a map rather than a token list?

I'm also somewhat in favor of the :--state syntax (or :--state(value) for non-boolean states), but I'd also want to see it kept in alignment with ::part(), so if this uses :--state then custom parts should use ::--part. (fwiw, differentiating pseudo elements from pseudo classes via :: vs : was intended for this extensibility point.)

@tkent-google
Copy link
Author

tkent-google commented Oct 10, 2019

@plinss Thank you for the comment.

Currently I agree with Tab's comment about non-boolean states. If we want to add more complex values, we can add something like addValue(token, value) to states object, and extend :state(<ident>) to :state(<ident>=value) in the future version.

If we want really-flexible state matching, we should apply Houdini-like extensibility to selector matching. e.g. my-element:func(myMatching) { ...} and function myMatching(element) { return element.state == 42; }.

@annevk
Copy link
Member

annevk commented Oct 10, 2019

(I was about to give thumbs up to @tkent-google's comment, but I'm not enthused at all about running script while selector matching. So thumbs up to the first bit.)

@plinss
Copy link
Member

plinss commented Oct 10, 2019

Using multiple booleans to represent non-boolean values can easily get out of hand as values get more complex. What about numeric states? It also doesn't have a clean path towards a future version where other state values are allowed, e.g. do I have to support both mode-collapsed and mode=collapsed?

I'm also not a fan of adding methods to a DOMTokenList that effectively turn it into something that's map-like when we have a perfectly serviceable map class already. The ergonomics of that API will just get weird and non-standard. We have enough of those, let's not add more.

I agree that we don't need complex selector matching in V1, I just want to see the API surface and the CSS syntax be a bit more future-proof. (FWIW, I think I prefer :state(mode collapsed) or :--mode(collapsed) rather than :state(mode=collapsed), there's no need to simply replicate attribute selectors, lets treat these like proper pseudo classes.)

I also agree that adding selectors that require script execution isn't a good answer here.

@annevk
Copy link
Member

annevk commented Oct 11, 2019

Thus far HTML has needed very few pseudo-classes that are more complex than booleans and in general folks mostly use classes and IDs which come down to booleans so I think you're overstating the case a bit. If there's enough demand for functional states of sorts I'd expect that to end up with a different API in any event.

@plinss
Copy link
Member

plinss commented Oct 11, 2019

Few is not zero, there's already an existence proof of the need for more than binary states. As people develop more custom elements it's not a stretch to imagine more uses.

The entire point of custom elements is to do things that HTML can't, so why hamstring custom elements to the least of HTML element's capabilities? The goal here is to be able to extend HTML, right?

There's also no point in replicating classes by another name. We already have the ability to set custom classes.

And why would we want two different APIs for setting custom states?

@domenic
Copy link
Member

domenic commented Oct 11, 2019

There is a section of the explainer which talks about why setting classes is not sufficient, which may be helpful to review.

@karlhorky
Copy link

Thus far HTML has needed

in general folks mostly use

Hm, this language indicates preservation of the status quo - the limited tools that have been historically provided in the platform.

I think new standards discussions should be about coming up with something for the future, and should be at least somewhat influenced by what tools real-world developers use when building web apps of non-insignificant complexity (take hints from real-world usage of frameworks like Lit, Svelte, React, Vue.js, etc. and how they treat the concept of "state").

If the argument is supposed to be to "use the platform", then the platform should provide compelling tools.


I guess I'm saying pretty much the same thing as @plinss is mentioning above too:

The ergonomics of that API will just get weird and non-standard. We have enough of those, let's not add more.

The goal here is to be able to extend HTML, right?

I suppose there is something to be said for not releasing a hamstrung first version.

Although if there would be near-future concrete plans for a more capable v2 right away from the start (a v2 like Tab mentioned as a response to my comment), then I guess that sounds ok as well.

@WebReflection
Copy link

P.S. for @annevk ... the fact a getter could return true but an el.matches(:state(thing)) could return false is also concerning ... nothing my proposal would fix, but an easy road to inconsistent state handling.

@karlhorky
Copy link

karlhorky commented Dec 15, 2019

specs shipping questioning only Googlers don't represent well what the Web/JS ecosystem needs

it would be awesome if specs would ask real-world DOM API users before landing on the browser

Agree :) I'm all for having quick consensus / shipping, but consulting real-world (external) users should be a bigger part of the process... I think standards can learn a lot from the experiences won by frameworks.

@domenic
Copy link
Member

domenic commented Dec 15, 2019

These allegations are not true. The spec is based on feedback from several component companies and browser vendors, most notably those that participated at TPAC.

@WebReflection
Copy link

The spec is based on feedback from several component companies

Who are these several component companies?

and browser vendors, most notably those that participated at TPAC

AFAIK TPAC is not representing the current Web development ecosystem though, which is why I've written what I've written.

@WebReflection
Copy link

WebReflection commented Dec 15, 2019

@domenic, regardless my previous comment, I've simply spent some time trying to understand the proposal and writing down my feedback.

You can surely ignore that, but this is part of developing in the Open Source: feedback might arrive from users, and you can read, ignore, or maybe consider, some part of the feedback.

As Custom Elements user, either as independent Web developer or as employed one, since we ship CE to million active users, I wanted to raise my concerns regarding this API.

If it's too late, then I blame myself for not paying enough attention in this project activities, but since I've been invited to write comments regarding this API, I hoped it wasn't too late already.

My conclusions, beside who's being involved, and I might regret my allegations if my assumptions are all wrong, remain the same: this API doesn't help much Web developers, as it covers only marginally one use case related mostly to Shadow DOM, but it could be better, and hopefully before it lands.

Best Regards.

@tkent-google
Copy link
Author

@WebReflection Thank you for the feedback.

The meaning of a state

The purpose of the proposal is not to help implementation of general states of custom elements. It's to provide a way to expose read-only states which can't be represented by HTML attributes. If a state of a custom element is already exposed as an HTML attribute, we don't need to apply the proposal to the state because we already have attribute selectors.
_internals.states is an interface with which custom element implementations tell their states to browsers. Using it as a primary storage of states is not a goal of the proposal though such usage is possible.

ElementInternals and attachInternals() are not a part of the proposal. The proposal just depends on them. ElementInternals is the place to provides APIs which only custom element implementations can call.

About ditching built-ins Elements

At this moment the proposal doesn't support customized built-in elements, but enabling the feature for customized builtin-elements is not difficult. We'd like to start with the smallest feature, and enabling it needs to change the behavior of existing APIs. So we'd like to defer it.

About using classes and the "collision" argument

_internals is not states. You seems to misread the proposal.

@WebReflection
Copy link

@tkent-google I have simply red the documentation where this._internals = this.attachInternals(); happens in the first example, and I would expect every DOM element capable of relating ElementInternals to any DOM node.

If this works only with Custom Elements, or even worse, with CE that use Shadow DOM, this proposal is not really beneficial for the Web, as CE, and especially the unpolyfillable (in a reliable way) Shadow DOM, are not the most common primitives out there.

@tkent-google
Copy link
Author

@WebReflection
attachInternals() works only with custom elements, but it doesn't require Shadow DOM.

@WebReflection
Copy link

@tkent-google and where would such method exist, if not in the HTMLElement.prototype ? Is it provided at runtime only once defined through customElements ? If so, don't you think it'll be a surprise to not provide that for built-in extends, as all the entry points are identical?

@tkent-google
Copy link
Author

@tkent-google and where would such method exist, if not in the HTMLElement.prototype ? Is it provided at runtime only once defined through customElements ? If so, don't you think it'll be a surprise to not provide that for built-in extends, as all the entry points are identical?

attachInternals() exists on HTMLElement interface, and it raises NotSupportedError if it is called for non-custom elements.
https://html.spec.whatwg.org/C/#dom-attachinternals

@WebReflection
Copy link

@tkent-google thanks, that answer the first part of my question, yet the "least surprise" would be having that working for built-in extends too.

The fact we think new features only for non built-in extends, since these has shipped and work great already in Chrome, Firefox, and the new Edge, is a bit concerning, imho.

Built-in extends should be first citizens of the Web, not something to eventually think about later. Since states use a specific pseudo selector to be accessed, there's not even a possible conflict between :checked and :state(checked), and people using built-ins with CEs would benefit from a consistent API in case they need/want/use state.

Best Regards.

@tkent-google
Copy link
Author

I filed whatwg/html#5166 for customized built-in elements.

@tkent-google
Copy link
Author

I think this review request can be closed if TAG has no other comments.

@alice alice removed this from the 2019-12-03-f2f-cupertino milestone Jan 27, 2020
@plinss
Copy link
Member

plinss commented Feb 21, 2020

@tkent-google please don't close TAG issues. The TAG hasn't made a determination here yet.

@plinss
Copy link
Member

plinss commented Mar 2, 2020

The CSSWG has taken up this issue and has our feedback on board. The TAG will monitor progress there and we're ready to close this for now.

@plinss plinss closed this as completed Mar 2, 2020
josepharhar added a commit to josepharhar/html that referenced this issue Nov 2, 2022
This is based on the WICG draft spec here:
https://wicg.github.io/custom-state-pseudo-class

Here are some spec issues where this feature has been discussed:
w3ctag/design-reviews#428
w3c/csswg-drafts#4805
@w3ctag w3ctag deleted a comment Oct 24, 2023
annevk pushed a commit to whatwg/html that referenced this issue Dec 24, 2023
This is based on this WICG draft: https://wicg.github.io/custom-state-pseudo-class.

This has been discussed in these issues (among others): w3ctag/design-reviews#428 & w3c/csswg-drafts#4805.

This will be added to Selectors level 5: w3c/csswg-drafts#4805.

Tests: https://wpt.fyi/results/custom-elements/state.

Co-authored-by: Domenic Denicola <d@domenic.me>
@rhiaro rhiaro added Mode: breakout Work done during a time-limited breakout session and removed Progress: breakout labels May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests