search

w3ctag / design-reviews

W3C specs and API reviews
Creative Commons Zero v1.0 Universal
332 stars 56 forks source link

[WebComponents] Custom state pseudo class #428

Closed tkent-google closed 4 years ago

tkent-google commented 5 years ago

こんにちは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):

plinss commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

(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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

karlhorky commented 5 years ago

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.

domenic commented 5 years ago

I'd like to stress that this feature is "custom pseudo classes". It is not "general state exposure mechanism". It is not "expose arbitrary data structures for CSS selection". It is just giving custom elements the ability to do what built-in elements do: expose boolean bits of external state, in an element-controlled fashion (unlike class names), to their consumers.

There is an additional capability being discussed, which is the CSS "pseudo class functions" like :lang() or :dir(). I think that would be an additional interesting capability to explore allowing customization of. But, despite both starting with :, I agree with others who have stated that this will require a new, separate API, because it targets rather different underlying models and use cases. For example, something analogous to :lang() would likely not want to be tied to particular element states, but instead run a (JS?) function evaluating computed properties all elements, as the spec for :lang() does. I don't think such "custom pseudo class functions" would be tied to custom elements at all, most likely.

plinss commented 5 years ago

At no point did I ever propose exposing "arbitrary data structures for CSS selection", I talked about strings and numbers in addition to booleans.

And yes, I understand that this feature is about custom pseudo classes. Pseudo classes are a "general state exposure mechanism". Not all states are boolean.

:lang() and :dir() are expressions of state, the fact that they require evaluating properties of multiple elements to compute does not change that. There's no reason that evaluation would have to be done at selector matching time, it can be pre-calculated and stored in the element. Just like I'd expect any other non-boolean (and boolean) custom pseudo elements to be.

The other functional pseudo classes, like :has() and :where() are something I'd expect to see exposed through a completely different mechanism, like a houdini method for custom selectors. That's not what I'm talking about here.

plinss commented 5 years ago

Discussed further within the TAG. Given that people are currently exposing state in custom elements by using attributes, and using attribute selectors, and this feature is meant to supplant that practice, it seems like a good idea to survey existing custom elements to see what kinds of states they're exposing rather than guess... Let's try to gather some real-world data here.

annevk commented 5 years ago

To be clear, the web components work involves quite a few developers using web components day-to-day, and they've been most vocal about getting this added. (See also the issue thread pointed to in OP.)

alice commented 5 years ago

I don't think @plinss was doubting use cases for the feature in general, but more looking to gauge whether a boolean-only solution would be sufficient.

e.g. if someone is styling a custom element based on [attr=foo] rather than [attr], that would indicate that they are styling based on a non-boolean state.

Westbrook commented 5 years ago

As a frequent user of custom elements and shadow dom, I can attest both to the general need and relief in seeing the development of this feature. Thank you all for being involved in making it come to life.

While I agree with @plinss that supporting non-boolean states is something that should be pushed for, I believe I could wait for a phase II (rather impatiently) if implementors believe that doing so would make implementation easier or faster. I've been following the development of the web components specification for a while, so I know that doesn't always come from technical restrictions... I would also like to add another vote against the idea of running script while selector matching, please no! This feature and conversation fall so timely in relation to a recent talk about stateful styles by David Khourshid outlining the benefits of this style of interface: https://slides.com/davidkhourshid/crafting-stateful-styles#/

What would leave me most amenable to waiting for a phase II implementation of :state(mode=collapsed) as a more wholistic proclamation as to the state of my elements would be assurances (by way of inclusion in the explainer) that multiple states would be supported by the current phase of the API. The explainer references :valid as the sort of support that we'd expect to add to our elements, and I hope that would go further as to include :required:focus:hover:valid, etc. Has this been considered, or rather its theoretical parallels of :state(required):state(focus):state(hover):state(valid) or :state(required focus hover valid)? I find it to be a rather logic and important extension to the thoughts outlined in the explainer as they relate to actual user/developer intent.

Thanks in advance for taking my thoughts on this into account. I'm very much looking forward to the introduction of this specification and would be pleased to offer any further support or report from the field that would be of benefit to that coming to pass.

plinss commented 5 years ago

As @alice said, I'm not questioning the utility of the feature (see my original comment), just the shape of the API.

FWIW, the TAG has a few developers that use and create custom elements as well :-)

