Use Change/Input events for checkable inputs #10830

jquense · 2017-09-26T14:09:53Z

This is pulled from #10238 and only changes the checkable inputs, switching them to use change/input events vs click which was (AFAICT) a IE8 only thing. No real bytes saved but it plugs an otherwise leaky bit of the abstraction

nhunzaker · 2017-09-26T15:37:05Z

@jquense Happy to do browser testing here. Is there anything special we should check for that might be missing from the DOM test fixtures?

jquense · 2017-09-26T15:51:37Z

I think we should have everything covered fixture wise. I just need need to confirm it works on ie9 feel free to test as well if you want :)

aweary · 2017-09-26T15:58:45Z

I've published the DOM fixtures with this change here:

http://change-event-checkable-inputs.surge.sh/

nhunzaker · 2017-10-06T11:00:12Z

src/renderers/dom/shared/eventPlugins/__tests__/ChangeEventPlugin-test.js

-    ReactTestUtils.SimulateNative.click(input);
-    ReactTestUtils.SimulateNative.click(input);
+    ReactTestUtils.SimulateNative.change(input);
+    ReactTestUtils.SimulateNative.change(input);
    expect(called).toBe(1);
  });


Does this drop support for clicking inputs to change them? Is this possibly breaking?

nhunzaker · 2017-10-06T11:47:24Z

I've verified this using our (maybe official?) support matrix:

Pass

IE 9, 10, 11
Edge 14
Chrome 49, 61
Firefox 56, ESR (52.4.0)
Safari 10, 11

Fail

Chrome 41 Radio input clicks
Safari 7.1 Radio input clicks

These failures are also issues on master. Clicking a radio button fires change:

Testing Notes:

We need to add the Array::includes polyfill for the range input arrow test (but it the test works correctly with the polyfill in IE10-11). I'll send out a separate PR for that.

Desktop Safari does not allow arrow keys for range inputs. Pretty lame for a11y, and confusing in the test case, but I'm not sure how to enable it. I don't think this is an issue on us. I'll followup with a PR that adds a note about Safari in this test.

jquense · 2017-10-06T12:50:20Z

Thanks Nathan! I wonder if the failures are browser bugs with labels and inputs double firing? We don't dedupe input changes in some safari versions as well because it's not supported

nhunzaker · 2017-10-16T18:45:34Z

Following up:

A fix for array.includes is on master.
Safari allows arrow key input if you focus the knob via a keyboard (tab focus). Master has an update that includes a button to force focus for easier testing
The browser bug for radio buttons is not present after I rebase master

I've pushed that up to:
http://react-radio-click-rebased.surge.sh/input-change-events

However: now there's in an issue in Safari <= 9 and Chrome <= 43 where the range input fires both a change event and input event. This is only viewable with the updates on master which fix the array.includes support issue in these browsers.

Arrow key presses fire a change event and input event in all versions of Safari and Chrome (that I've tested), but something in these old browsers is causing both events to produce a synthetic change event; duplicately firing on arrow key presses.

I'm curious if some sort of legacy change event behavior for IE is also getting picked up by these old browsers, and if we can avoid that pathway to prevent the duplication.

Could we get away with only subscribing to the change input type in IE?

jquense · 2017-11-06T17:00:01Z

@nhunzaker thanks for all the deets here. Is there a sense of whether anything in this PR regressed something or are we seeing some weird behavior that has always existed but we are just seeing?

nhunzaker · 2017-11-08T00:07:30Z

@jquense This started in React 15.6.0 (works in React 15.5.4), so it's not directly related to this PR. I'd be okay with syncing up later to figure out why this is happening, and how much we care about it.

nhunzaker

Rebased with master, and I've redeployed the fixtures:
http://react-radio-click-rebased.surge.sh/input-change-events

I'm concerned about this as a breaking change. Apps using SimulateNative.click will need to switch over to SimulateNative.change. How common is that?

Still, I am attracted to the idea of reducing the complexity of this code.

How does everyone else feel?

nhunzaker · 2017-11-08T00:14:07Z

packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.js

-    ReactTestUtils.SimulateNative.click(input);
-    ReactTestUtils.SimulateNative.click(input);
+    ReactTestUtils.SimulateNative.change(input);
+    ReactTestUtils.SimulateNative.change(input);
    expect(called).toBe(1);
  });


Am I correct to assume this is breaking change? Is this a change we are comfortable sending out in a minor release?

that's a good question, technically i'd be relying on a private implementation detail, but i can see it breaking tests for folks as well. I defer to what the team thing on this

I am comfortable with this, but it should go out in a minor. I'd hate for tests to fail on a patch.

gaearon · 2018-01-05T16:33:30Z

Could you rebase this please? I'd be okay with doing it in a minor since

