Add custom element property support behind a flag

[josepharhar]

Summary

This implements the solution I've been proposing and discussing for #11347 which does an "in" check on the custom element to determine if we should be assigning to the element's property instead of its attribute, as well as adding support for custom events by making JSX attributes which start with "on" event listeners.

This doesn't implement the whenDefined -> reapply properties behavior we have been discussing, I'm planning on addressing that later.

Test Plan

I've been keeping my test site up to date and this patch includes several automated tests, but I still haven't gotten around to using the fixtures/attribute-behavior test to look for more behavior changes yet.

[facebook-github-bot]

Hi @josepharhar!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[josepharhar]

@sebmarkbage please take a look!
Out of curiosity, is there a way I can enable the flag at runtime without recompiling all of react with it hard coded to true?

[sizebot]

Comparing: c7917fe...4bd3b44

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	129.89 kB	129.89 kB	=	41.62 kB	41.63 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	+0.28%	134.64 kB	135.02 kB	+0.28%	43.00 kB	43.12 kB
facebook-www/ReactDOM-prod.classic.js	=	428.08 kB	428.08 kB	=	78.60 kB	78.60 kB
facebook-www/ReactDOM-prod.modern.js	+0.27%	416.64 kB	417.77 kB	+0.28%	76.91 kB	77.12 kB
facebook-www/ReactDOMForked-prod.classic.js	=	428.08 kB	428.08 kB	=	78.60 kB	78.60 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-experimental/react-dom/cjs/react-dom-server-legacy.browser.production.min.js	+0.36%	32.19 kB	32.31 kB	+0.44%	10.80 kB	10.85 kB
oss-experimental/react-dom/umd/react-dom-server-legacy.browser.production.min.js	+0.35%	32.38 kB	32.49 kB	+0.46%	10.94 kB	10.99 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.production.min.js	+0.35%	32.85 kB	32.96 kB	+0.35%	11.24 kB	11.28 kB
oss-experimental/react-dom/umd/react-dom-server.browser.production.min.js	+0.35%	33.01 kB	33.13 kB	+0.43%	11.35 kB	11.39 kB
facebook-www/ReactDOMServer-prod.modern.js	+0.34%	75.56 kB	75.82 kB	+0.36%	16.17 kB	16.23 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.node.production.min.js	+0.32%	35.94 kB	36.05 kB	+0.32%	12.06 kB	12.09 kB
oss-experimental/react-dom/cjs/react-dom-server.node.production.min.js	+0.32%	36.09 kB	36.21 kB	+0.35%	12.23 kB	12.27 kB
oss-experimental/react-dom/umd/react-dom.production.min.js	+0.28%	134.69 kB	135.07 kB	+0.32%	43.65 kB	43.79 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	+0.28%	134.64 kB	135.02 kB	+0.28%	43.00 kB	43.12 kB
facebook-www/ReactDOM-prod.modern.js	+0.27%	416.64 kB	417.77 kB	+0.28%	76.91 kB	77.12 kB
facebook-www/ReactDOMForked-prod.modern.js	+0.27%	416.64 kB	417.77 kB	+0.28%	76.91 kB	77.13 kB
oss-experimental/react-dom/umd/react-dom.profiling.min.js	+0.26%	143.63 kB	144.01 kB	+0.25%	46.39 kB	46.50 kB
oss-experimental/react-dom/cjs/react-dom.profiling.min.js	+0.26%	144.24 kB	144.62 kB	+0.30%	45.86 kB	45.99 kB
facebook-www/ReactDOM-profiling.modern.js	+0.25%	448.47 kB	449.60 kB	+0.26%	82.35 kB	82.56 kB
facebook-www/ReactDOMForked-profiling.modern.js	+0.25%	448.47 kB	449.60 kB	+0.26%	82.35 kB	82.57 kB
oss-experimental/react-dom/umd/react-dom-server.browser.development.js	+0.24%	238.15 kB	238.72 kB	+0.27%	56.00 kB	56.16 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.development.js	+0.24%	226.83 kB	227.37 kB	+0.27%	55.41 kB	55.56 kB
oss-experimental/react-dom/cjs/react-dom-server.node.development.js	+0.24%	226.84 kB	227.38 kB	+0.27%	55.29 kB	55.44 kB
oss-experimental/react-dom/umd/react-dom-server-legacy.browser.development.js	+0.24%	240.72 kB	241.29 kB	+0.26%	56.34 kB	56.49 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.browser.development.js	+0.24%	229.25 kB	229.79 kB	+0.26%	55.74 kB	55.89 kB
facebook-www/ReactDOMServer-dev.modern.js	+0.23%	228.69 kB	229.23 kB	+0.27%	54.80 kB	54.95 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.node.development.js	+0.23%	230.86 kB	231.40 kB	+0.26%	56.19 kB	56.33 kB
oss-experimental/react-dom/cjs/react-dom.development.js	+0.21%	1,026.12 kB	1,028.28 kB	+0.24%	230.29 kB	230.84 kB
oss-experimental/react-dom/umd/react-dom.development.js	+0.21%	1,077.13 kB	1,079.37 kB	+0.27%	232.79 kB	233.41 kB
facebook-www/ReactDOMForked-dev.modern.js	+0.20%	1,100.83 kB	1,103.06 kB	+0.23%	243.91 kB	244.46 kB
facebook-www/ReactDOM-dev.modern.js	+0.20%	1,100.83 kB	1,103.06 kB	+0.23%	243.91 kB	244.47 kB

