Fix change events for custom elements #22938

josepharhar · 2021-12-11T01:43:33Z

Summary

This patch makes onChange handlers listen to change events for custom elements. There was some odd behavior with the onChange handler and change events, but this patch will make it work for custom elements.

This fixes #22888 and I manually tested that it fixes Dan's example.

How did you test this change?

I tested it with the broken example and added tests in DOMPropertyOperations-test.js.

sizebot · 2021-12-11T01:47:40Z

Comparing: c09596c...93c9db1

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.52 kB	129.52 kB	=	41.53 kB	41.53 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	+0.03%	134.65 kB	134.69 kB	+0.07%	43.02 kB	43.04 kB
facebook-www/ReactDOM-prod.classic.js	=	427.76 kB	427.76 kB	=	78.53 kB	78.53 kB
facebook-www/ReactDOM-prod.modern.js	+0.06%	417.47 kB	417.74 kB	+0.06%	77.06 kB	77.11 kB
facebook-www/ReactDOMForked-prod.classic.js	=	427.76 kB	427.76 kB	=	78.53 kB	78.53 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 93c9db1

sebmarkbage · 2021-12-11T17:12:05Z

This would break the event order relative to other click events etc. Break stop propagation etc. Since the react ones are delegated.

That's partly why we wanted to keep special casing React events.

I suspect that the fix might be specific to onChange and that the rest would work well because you really shouldn't repurpose the built in names with custom events. Since they can bubble through.

We want to remove the synthetic event system all together and maybe it would make sense to put that in the same release. It's a much bigger change tho.

josepharhar · 2021-12-12T17:47:51Z

This would break the event order relative to other click events etc. Break stop propagation etc. Since the react ones are delegated.

That's partly why we wanted to keep special casing React events.

I suspect that the fix might be specific to onChange and that the rest would work well because you really shouldn't repurpose the built in names with custom events. Since they can bubble through.

We want to remove the synthetic event system all together and maybe it would make sense to put that in the same release. It's a much bigger change tho.

Ok, I agree with your points, thanks! I'll try changing react's event system for custom elements then.

Here's a more detailed example: https://codesandbox.io/s/sleepy-field-z7g83?file=/src/App.js
It highlights that there are two problems with the onChange example that Dan made:

React's event system only looks at input events for the onInput handler, and the onChange handler doesn't seem to do anything at all.
React's event system only looks at bubbling events, at least for input events.

If you have any advice for where to look to fix these things, it would be greatly appreciated! I couldn't even figure out why input and change are handled differently while debugging last week.

gaearon · 2021-12-12T18:42:01Z

If you have any advice for where to look to fix these things, it would be greatly appreciated! I couldn't even figure out why input and change are handled differently while debugging last week.

Do you want to look at ChangeEventPlugin? Only events “registered” by SimpleEventPlugin work “as is”. The rest (like onChange) have special behavior described in the plugins.

…bbles

josepharhar · 2021-12-25T02:59:28Z

Ok, instead of bypassing the react event system, I made a small change to SimpleEventPlugin to make onChange work for custom elements. I updated this PR description, and I tested that it works with Dan's example.

Do you want to look at ChangeEventPlugin? Only events “registered” by SimpleEventPlugin work “as is”. The rest (like onChange) have special behavior described in the plugins.

I tried messing with ChangeEventPlugin but I couldn't get it to work quite right, and it was more complex, and there is a long comment here suggesting that all the logic should be in SimpleEventPlugin anyway...

gaearon · 2022-01-12T20:11:27Z

Is change event the only special event? I would expect to have special events for each plugin. Why does this change (pardon the pun) only affect change?

josepharhar · 2022-01-13T01:07:11Z

After coming back to this after a while, I found that I could just modify ChangeEventPlugin to fix this despite my previous comment saying that I couldn't get it to work 😅 I pushed a commit to do this.

Is change event the only special event? I would expect to have special events for each plugin. Why does this change (pardon the pun) only affect change?

There is code in ChangeEventPlugin to only call onChange handlers for select and input elements:

react/packages/react-dom/src/events/plugins/ChangeEventPlugin.js