it's an implementation detail
in a way it kinda "fixes" it to the expected behavior

nhunzaker · 2018-01-10T16:41:10Z

packages/react-dom/src/shared/ReactDOMInjection.js

+import SVGDOMPropertyConfig from './SVGDOMPropertyConfig';
+
+DOMProperty.injection.injectDOMPropertyConfig(HTMLDOMPropertyConfig);
+DOMProperty.injection.injectDOMPropertyConfig(SVGDOMPropertyConfig);


Should this be added?

i don't think so, seems like a rebase issue

nhunzaker

Looks good overall, just curious about that newly added injection file.

jquense · 2018-04-18T21:22:18Z

@nhunzaker long time coming but i've updated this :P

pull-bot · 2018-04-18T21:32:12Z

ReactDOM: size: -0.0%, gzip: -0.0%

Details of bundled changes.

Comparing: 1609cf3...f0664d5

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	-0.1%	-0.1%	642.55 KB	642.23 KB	150.91 KB	150.82 KB	UMD_DEV
react-dom.production.min.js	-0.0%	-0.0%	96.17 KB	96.13 KB	31.18 KB	31.16 KB	UMD_PROD
react-dom.development.js	-0.1%	-0.1%	638.69 KB	638.37 KB	149.74 KB	149.65 KB	NODE_DEV
react-dom.production.min.js	-0.0%	-0.0%	96.16 KB	96.12 KB	30.72 KB	30.7 KB	NODE_PROD
ReactDOM-dev.js	-0.0%	-0.1%	645.94 KB	645.62 KB	148.06 KB	147.97 KB	FB_WWW_DEV
ReactDOM-prod.js	-0.0%	-0.0%	277.82 KB	277.69 KB	52.13 KB	52.11 KB	FB_WWW_PROD
react-dom.profiling.min.js	-0.0%	-0.1%	97.29 KB	97.25 KB	31.11 KB	31.08 KB	NODE_PROFILING
ReactDOM-profiling.js	-0.0%	-0.0%	281.11 KB	280.98 KB	52.85 KB	52.83 KB	FB_WWW_PROFILING

Generated by 🚫 dangerJS

nhunzaker · 2018-04-19T00:42:30Z

@jquense doing a build and some testing, but this looks good from a surface level. I'll report back!

nhunzaker · 2018-04-19T01:17:20Z

This looks good! Tested in:

IE 9, 10, 11
EDGE 16
iOS 9 Safari (iPhone 6)
Chrome Android 6
Safari 10+ is okay
Firefox 59, 51, 47
Chrome 65, 43

It's worth noting that older versions of Chrome and Safari exhibit a behavior where the changes double fire on range inputs when you use arrow keys:

Chrome 41; Safari 7.1, 8, 9

However this is already the case on master.

This is good to go by me!

gaearon · 2018-04-19T01:26:57Z

Does this help us fix any bugs or is this purely to simplify?

nhunzaker · 2018-04-19T01:43:07Z

I read this as a simplification with a net benefit towards correctness. With this PR, React no longer needs to listen, at the cost of correctness, to click events in IE8. It's a small course-correction that falls more in line with the expected event behavior (or my interpretation of it anyway).

Beyond that I'm not sure of any particular bug fixes this addresses.

jquense · 2018-04-19T12:46:12Z

My guess is it'll "fix" #12643 as well but I'm still looking into why that behavior even happens. Beyond that tho it closes a leaky bit of the abstraction, that confuses folks occasionally

philipp-spiess · 2018-07-15T11:52:13Z

This will also fix #9023. We currently have no way of knowing if preventDefault() was called inside the onClick handler since it will fire after the ChangeEventPlugin does the job so onChange will always be fired.

@jquense You can cherry pick this commit to add a test. It's based on a rebased version of your PR.

One thing I found out while rebasing is that some tests no longer pass after my cleanup in #13176. I think this is due to jsdom not firing the change event properly this might need more investigation. I worked around this by triggering the change event manually: philipp-spiess@b39cc3f

gaearon · 2018-08-02T19:30:53Z

Can you bring this up to date please?

jquense · 2018-08-02T21:32:12Z

updated!

gaearon · 2018-08-02T21:34:00Z

How likely is it to break anything? Would this only affect unit tests that dispatch events?

jquense · 2018-08-02T23:04:20Z

The only breaking behavior i can think is of, is you are triggering native change events on checkbox and not expecting them to flow through components. But that would be really weird to do...

IDK, it really shouldn't break anyone, this is an implementation detail, but any time we change the logic here, someone complains it doesn't work in an old version of selenium running on a toaster or something. We've made more aggressive changes in this space on minor bumps before and it's been ok :P

gaearon · 2018-08-03T01:14:25Z