Generated by 🚫 dangerJS against 4bd3b44

[sebmarkbage]

It's ok to set this to __EXPERIMENTAL__ and it'll be on in the experimental builds but the the "next" stable release channel which 18 is on.

That way it'll have some test coverage in CI on and you get a build to experiment with.

[josepharhar]

Awesome!!
Should I set it to __EXPERIMENTAL__ in all of the separate feature flag files I had to add this flag to?

[sebmarkbage]

Oops stamped too soon.

[gaearon]

What about htmlFor? Is that needed here?

[josepharhar]

className is special because all elements have a builtin className property which triggers the 'className' in node check when rendering on the client - meaning that rendering on the client will always use a property instead of an attribute for className. Without this, one of the automated tests was getting errors (I think during hydration?) due to this mismatch.
Alternatively we could make it so that when client rendering className is always set as an attribute instead of a property, but I think this makes more sense and it's what Preact is already doing.

[sebmarkbage]

Yea this make sense because htmlFor doesn't have any semantics on custom element which are just Generic HTML elements.

There are a number of other ones that are case sensitive for similar reasons. contentEditable, tabIndex, etc. They probably work if used correctly, but it would be nice to add a warning if they use the wrong case.

There are some that just won't work like offsetHeight etc. Since those exist as properties but are read-only. Probably should ensure we have a nice warning about those and why they don't work. So that if they're rendered on the server, it doesn't look like they work.

Similarly there are some we have to be careful with like innerHTML and innerText. It shouldn't pass through as a property to custom elements - even in production so it can't be a DEV only error. Could be worth a test.

[sebmarkbage]

I'm surprised innerText and textContent aren't reserved properties in React. Doesn't seem right.

[josepharhar]

There are a number of other ones that are case sensitive for similar reasons. contentEditable, tabIndex, etc. They probably work if used correctly, but it would be nice to add a warning if they use the wrong case.

contentEditable is interesting, it looks like the attribute can be either contentEditable or contenteditable, they both work, and the reflected object property is contentEditable.
So I guess if someone writes <my-custom-element contentEditable={true} /> or <my-custom-element contenteditable={true} /> they should both work fine, right...?

There are some that just won't work like offsetHeight etc. Since those exist as properties but are read-only. Probably should ensure we have a nice warning about those and why they don't work. So that if they're rendered on the server, it doesn't look like they work.

I mean, you can assign to offsetHeight from script and the browser doesn't complain about it... but yeah I guess it would be weird to have a state where you can assign to an attribute called offsetHeight when rendering on the server, but you can't on the client because it will always trigger the 'offsetHeight' in node check...

Should I consider every global attribute and every built in property?

[josepharhar]

For the read only properties like offsetHeight, I just pushed a commit which emits the warning: 1a093e5

However, the test fails because it throws an exception when assigning to the read only properties - but this exception doesn't occur in the browser or in JSDOM... what kind of browser/DOM setup is being used in these tests?

[sebmarkbage]

“use strict” throws, right?

[josepharhar]

Oh wow, today I learned...
What do you think we should do? Remove the test? Make it somehow get rid of the 'use strict'?

[sebmarkbage]

I believe if you render it on the server and hydrate it wouldn’t error.

The reason for the warning would be so that if you render it on the server, during development you should know not to.

So you can test hydrating it.

[josepharhar]

Ah, that makes sense!
I replaced the client rendering check with a hydration check and made a test for it.

[sebmarkbage]

This seems a bit slow to do for every property. Likely will need some refactoring. However, is this check also equivalent on the built-ins?

[josepharhar]

Are you referring to name in (node: any) or (typeof (node: any)[name]) === 'boolean'?

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[josepharhar]

Is there an up-to-date document that describes the final strategy and a spec of what we do in each case?