adamdbradley commented 5 years ago

I’m excited for this proposal and could see how it could reduce JS and simplify Ionic Framework components. The question about states for non-boolean is a great question, and initially I thought it should be required. However, I wasn’t able to come up with any scenarios within Ionic where boolean-only wouldn’t be sufficient. So I think it's a good suggestion to start out with boolean only, and possibly look into non-boolean states as phase 2, and evaluate if they're required.

domenic commented 5 years ago

@Westbrook yeah, just like any other CSS pseudo class, you can select on multiple of them. So :state(x):state(y) selects something in both states x and y, while :state(x), :state(y) selects something in either state x or y. Again, just like the built-ins :).

We can definitely add some examples using this to the explainer. Are there other existing CSS features that you think it would also be good to have examples of combining :state with?

Westbrook commented 5 years ago

I was predominately looking to ensure that :state(x):state(y) and its proper syntax was outlined very clearly. In that you don't get much from :valid:valid and in that some comments here and in other conversations had implied :state(x y) may have been possible. Having a difinitive "this is what can be done and how you'd do it" will be a great resource to point people.

As for combined examples, it seems easy to assume that things in the realm of :hover and :focus, or even the positional :first-child, should clearly work out of the box without directly stating, but including coverage for pseudo-elements :state(x)::before and relational selectors :not(:state(x)) might prevent confusion down the road.

alice commented 5 years ago

I'd just like to double check that @rakina, @domenic and @tkent-google noted @plinss' suggestion https://github.com/w3ctag/design-reviews/issues/428#issuecomment-542534030 about getting data demonstrating the need or lack thereof for non-boolean states.

domenic commented 5 years ago

@tkent-google and I spent some time discussing how to collect data on this subject, as well as the general problem space. Here is where we ended up:

Given that people are currently exposing state in custom elements by using attributes, and using attribute selectors, and this feature is meant to supplant that practice,

We don't think this is the right premise to be operating from. This feature is for states which depend on extrinsic factors such as user input, as the first paragraph of the explainer mentions. Component authors can't map such states to content attributes now, or do so but then end up with a fragile public interface---because component users can modify content attribute values.

To see what we mean, consider the built-in pseudo-class states :active, :hover, :valid, and :invalid, which can't be mapped to content attributes. I.e., what would aEl.removeAttribute("hover") mean? This feature is intended for cases like that.

(Side note: there are some older pseudo-classes like :checked which map more directly to content attributes, but newer elements like <dialog> and <details>, or element proposals like <std-switch>, do not create redundant pseudo-classes that are 1:1 with content attributes, instead allowing authors to do normal attribute selectors like dialog[open] { ... } or std-switch[on] { ... }.)

Regardless, we respect the call for research and did a bit of looking through existing custom element control libraries to find what types of internal states they expose.

So our conclusion is that we don't need to support non-boolean states and the current API using DOMTokenList is enough. If a component author want to expose a non-boolean state, it should be converted to multiple boolean states or a state change event can be provided for maximum flexibility.

That said, we recognize that in the overall mission of allowing "custom X" features to be as powerful as "built-in X" features, there is a missing gap for X = "custom pseudo-functions" like :dir(), :lang(), :is(), :not(), :where(), :has(), :nth-child(), :nth-col(), etc.

For the reasons explained in previous posts in this thread, we don't think these should be considered as an extension of the problem space being tackled here, i.e. in the same ways as custom states that reflect element-internal properties. As shown by the use case analysis on libraries (as well as analysis of the existing built-in HTML elements/CSS pseudo-functions), custom pseudo-functions don't provide an element-specific API. As such ElementInternals is not the right place for them.

We'd expect these to fit better with the Houdini efforts to make CSS more extensible and tied to JavaScript in general. For example:

All of these are not specific to a given custom element's implementation, but are powerful and interesting pieces of functionality that would be good to allow developers access to, as part of the general explain-CSS effort.

We hope this helps!

kenchris commented 4 years ago

https://wicg.github.io/custom-state-pseudo-class/

