whatwg / html

HTML Standard
https://html.spec.whatwg.org/multipage/
Other
8.12k stars 2.67k forks source link

Add `class` & `for` aliases for `className` and `htmlFor` #9379

Open jakearchibald opened 1 year ago

jakearchibald commented 1 year ago

https://dom.spec.whatwg.org/#dom-element-classname https://html.spec.whatwg.org/multipage/forms.html#dom-label-htmlfor

I assume these were originally given unusual and inconsistent names to avoid reserved words, but it isn't an issue with modern JavaScript.

Could class be added to alias className, and for to alias htmlFor? The old names would be considered legacy.

It's a minor thing, but it's polyfillable, and removes a weird inconsistency on the platform.

tabatkins commented 1 year ago

Yeah, pre-ES5 those keywords were reserved unconditionally. But it's been a decade since they were usable for property access.

aminya commented 1 year ago

Well, Solidjs, as an example, doesn't expose these names in JSX. It simply uses class. The fact that React is doing this seems like an issue that can be fixed on their part. So I do not think the HTML/DOM specification needs to change because of that.

annevk commented 1 year ago

What's a good use case for className that is not better tackled with classList? (Also, this part of the request impacts whatwg/dom, not whatwg/html.)

gioboa commented 1 year ago

Well, Solidjs, as an example, doesn't expose these names in JSX. It simply uses class. The fact that React is doing this seems like an issue that can be fixed on their part. So I do not think the HTML/DOM specification needs to change because of that.

Same for Qwik we can use class and for in JSX templete and it's so cosy

jakearchibald commented 1 year ago

@annevk cases where you want to override previous changes, or setting an initial set of class names.

Gpx commented 1 year ago

Well, Solidjs, as an example, doesn't expose these names in JSX. It simply uses class. The fact that React is doing this seems like an issue that can be fixed on their part. So I do not think the HTML/DOM specification needs to change because of that.

I think this proposal should be considered on its own ignoring what JSX or other libraries are doing. When teaching how to manipulate the DOM this is often something that catches students off guard. IMO it makes sense to add an alias for these properties. It feels more natural to interact with for rather than htmlFor and the same goes for class.

jakearchibald commented 1 year ago

I think this proposal should be considered on its own ignoring what JSX or other libraries are doing.

Agreed. I didn't mention frameworks or JSX in the proposal for this reason.

patrickhlauke commented 1 year ago

I'd be in favour of the new shorter consistent properties, but for backwards compat likely need to keep the old ones as an alias (assuming that was the implicit intention with making them legacy)

jakearchibald commented 1 year ago

The proposal is to add aliases, not rename the properties.

patrickhlauke commented 1 year ago

Doh, sorry, it's right there in the OP and I managed to miss it.

hinell commented 1 year ago

You should clarify your post on whether you are talking about HTML Attributes or Element properties.

jakearchibald commented 1 year ago

@hinell added spec links. Although, I thought it would be obvious since className and htmlFor are properties, not attributes.

hinell commented 1 year ago

@jakearchibald class and for are attributes actually. The latter is used in lables. :eyes:

patrickhlauke commented 1 year ago

@hinell, please...we all know that. which is the whole point of this issue, adding aliases for matching properties...

tsukinoko-kun commented 1 year ago

What about functions like getElementsByClassName? Wouldn't it be even more confusing if you have to use document.getElementsByClassName("foo") but el.class is ok? Besides the confusion involving reflection.

bathos commented 1 year ago

Wouldn't it be even more confusing if you have to use document.getElementsByClassName("foo") but el.class is ok?

Content attribute reflection uses property names which are the same as the content attribute name modulo casing as a rule with only a handful of rare exceptions. These exceptions exist due to syntax constraints in JavaScript that were eliminated 12 years ago. They are now anomalies that easily lead to bugs — there’s no error thrown if you assign to elem.for or elem.class and it’s easy to miss that code trying to do this isn’t working as intended (especially in the for case). Also, a class content attribute’s value is a string of class name tokens (plural) separated by spaces, not just one, so className was a poor choice even for the workaround name.

The name getElementsByClassName is also a bit odd (it takes a spaced token list string too, not just one class name), but if someone tries to invoke getElementsByClass(), an error will be thrown as immediate feedback. This inconsistency is smaller, safer, and more discoverable than the inconsistency that already exists by not reflecting the content attribute with a property that has the same name.

mgol commented 1 year ago

I know it's technically out of scope but it's good to consider SVG here. SVG has className with different semantics than HTML className which leads to endless bugs that I keep bumping into; mostly in utils that accept both HTML & SVG nodes. Having the class property working only in HTML will lead to the same kind of bugs that we already have with className between HTML & SVG.

Would it be possible to discuss aligning this time and having SVG also support the class property but compatible with HTML?

jakearchibald commented 1 year ago

@mgol I think that's an excellent idea.

For others on the thread: in SVG, el.className is an SVGAnimatedString, meaning assignments need to be done differently vs html elements.

I'd forgotten about this, but it's definitely caught me out in the past.

The class property should be added to Element, and behave the same across all elements.

Edit: className is currently defined on Element, but SVGElement overrides it.

zcorpan commented 1 month ago