Lines 75 to 81 in c09596c

    
           function shouldUseChangeEvent(elem) { 
        
             const nodeName = elem.nodeName && elem.nodeName.toLowerCase(); 
        
             return ( 
        
               nodeName === 'select' || 
        
               (nodeName === 'input' && (elem: any).type === 'file') 
        
             ); 
        
           }

As far as I know, the other plugins don't have this constraint.

packages/react-dom/src/events/plugins/ChangeEventPlugin.js

gaearon · 2022-01-14T15:48:10Z

packages/react-dom/src/events/plugins/ChangeEventPlugin.js

+  if (
+    enableCustomElementPropertySupport &&
+    targetInst &&
+    isCustomComponent(targetInst.elementType, targetInst.pendingProps)


I think this should use memoizedProps instead which represent the ones that were committed. pendingProps represent last work in progress which could've been interrupted.

Also, I'm able to comment out this whole line and the tests still pass. I think it's because there is no test for the is="...." case which isCustomComponent is trying to catch.

Hmm. I understand we'd want <div is="my-custom-element"> to work with change event.

But if you do <input is="my-custom-element">, which branch of logic should we use? Should it use the "raw" behavior or the polyfill which we normally use for inputs?

@sebmarkbage do you have an opinion on this? seems like it's a bit simpler to turn off polyfilling when you do <input is="my-thing"> but that would mean that adding is changes semantics of your onChange handler. maybe that's ok because you decided to "go manual"? especially in the light of onChange->onInput future change?

Also, I'm able to comment out this whole line and the tests still pass. I think it's because there is no test for the is="...." case which isCustomComponent is trying to catch.

On my machine, if I comment out just the && isCustomComponent(...), 7 tests fail from various test files. If I undo the entire change here, 2 of the tests I added fail.

Hmm. I understand we'd want <div is="my-custom-element"> to work with change event.

Yes, <div is='my-custom-element'> should get this behavior as well.
I could copy all of the tests I added and use is instead of <my-custom-element /> to show that I used isCustomComponent, but you could say the same thing about all of the many custom element tests I added...

But if you do , which branch of logic should we use? Should it use the "raw" behavior or the polyfill which we normally use for inputs?

Based on what I read in DOMPluginEventSystem, ChangeEventPlugin is a "polyfill", right? The code I added is in ChangeEventPlugin, which means that <input> and custom elements should now be treated the same with regards to this behavior, right...?

I think this should use memoizedProps instead which represent the ones that were committed. pendingProps represent last work in progress which could've been interrupted.

Thanks, I was wondering which I should use! I have changed it to memoizedProps.

Based on what I read in DOMPluginEventSystem, ChangeEventPlugin is a "polyfill", right? The code I added is in ChangeEventPlugin, which means that and custom elements should now be treated the same with regards to this behavior, right...?

I meant the code of the polyfill below (look how this function is used). In the polyfill itself, we decide between using one of the events under the hood: change, input, or click. We decide that based on the type of the element. The code you added makes us always use change event for custom elements. That's good for <custom-element> or <div is="custom-element">. What I'm not sure about is <input is="custom-element">.

Should <input is="custom-element"> map onChange to the "raw" change event (which means it would work differently from <input> which in React maps onChange to input)? Or should it map onChange to the input/click events to mirror how <input> works?

In the latter case, it would mean that some custom elements still get special polyfill behavior that isn't 1:1 to the event names. That seems like it would be confusing. But if we go with former case (raw events), adding is to an existing input and having its onChange completely break (no longer called on every keystroke) also seems very confusing? Which is the behavior we'd get if it kept it raw like in this PR.

Using raw events for <input is="custom-element"> feels like simpler behavior to me but I'm not sure. There's also a potential intent to change it for <input> in distant future altogether: #9657.

@josepharhar Taking a step back, we should look at how controlled inputs generally work with <input is="my-custom-element">. Like how are you supposed to write a controlled input? One of the features of controlled inputs is that <input value=""> with no onChange handler won't allow typing. The same should be true for <input value="" is="my-custom-element">. Is that the case? If it already works, is it necessary to add onChange (like normal React) or onInput (closer to browser behavior) to make it respond to typing? That's what we need to decide on.

I just talked a bit with some LitElement folks about this, and they pointed out that Safari doesn't support the is attribute at all and that they advise against using it: https://bugs.webkit.org/show_bug.cgi?id=182671

I think that it would make the most sense to continue doing all of react's current behavior for form controls when they have the is attribute. Besides, the element and its shadowdom is still the same in this case when using is.

I pushed a commit to do this with tests.

sebmarkbage · 2022-01-15T18:50:47Z

We should have some tests for these cases too:

<div onChange={…} onInput={…}>
  <my-custom-element />
  <input />
  <input is=“my-custom-element” />
</div>

I.e. what is the bubbling expectations.

Also what about <my-custom-element onChange={…}><div /></my-custom-element>?

Where the target is the child div.

gaearon · 2022-01-18T18:44:37Z

packages/react-dom/src/__tests__/DOMPropertyOperations-test.js

+      customInput.dispatchEvent(new Event('click', {bubbles: true}));
+
+      expect(customOnInputHandler).toHaveBeenCalledTimes(
+        regularOnInputHandler.mock.calls.length,


This is kind of fragile. (What if we change some heuristic and all of these are 0.) Can you make the test a bit more specific? Also ideally you'd verify that the handler runs as early as possible, and not at the end of the function. So that if there's a timing difference, it is visible.

I added expectations after each dispatchEvent and put specific numbers in

josepharhar · 2022-01-18T20:25:31Z

We should have some tests for these cases too:
<div onChange={…} onInput={…}>
  <my-custom-element />
  <input />
  <input is=“my-custom-element” />
</div>
I.e. what is the bubbling expectations.

Thanks for pointing this out!
I pushed a test for this.

Also what about <my-custom-element onChange={…}><div /></my-custom-element>?

Where the target is the child div.

I added a test for this, and unfortunately it looks like change/onChange doesn't work, but input/onInput does work... I think I could fix this by walking all the way through the event path of the event and calling createAndAccumulateChangeEvent on each node, or go back to the SimpleEventPlugin approach, but I'm not sure what to do now...

gaearon · 2022-01-19T03:34:11Z

packages/react-dom/src/__tests__/DOMPropertyOperations-test.js

+
+      regularInput.dispatchEvent(new Event('input', {bubbles: true}));
+      expect(regularOnInputHandler).toHaveBeenCalledTimes(1);
+      expect(regularOnChangeHandler).toHaveBeenCalledTimes(0);


It looks like onChange never fires in any of these? Then it seems like this isn't testing the thing we care about. Since we care that input causes onChange.

Most likely what's missing is the actual change in the value. (And I think we also have to force React to "see" it with a hack). See how existing input tests do it:

react/packages/react-dom/src/__tests__/ReactDOMInput-test.js

Lines 226 to 229 in 69eb020

setUntrackedValue.call(instance.a, 'giraffe');

// This must use the native event dispatching. If we simulate, we will

// bypass the lazy event attachment system so we won't actually test this.

dispatchEventOnNode(instance.a, 'input');

I've pushed a few commits that address this and make the tests have a more realistic sequence of events.

gaearon

need to fix tests

josepharhar · 2022-01-19T21:17:55Z

Thanks for finishing this up!

* Bypass react event system for custom elements * Going to try fixing react event system instead * finally got it to call onChange, but perhaps too many times * update test * Removed ReactDOMComponent changes, now works but still doubles for bubbles * Maybe i should only support bubbling events * removed some old stuff * cleaned up changeeventplugin stuff * prettier, lint * removed changeeventplugin stuff * remove unneeded gate for onInput test * Go back to using ChangeEventPlugin * Add input+change test * lint * Move logic to shouldUseChangeEvent * use memoizedProps instead of pendingProps * Run form control behavior before custom element behavior * add bubbling test * forgot to append container to body * add child event target test * expand <input is=...> test expectations * Make tests more realistic * Add extra test * Add missing gating * Actually fix gating Co-authored-by: Dan Abramov <dan.abramov@me.com>

Bypass react event system for custom elements

b5b3a87

facebook-github-bot added the CLA Signed label Dec 11, 2021

josepharhar mentioned this pull request Dec 11, 2021

Bug: onChange event handlers don't work on custom elements #22888

Closed

gaearon requested a review from sebmarkbage December 11, 2021 16:43

josepharhar added 10 commits December 14, 2021 20:05

Going to try fixing react event system instead

e2bbc6a

finally got it to call onChange, but perhaps too many times

b194a40

update test

ace371d

Removed ReactDOMComponent changes, now works but still doubles for bu…

0f35e74

…bbles

Maybe i should only support bubbling events

82f2ac4

removed some old stuff

2cbbce3

cleaned up changeeventplugin stuff

952398d

prettier, lint

eb315d7

removed changeeventplugin stuff

9c99902

remove unneeded gate for onInput test

5d1e6da

josepharhar changed the title ~~Bypass react event system for custom elements~~ Fix change events for custom elements Dec 25, 2021

josepharhar added 4 commits January 12, 2022 17:36

Go back to using ChangeEventPlugin

a5db602

Add input+change test

9b77933

Merge with 5 weeks of commits

34c35e8

lint

dd6c930

gaearon reviewed Jan 13, 2022

View reviewed changes

packages/react-dom/src/events/plugins/ChangeEventPlugin.js Outdated Show resolved Hide resolved

Move logic to shouldUseChangeEvent

c4e2dbe

gaearon reviewed Jan 14, 2022

View reviewed changes

use memoizedProps instead of pendingProps

741db52

Run form control behavior before custom element behavior

2d17c97

gaearon reviewed Jan 18, 2022

View reviewed changes

josepharhar added 3 commits January 18, 2022 11:03

add bubbling test

3f345c8

forgot to append container to body

41a9ad7

add child event target test

81d4517

expand <input is=...> test expectations

69eb020

gaearon reviewed Jan 19, 2022

View reviewed changes

gaearon requested changes Jan 19, 2022

View reviewed changes

gaearon added 3 commits January 19, 2022 19:12

Make tests more realistic

6eaf85c

Add extra test

c58a742

Add missing gating

2ab52df

gaearon approved these changes Jan 19, 2022

View reviewed changes

Actually fix gating

93c9db1

gaearon merged commit 05a55a4 into facebook:main Jan 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix change events for custom elements #22938

Fix change events for custom elements #22938

josepharhar commented Dec 11, 2021 •

edited

Loading

sizebot commented Dec 11, 2021 •

edited

Loading

sebmarkbage commented Dec 11, 2021

josepharhar commented Dec 12, 2021

gaearon commented Dec 12, 2021

josepharhar commented Dec 25, 2021

gaearon commented Jan 12, 2022

josepharhar commented Jan 13, 2022

gaearon Jan 14, 2022

gaearon Jan 14, 2022

gaearon Jan 14, 2022 •

edited

Loading

gaearon Jan 14, 2022

josepharhar Jan 15, 2022

gaearon Jan 15, 2022 •

edited

Loading

gaearon Jan 18, 2022

josepharhar Jan 18, 2022

sebmarkbage commented Jan 15, 2022 •

edited

Loading

gaearon Jan 18, 2022

josepharhar Jan 18, 2022

josepharhar commented Jan 18, 2022

gaearon Jan 19, 2022

gaearon Jan 19, 2022 •

edited

Loading

gaearon left a comment

josepharhar commented Jan 19, 2022

	setUntrackedValue.call(instance.a, 'giraffe');
	// This must use the native event dispatching. If we simulate, we will
	// bypass the lazy event attachment system so we won't actually test this.
	dispatchEventOnNode(instance.a, 'input');

Fix change events for custom elements #22938

Fix change events for custom elements #22938

Conversation

josepharhar commented Dec 11, 2021 • edited Loading

Summary

How did you test this change?

sizebot commented Dec 11, 2021 • edited Loading

Critical size changes

Significant size changes

sebmarkbage commented Dec 11, 2021

josepharhar commented Dec 12, 2021

gaearon commented Dec 12, 2021

josepharhar commented Dec 25, 2021

gaearon commented Jan 12, 2022

josepharhar commented Jan 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon Jan 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage commented Jan 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josepharhar commented Jan 18, 2022

Choose a reason for hiding this comment

gaearon Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

josepharhar commented Jan 19, 2022

josepharhar commented Dec 11, 2021 •

edited

Loading

sizebot commented Dec 11, 2021 •

edited

Loading

gaearon Jan 14, 2022 •

edited

Loading

gaearon Jan 15, 2022 •

edited

Loading

sebmarkbage commented Jan 15, 2022 •

edited

Loading

gaearon Jan 19, 2022 •

edited

Loading