This is the last document I made: https://docs.google.com/document/d/1wwL_YZ5TnYorEEr2Lkeh9rgbnsynDXXI7tCejVcpE6I/edit#
There is one behavior that has diverged from this proposal: the usage of the type of the variable when deciding to use addEventListener or not. I originally proposed that we do the same thing as preact here (forward everything starting with "on" to addEventListener), but if I remember correctly Sebastian felt strongly that we shouldn't reserve everything starting with "on" for event listeners. Jason Miller also felt strongly about this the other way, so maybe it's something that I can revisit/discuss more after merging this PR - maybe more users will chime in if they get to test it out.

I added another test for the behavior I discussed in the doc about not removing attributes when assigning to properties.

Missing SSR test coverage. We need to understand the exact current behavior, including edge cases. (E.g. that we don't currently set properties on hydration.)

I added some tests for this, thanks!

Changing any line fails some test. This ensures we know what's intentional or not, and avoids regressions when refactoring or changing approaches.
Any intentional design decisions ("X behaves in a Y way") has a test associated with it. So that we can distinguish intentional decisions and bugs.

I skimmed the patch as well as the doc, and I believe everything is being tested now

[gaearon]

so maybe it's something that I can revisit/discuss more after merging this PR - maybe more users will chime in if they get to test it out.

seems reasonable

[trusktr]

This is not flexible enough for custom element users (#11347 (comment)).

In today's modern Custom Elements world, users need choice:

• Sometimes users will want the default behavior of this PR (JS properties if detected, fallback to attributes)
• Sometimes, users always want to use an attribute regardless if the custom element supports both, for styling purposes (some-element[foo="bar"] { ... }).
• Quite often a custom element author writes elements that map attributes to JS properties, but despite that, and despite the JS property existing, users need to set the attribute in order to trigger the custom element's own internal style (ShadowRoot contains :host[some-prop] { ... }). This is a requirement set by custom element authors.
• Sometimes an element does not have a property, but the user needs to set properties because something outside of the custom element is reading them as data.
• Sometimes custom element authors must set attributes regardless because something outside of the custom elements is reading them as data (HTMX, Alpine.js, etc)

This is why Lit's html (cc @justinfagnani), Solid's JSX, Solid's html (cc @ryansolid), and others, were designed to allow these use cases be explicitly controllable by the user.

I've experienced all of these use cases in at most two apps. They are not rare.

Here's a Lit example:

return html`
  <some-element
    ?some-boolean=${this.someBoolean}
    another-booolean
    some-attribute=${this.someString}
    .some-property=${this.someNonString}
  ></some-element>
`

Solid uses attr:foo= and prop:foo= syntax in both JSX and html to give users control.

For the above reasons, this solution does not meet the requirements that custom element users have, and this solution is incomplete by today's standards.

Custom element users need a complete solution to this. If not with syntax, then with something runtime like "directives". F.e. foo={attr(someValue)} setting an attribute vs foo={prop(someValue)} setting a JS prop.

[trusktr]

@jfbrennan since you were curious, see that ☝️. Custom element users must be responsible for knowing how custom elements are used, including whether to set an attribute or a property, and no system can accurately determine this. We're still gonna need to resort to useRef and imperative code, missing out on declarative JSX benefits for cases not covered.

I bet someone can solve this for 100% of the needed cases by making an html template tag with syntax like Lit's html, but that returns React vdom, and internally it creates components that wrap useRef for the cases that JSX does not support.

[jfbrennan]

That's too bad, but actually a good thing long term. Proprietary frameworks that consider themselves the center of the universe will die, those that have already caught up with the needs of the WC era will live on.

[jmleep]

Maybe I'm making a wrong assumption, but I installed react@experimental and react-dom@experimental (looking at this guidance on docs), but I couldn't get any of my custom events to fire by prefixing the event name with on. It looks like this is scheduled to go out in the next major release, is that still correct?

[josepharhar]

Can you provide a minimal example?

[Amrender-Singh]

Hi @josepharhar ,

I'm reporting a potential bug I encountered while testing custom elements with the latest versions of react@experimental and react-dom@experimental.

Observed behavior:

I created a custom element with a callback function bound to a custom event.
When the event is triggered on the custom element, the callback does not fire.

Example:
https://stackblitz.com/edit/vitejs-vite-vpqkrw?file=src%2FApp.jsx

When I attach the event listener directly without using react, the callback is getting triggered.

[eps1lon]

@Amrender-Singh I can't get this to work with plain HTML either: https://stackblitz.com/edit/vitejs-vite-vuvkyc?file=index.html,customElement.js

I believe the problem is that you're dispatching the custom event at this which is not the instance of the custom element. There's a question on StackOverflow (https://stackoverflow.com/a/70789592) that describes the same problem I believe.

[eps1lon]

@trusktr The cases you believe are currently not covered are best raised in a new issue for visibility. Though I believe they can be covered by how React implements custom elements in a way that seems to me more like idiomatic HTML:

Sometimes, users always want to use an attribute regardless if the custom element supports both, for styling purposes (some-element[foo="bar"] { ... }).

Isn't the solution here either using Custom states and custom state pseudo-class CSS selectors or attribute reflection? Just like you must use :checked instead of [checked] to style an HTML checkbox, you'd have to use some-element:state(foo). For the cases where you need [foo="bar"] wouldn't you have implemented attribute reflection for the property in your custom element anyway?

Sometimes an element does not have a property, but the user needs to set properties because something outside of the custom element is reading them as data.

You'd have to use a ref here for built-in browser components as well, no? Seems like this is a feature request that's not specific to custom elements.

[Amrender-Singh]

@eps1lon It is working fine in HTML+JS, the main issue with your example was you were not listening to the event correctly, I think in order to listen for custom events using vanilla JS you need to register listener using addEventListener.

https://open-wc.org/guides/knowledge/events/
https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener

Example:
https://plnkr.co/edit/UT2jGd67TA6ePyiH?open=lib%2Fscript.js

[eps1lon]

The event handler for custom elements is case sensitive.

So if you dispatch customclick your React event handler should be oncustomclick not onCustomClick. We have tests for this in this PR. This also matches how it works in HTML where addEventListener('Customclick') would not catch new CustomEvent('customclick').

I'll search for some more context in the spec and other frameworks. Though at first I would expect that onCustomclick would at least work out of the box since that's also how it works for standard browser events.

You should also drop the on from the event type: new Event(type) -> on${type} to match how browser events and handlers are named.

[eps1lon]

Not sure if we should special case the first letter here and map onSomecustomevent to somecustomevent. That's now how you'd write in HTML but then you'd also not write onClick either. Maybe it's better to break the capitalisation magic early so that people don't assume that we can magically also map onCustomClick to customclick.

The main confusion here was that React removes the on from the event listener not that the listener is case-sensitive.

[jmleep]

How does this work with events that use dashes or colons?

custom-event

[eps1lon]

How does this work with events that use dashes or colons?

custom-event

events with dashes already have a test: https://github.com/facebook/react/pull/22184/files#diff-db98d094dc4f5b1a9c32b4da4df310370707408a929762868d44bf1acd515923R188

events with colon do not work as JSX attribute due to not being supported by React's JSX though we don't have a test if they're spread to props as an object e.g. <div {...{ 'oncustom:click': () => console.log(1) }} />. @jmleep could you add a test to packages/react-dom/src/__tests__/DOMPropertyOperations-test.js just like for the other event handler test but with <div {...{ 'oncustom:click': () => console.log(1) }} /> instead?

[trusktr]

Isn't the solution here either using Custom states and custom state pseudo-class CSS selectors or attribute reflection?

That's one possible solution that a custom element author (not the user) has.

For the cases where you need [foo="bar"] wouldn't you have implemented attribute reflection for the property in your custom element anyway?

As a custom elements user (not an author) there is no guarantee that every author of all custom elements that ever exist will follow any rules specifically for React, so manual control is needed.

Because of this, any templating system that exists (not only React JSX) and supports custom elements needs to give users the ability to choose whether they set HTML attributes vs JS properties.

Lit's html template tag is the reference implementation for this type of support, as it was designed for custom elements from the start. Solid's html template tag follows in second place, currently missing boolean attribute syntax.

See here for a better description of the problem and how Lit and Solid support use cases:

• Boolean attributes solidjs/solid#2158

See here for the custom-elements-everywhere test that will document support or lack of support for this across frameworks:

• New test: control over setting attributes, JS properties, events, and boolean attributes webcomponents/custom-elements-everywhere#2352

---

Sometimes an element does not have a property, but the user needs to set properties because something outside of the custom element is reading them as data.

You'd have to use a ref here for built-in browser components as well, no? Seems like this is a feature request that's not specific to custom elements.

Sometimes you just need to set a JS property and not an attribute, for whatever reason (you might not need a ref at all, you just want to set the JS property in the JSX, and something else like a jQuery plugin outside of the component could be reading the JS properties).

"Use a ref" essentially means "use plain JS (instead of React JSX)". Plain JS has always worked, but that defeats the goal here, which is to not have to break out of React JSX into plain JS (because that's much more convenient for the developer experience).