WebReflection commented 4 years ago

There are few things puzzling me regarding this proposal, and I'd like to list all points in here.

The meaning of a state

Even in the HTML world, components states can be defined either via classes or attributes, where attributes accept (at least) strings. Using a DOMTokenList to cover an element state seems like a very poor choice, especially if there is already a plan to allow :state(mode=collapsed) in the future.

Most used frameworks though, use state as basic JS object/map, allowing (at least) serializable data to be part of the state, so that while I agree the state should be handled internally, the developer experience I'd expect from this proposal would rather look like:

this._internals = this.attachInternals({status: "initial", uid: Math.random()});

// at any point in time ... updates *only* status, keep uid as it was
this._internals.update({status: "update"});

// at any point in time ... retrieve a value associated with the state
this._internals.get("status"); // "update"
this._internals.get("uid"); // the random number as string

// at any point in time ... replace the whole state, losing uid
this._internals.replace({status: "uid-less"});

// at any point in time, know if a state is set
this._internals.has("uid"); // false

// same as previously mentioned, for example purpose
this._internals.get("status"); // "uid-less"
this._internals.get("uid"); // null

// at any point in time, update a single entry of the state
this._internals.set("uid", Math.random());

this._internals.has("uid"); // true

Keeping the state in sync at once, and through an object, would make states handling per element much easier, and similar to what common frameworks do, only state keys that changed value should result into a CSS call/update/repaint

Benefits

About not allowing JS functions for states

This from @annevk is an interesting one:

I'm not enthused at all about running script while selector matching

The current proposal bases CSS info on JavaScript classes getters (get checked or set checked), meaning that :state(checked) will always invoke a function, so that I don't see, within the current proposal, why functions would not be allowed, when to retrieve states we need to define a function (accessor) anyway. However, my previous proposal would get rid of this issue all-together, because you don't need to expose getters and setters per each private state key, so the running function, within the CSS scope, happens only when, internally, the engine decides it should run, without touching JS at all.

About ditching built-ins Elements

Despite what WebKit does, there are numerous projects based on Custom Elements built-ins, plus the majority of VDom based frameworks don't care much about Custom Elements.

Confining this functionality only in classes that extend HTMLElement means this will have an incredibly slow adoption/value per effort ratio:

Using the suggested approach with Map could make this feature more broadly usable, and partially polyfillable, without needing to introspect classes, specially because classes might define this._internals as this.#internals, without exposing all keys handled by the internal class field.

About using classes and the "collision" argument

What this proposal would like to solve, seems to be an issue nobody using frameworks really have.

Moreover, it's not clear how exposing this.attachInternals() would solve conflicting states, as I believe el._internals = el.attachInternals() could be done at any time, as well as defining getters and setters at distance, either on the element, or its specific class prototype, if it's a custom element.

As summary, I am not sure this specification, as presented, would really solve something concretely for Web FE developers, but I'd be happy to know more about the clear broken use case this feature would like to fix.

Best Regards

karlhorky commented 4 years ago

@WebReflection I guess there's more discussion happening elsewhere (in https://github.com/w3c/webcomponents/issues/738), where @tkent-google mentioned that they are getting ready to ship in Chrome:

If this doesn't have any significant issues, I'd like to try to ship this in Google Chrome.

annevk commented 4 years ago

The current proposal bases CSS info on JavaScript classes getters

It doesn't. The CSS engine can inspect the element's states token list's token set directly.

WebReflection commented 4 years ago

@karlhorky thanks for the hint, but reading through that, this is my outcome:

I know none of these points are your direct fault, but I also believe it's already too late to reconsider this proposal.

@annevk about that ...

The CSS engine can inspect the element's states token list's token set directly.

It was silly/stupid of me thinking the JS would've played a role in the CSS side, but here is my quick feedback on the matter:

Thanks anyway for the clarification, it made instant sense as soon as I've read it, yet please consider above points to maybe improve the current documentation, or specification, sate.

Best Regards.

WebReflection commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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

WebReflection commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

I filed https://github.com/whatwg/html/issues/5166 for customized built-in elements.

tkent-google commented 4 years ago

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

plinss commented 4 years ago

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

plinss commented 4 years ago

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.