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

Add class & for aliases for className and htmlFor #9379

Open
jakearchibald opened this issue Jun 4, 2023 · 29 comments · May be fixed by #10630
Open

Add class & for aliases for className and htmlFor #9379

jakearchibald opened this issue Jun 4, 2023 · 29 comments · May be fixed by #10630
Labels
addition/proposal New features or enhancements needs compat analysis

Comments

@jakearchibald
Copy link
Contributor

jakearchibald commented Jun 4, 2023

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
Copy link
Contributor

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

@aminya
Copy link

aminya commented Jun 4, 2023

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.

@domenic domenic added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest labels Jun 5, 2023
@annevk
Copy link
Member

annevk commented Jun 5, 2023

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
Copy link

gioboa commented Jun 5, 2023

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
Copy link
Contributor Author

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

@Gpx
Copy link

Gpx commented Jun 5, 2023

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
Copy link
Contributor Author

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
Copy link
Member

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 jakearchibald changed the title Add class & for aliases for className and htmlFor Add class & for aliases for className and htmlFor Jun 5, 2023
@jakearchibald
Copy link
Contributor Author

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

@patrickhlauke
Copy link
Member

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

@hinell
Copy link

hinell commented Jun 5, 2023

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

@jakearchibald
Copy link
Contributor Author

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

@hinell
Copy link

hinell commented Jun 5, 2023

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

@patrickhlauke
Copy link
Member

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

@tsukinoko-kun
Copy link

tsukinoko-kun commented Jun 5, 2023

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
Copy link

bathos commented Jun 6, 2023

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
Copy link

mgol commented Jun 6, 2023

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
Copy link
Contributor Author

jakearchibald commented Jun 6, 2023

@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.

Psychpsyo added a commit to Psychpsyo/html that referenced this issue Sep 17, 2024
This adds for as an alias for htmlFor to be
more consistent and intuitive. This is now possible
since 'for' is no longer a globally reserved word
in Javascript.
It also updates one example to match the new
'class' alias for 'className' from the DOM spec.

fixes whatwg#9379
@Psychpsyo Psychpsyo linked a pull request Sep 17, 2024 that will close this issue
5 tasks
@smaug---- smaug---- added the agenda+ To be discussed at a triage meeting label Sep 17, 2024
@zcorpan
Copy link
Member

zcorpan commented Sep 17, 2024

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
Copy link

Psychpsyo commented Sep 17, 2024

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
Copy link
Contributor Author

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

@smaug----
Copy link

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
Copy link
Contributor

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
Copy link

Psychpsyo commented Sep 18, 2024

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
Copy link
Contributor

.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
Copy link
Member

zcorpan commented Sep 18, 2024

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
Copy link

Psychpsyo commented Sep 18, 2024

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
Copy link
Member

zcorpan commented Sep 18, 2024

@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---- smaug---- removed the agenda+ To be discussed at a triage meeting label Oct 1, 2024
@smaug----
Copy link

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

@zcorpan zcorpan added needs compat analysis and removed needs implementer interest Moving the issue forward requires implementers to express interest labels Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs compat analysis
Development

Successfully merging a pull request may close this issue.