From 7b1bbe6befc269e62abccafd5e14777278fa0100 Mon Sep 17 00:00:00 2001 From: Jason Quense Date: Wed, 18 Apr 2018 17:21:42 -0400 Subject: [PATCH] Use change or input events for checkable inputs --- .../src/client/inputValueTracking.js | 1 - .../react-dom/src/events/ChangeEventPlugin.js | 16 +--- .../ChangeEventPlugin-test.internal.js | 73 +++++++++---------- 3 files changed, 38 insertions(+), 52 deletions(-) diff --git a/packages/react-dom/src/client/inputValueTracking.js b/packages/react-dom/src/client/inputValueTracking.js index 732a77beee891..e4c5c07faec5b 100644 --- a/packages/react-dom/src/client/inputValueTracking.js +++ b/packages/react-dom/src/client/inputValueTracking.js @@ -122,7 +122,6 @@ export function updateValueIfChanged(node: ElementWithValueTracker) { if (!tracker) { return true; } - const lastValue = tracker.getValue(); const nextValue = getValueFromNode(node); if (nextValue !== lastValue) { diff --git a/packages/react-dom/src/events/ChangeEventPlugin.js b/packages/react-dom/src/events/ChangeEventPlugin.js index 5e0dcbb3be4b9..77edaedbc40db 100644 --- a/packages/react-dom/src/events/ChangeEventPlugin.js +++ b/packages/react-dom/src/events/ChangeEventPlugin.js @@ -197,10 +197,7 @@ function getTargetInstForInputEventPolyfill(topLevelType, targetInst) { /** * SECTION: handle `click` event */ -function shouldUseClickEvent(elem) { - // Use the `click` event to detect changes to checkbox and radio inputs. - // This approach works across all browsers, whereas `change` does not fire - // until `blur` in IE8. +function isCheckableInput(elem) { const nodeName = elem.nodeName; return ( nodeName && @@ -209,12 +206,6 @@ function shouldUseClickEvent(elem) { ); } -function getTargetInstForClickEvent(topLevelType, targetInst) { - if (topLevelType === 'topClick') { - return getInstIfValueChanged(targetInst); - } -} - function getTargetInstForInputOrChangeEvent(topLevelType, targetInst) { if (topLevelType === 'topInput' || topLevelType === 'topChange') { return getInstIfValueChanged(targetInst); @@ -271,12 +262,13 @@ const ChangeEventPlugin = { getTargetInstFunc = getTargetInstForInputEventPolyfill; handleEventFunc = handleEventsForInputEventPolyfill; } - } else if (shouldUseClickEvent(targetNode)) { - getTargetInstFunc = getTargetInstForClickEvent; + } else if (isCheckableInput(targetNode)) { + getTargetInstFunc = getTargetInstForInputOrChangeEvent; } if (getTargetInstFunc) { const inst = getTargetInstFunc(topLevelType, targetInst); + if (inst) { const event = createAndAccumulateChangeEvent( inst, diff --git a/packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.internal.js b/packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.internal.js index e5f8969ba119a..064a42dadb254 100644 --- a/packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.internal.js +++ b/packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.internal.js @@ -79,13 +79,7 @@ describe('ChangeEventPlugin', () => { container, ); - // Secretly, set `checked` to false, so that dispatching the `click` will - // make it `true` again. Thus, at the time of the event, React should not - // consider it a change from the initial `true` value. - setUntrackedChecked.call(node, false); - node.dispatchEvent( - new MouseEvent('click', {bubbles: true, cancelable: true}), - ); + node.dispatchEvent(new Event('change', {bubbles: true, cancelable: true})); // There should be no React change events because the value stayed the same. expect(called).toBe(0); }); @@ -103,13 +97,7 @@ describe('ChangeEventPlugin', () => { container, ); - // Secretly, set `checked` to true, so that dispatching the `click` will - // make it `false` again. Thus, at the time of the event, React should not - // consider it a change from the initial `false` value. - setUntrackedChecked.call(node, true); - node.dispatchEvent( - new MouseEvent('click', {bubbles: true, cancelable: true}), - ); + node.dispatchEvent(new Event('change', {bubbles: true, cancelable: true})); // There should be no React change events because the value stayed the same. expect(called).toBe(0); }); @@ -128,18 +116,15 @@ describe('ChangeEventPlugin', () => { ); expect(node.checked).toBe(false); - node.dispatchEvent( - new MouseEvent('click', {bubbles: true, cancelable: true}), - ); - // Note: unlike with text input events, dispatching `click` actually - // toggles the checkbox and updates its `checked` value. + setUntrackedChecked.call(node, true); + node.dispatchEvent(new Event('change', {bubbles: true, cancelable: true})); + expect(node.checked).toBe(true); expect(called).toBe(1); expect(node.checked).toBe(true); - node.dispatchEvent( - new MouseEvent('click', {bubbles: true, cancelable: true}), - ); + setUntrackedChecked.call(node, false); + node.dispatchEvent(new Event('change', {bubbles: true, cancelable: true})); expect(node.checked).toBe(false); expect(called).toBe(2); }); @@ -218,22 +203,29 @@ describe('ChangeEventPlugin', () => { container, ); - // Set the value, updating the "current" value that React tracks to true. + // Set it programmatically. input.checked = true; - // Under the hood, uncheck the box so that the click will "check" it again. - setUntrackedChecked.call(input, false); - input.dispatchEvent( - new MouseEvent('click', {bubbles: true, cancelable: true}), - ); + + // Even if a DOM event fires, React sees that the real input value now + // ('bar') is the same as the "current" one we already recorded. + input.dispatchEvent(new Event('change', {bubbles: true, cancelable: true})); expect(input.checked).toBe(true); - // We don't expect a React event because at the time of the click, the real - // checked value (true) was the same as the last recorded "current" value - // (also true). + // In this case we don't expect to get a React event. expect(called).toBe(0); - // However, simulating a normal click should fire a React event because the - // real value (false) would have changed from the last tracked value (true). - input.dispatchEvent(new Event('click', {bubbles: true, cancelable: true})); + // However, we can simulate user toggling the checkbox by calling + // the underlying setter. + setUntrackedChecked.call(input, false); + + // Now, when the event fires, the real input value (false) differs from the + // "current" one we previously recorded (false). + input.dispatchEvent(new Event('input', {bubbles: true, cancelable: true})); + expect(input.checked).toBe(false); + // In this case React should fire an event for it. + expect(called).toBe(1); + + // Verify again that extra events without real changes are ignored. + input.dispatchEvent(new Event('change', {bubbles: true, cancelable: true})); expect(called).toBe(1); }); @@ -257,8 +249,8 @@ describe('ChangeEventPlugin', () => { ); setUntrackedChecked.call(input, true); - input.dispatchEvent(new Event('click', {bubbles: true, cancelable: true})); - input.dispatchEvent(new Event('click', {bubbles: true, cancelable: true})); + input.dispatchEvent(new Event('change', {bubbles: true, cancelable: true})); + input.dispatchEvent(new Event('change', {bubbles: true, cancelable: true})); expect(called).toBe(1); }); @@ -287,23 +279,26 @@ describe('ChangeEventPlugin', () => { const option2 = div.childNodes[1]; // Select first option. + setUntrackedChecked.call(option1, true); option1.dispatchEvent( - new Event('click', {bubbles: true, cancelable: true}), + new Event('change', {bubbles: true, cancelable: true}), ); expect(called1).toBe(1); expect(called2).toBe(0); // Select second option. + setUntrackedChecked.call(option2, true); option2.dispatchEvent( - new Event('click', {bubbles: true, cancelable: true}), + new Event('change', {bubbles: true, cancelable: true}), ); expect(called1).toBe(1); expect(called2).toBe(1); // Select the first option. // It should receive the React change event again. + setUntrackedChecked.call(option1, true); option1.dispatchEvent( - new Event('click', {bubbles: true, cancelable: true}), + new Event('change', {bubbles: true, cancelable: true}), ); expect(called1).toBe(2); expect(called2).toBe(1);