I checked httparchive (only sample_data which is 10k pages) for resources with .class = ... to see if something looks like it could break if we add class to Element.

https://docs.google.com/spreadsheets/d/1VoVQBocvtPzBJttHchZo00xXfQJeihTvFUse2Rw-NQo/edit?usp=sharing

It's hard to tell with static analysis if it's an element or not, but at least some of them look like they're elements, but also it looks like those generally intended to use className. For example

j.body.class="notranslate"

in https://code.jivosite.com/script/widget/bp3D59t7jk

Similarly in GitHub search: https://github.com/search?q=.class%3D+lang%3Ajavascript&type=code , for example:

                    button_li[i].className="current";
                    pic_li[i].className="current";
                }else{
                    button_li[i].class="but";
                    pic_li[i].className="pic";
                }

That said, there's some risk of breaking web content. Pages might set class on an element and not expect it to overwrite the class content attribute, or set class to something that's not a string and expect that object to be returned on getting.

Maybe use counters could be useful here?

Psychpsyo commented 1 month ago

Maybe use counters could be useful here?

At least in Firefox, we plan to put this behind a preference that's initially only enabled in Nightly.

jakearchibald commented 1 month ago

Nice! Is this going to be regular reflection even on SVG elements?

smaug---- commented 1 month ago

At least in Firefox, we plan to put this behind a preference that's initially only enabled in Nightly.

Well, first someone will need to take a look at the data zcorpan provided and investigate a bit more if this is possible at all. Are there some obvious, common cases which this would break?

WebReflection commented 1 month ago

I love the idea ... but ...

The proposal is to add aliases, not rename the properties.

for a migration / deprecation pattern it'd be better to make class and for direct accessors and alias the legacy, or better, if class is the new way one could override className accessors by providing a warning or a debug detail that it's legacy / deprecated and class should be used instead.

If class happens to be invoked passing through className because it's just an alias, then this approach would not work (I am looking mostly at React here).

If by overriding className accessor class remain usable without passing through that though, this whole concern of mine can be ignored.

Looking forward to update all my libraries that monkey-patch class to className once this is out, thanks for proposing this lovely update.

Psychpsyo commented 1 month ago

I love the idea ... but ...

The proposal is to add aliases, not rename the properties.

for a migration / deprecation pattern it'd be better to make class and for direct accessors and alias the legacy, or better, if class is the new way one could override className accessors by providing a warning or a debug detail that it's legacy / deprecated and class should be used instead.

If class happens to be invoked passing through className because it's just an alias, then this approach would not work (I am looking mostly at React here).

If by overriding className accessor class remain usable without passing through that though, this whole concern of mine can be ignored.

Looking forward to update all my libraries that monkey-patch class to className once this is out, thanks for proposing this lovely update.

The way that I'm currently imagining (and PRing) this is that .class and .className would be two separate attributes, they would just do the same thing. So something like this would work as a monkey patch/polyfill without causing any issues: (apart from the potential log spam, but that's besides the point)

if ("class" in Element.prototype) {
  Object.defineProperty(Element.prototype, "className", {
    get: function() {
      console.warn("Element.className is deprecated. Use Element.class instead.");
      return this.getAttribute("class");
    },
    set: function(value) {
      console.warn("Element.className is deprecated. Use Element.class instead.");
      this.setAttribute("class", value);
    }
  });
} else {
  Object.defineProperty(Element.prototype, "class", {
    get: function() {
      return this.getAttribute("class");
    },
    set: function(value) {
      this.setAttribute("class", value);
    }
  });
}

The exact same would go for .for and .htmlFor.

WebReflection commented 1 month ago

.class and .className would be two separate attributes, they would just do the same thing.

then my concern is gone, thanks. (unrelated P.S. you don't need that else in there)

zcorpan commented 1 month ago

Maybe use counters could be useful here?

I filed https://issues.chromium.org/u/1/issues/367992694 . If that is implemented, then when it's in the Stable release the next httparchive run should have accurate data on which pages set these properties (during page load). We can then query for those pages and sort by rank magnitude and assess compat impact by manually testing pages with a browser build where class and for are implemented.

Psychpsyo commented 1 month ago

Maybe use counters could be useful here?

I filed https://issues.chromium.org/u/1/issues/367992694 . If that is implemented, then when it's in the Stable release the next httparchive run should have accurate data on which pages set these properties (during page load). We can then query for those pages and sort by rank magnitude and assess compat impact by manually testing pages with a browser build where class and for are implemented.

I realize this is a bit early, but I've already prepared a build that implements class and for, here: https://chromium-review.googlesource.com/c/chromium/src/+/5866772

I'd be open to implement the use counters as well, but I might not get to it until later this week.

Also, from searching Github earlier, I found a lot of uses for for on label elements, with almost all of them looking like bugs that should be setting htmlFor instead.

zcorpan commented 1 month ago

@Psychpsyo nice!

Yeah, this change has the potential to make some content do what the author intended, which would be nice indeed. However, the web is big, and there's still a risk of breaking other sites with different expectations, which could negatively impact users and block shipping. So I think it's worthwhile to look for those needles.

smaug---- commented 1 month ago

Seems like we need some more data here from the use counters, so dropping agenda+ for now.