The only breaking behavior i can think is of, is you are triggering native change events on checkbox and not expecting them to flow through components. But that would be really weird to do...

What if you're triggering clicks and do expect them to flow through?

jquense · 2018-08-03T02:10:24Z

Generally native clicks also trigger change events on inputs, so it shouldn't be a problem: http://jsfiddle.net/j3g16ydw/2/

gaearon · 2018-08-03T11:53:15Z

Looks like CI fails?

philipp-spiess · 2018-09-03T19:35:04Z

This will also fix #13459.

We might need to add a warning though when click is fired but change is not (always remembering #13462 here). This case also seems to be happening in the failing unit tests.

gaearon · 2018-09-03T19:56:52Z

I think this is one of those things we'll need to put behind a flag.

stale · 2020-01-10T08:52:31Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

stale · 2020-01-17T08:59:04Z

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!

facebook-github-bot added the CLA Signed label Sep 26, 2017

nhunzaker reviewed Oct 6, 2017

View reviewed changes

nhunzaker force-pushed the radio-click branch from 5fcaf86 to 607945d Compare November 8, 2017 00:10

nhunzaker reviewed Nov 8, 2017

View reviewed changes

jquense mentioned this pull request Jan 5, 2018

Do we need to listen for click on change events? #11971

Closed

2 tasks

jquense force-pushed the radio-click branch from 607945d to 4378080 Compare January 5, 2018 17:01

nhunzaker reviewed Jan 10, 2018

View reviewed changes

nhunzaker suggested changes Jan 10, 2018

View reviewed changes

jquense mentioned this pull request Apr 18, 2018

onChange doesn't fire if a capture-phase document listener for the underlying native event calls setState() #12643

Closed

Use change or input events for checkable inputs

7b1bbe6

jquense force-pushed the radio-click branch from 40d266e to 7b1bbe6 Compare April 18, 2018 21:21

nhunzaker approved these changes Apr 19, 2018

View reviewed changes

jquense mentioned this pull request Jul 9, 2018

Needless checkbox warning when using onClick instead of onChange #13171

Closed

Merge branch 'master' into radio-click

f0664d5

philipp-spiess mentioned this pull request Aug 22, 2018

React onChange events not bubbling up on unmount #13459

Closed

Akshat-Jain approved these changes Sep 14, 2018

View reviewed changes

stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 10, 2020

stale bot closed this Jan 17, 2020

Use Change/Input events for checkable inputs #10830

Use Change/Input events for checkable inputs #10830

Conversation

jquense commented Sep 26, 2017

nhunzaker commented Sep 26, 2017

jquense commented Sep 26, 2017

aweary commented Sep 26, 2017

nhunzaker Oct 6, 2017

Choose a reason for hiding this comment

nhunzaker commented Oct 6, 2017 • edited Loading

jquense commented Oct 6, 2017

nhunzaker commented Oct 16, 2017 • edited Loading

jquense commented Nov 6, 2017

nhunzaker commented Nov 8, 2017

nhunzaker left a comment

Choose a reason for hiding this comment

nhunzaker Nov 8, 2017

Choose a reason for hiding this comment

jquense Nov 16, 2017

Choose a reason for hiding this comment

nhunzaker Nov 16, 2017

Choose a reason for hiding this comment

gaearon commented Jan 5, 2018

nhunzaker Jan 10, 2018

Choose a reason for hiding this comment

jquense Jan 10, 2018

Choose a reason for hiding this comment

nhunzaker left a comment

Choose a reason for hiding this comment

jquense commented Apr 18, 2018

pull-bot commented Apr 18, 2018 • edited Loading

react-dom

nhunzaker commented Apr 19, 2018

nhunzaker commented Apr 19, 2018

gaearon commented Apr 19, 2018

nhunzaker commented Apr 19, 2018 • edited Loading

jquense commented Apr 19, 2018

philipp-spiess commented Jul 15, 2018 • edited Loading

gaearon commented Aug 2, 2018 • edited Loading

jquense commented Aug 2, 2018

gaearon commented Aug 2, 2018

jquense commented Aug 2, 2018

gaearon commented Aug 3, 2018

jquense commented Aug 3, 2018

gaearon commented Aug 3, 2018

philipp-spiess commented Sep 3, 2018

gaearon commented Sep 3, 2018

stale bot commented Jan 10, 2020

stale bot commented Jan 17, 2020

nhunzaker commented Oct 6, 2017 •

edited

Loading

nhunzaker commented Oct 16, 2017 •

edited

Loading

pull-bot commented Apr 18, 2018 •

edited

Loading

nhunzaker commented Apr 19, 2018 •

edited

Loading

philipp-spiess commented Jul 15, 2018 •

edited

Loading

gaearon commented Aug 2, 2018 •

edited

Loading