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.

Sign up for GitHub

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

Jump to bottom

Use Change/Input events for checkable inputs #10830

Closed
wants to merge 2 commits into from
Closed

Use Change/Input events for checkable inputs #10830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7b1bbe6
Use change or input events for checkable inputs
jquense Apr 18, 2018
f0664d5
Merge branch 'master' into radio-click
jquense Aug 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/react-dom/src/client/inputValueTracking.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,6 @@ export function updateValueIfChanged(node: ElementWithValueTracker) {
if (!tracker) {
return true;
}

const lastValue = tracker.getValue();
const nextValue = getValueFromNode(node);
if (nextValue !== lastValue) {
Expand Down
16 changes: 4 additions & 12 deletions packages/react-dom/src/events/ChangeEventPlugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,10 +207,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 &&
Expand All @@ -219,12 +216,6 @@ function shouldUseClickEvent(elem) {
);
}

function getTargetInstForClickEvent(topLevelType, targetInst) {
if (topLevelType === TOP_CLICK) {
return getInstIfValueChanged(targetInst);
}
}

function getTargetInstForInputOrChangeEvent(topLevelType, targetInst) {
if (topLevelType === TOP_INPUT || topLevelType === TOP_CHANGE) {
return getInstIfValueChanged(targetInst);
Expand Down Expand Up @@ -275,12 +266,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,
Expand Down
73 changes: 34 additions & 39 deletions packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,13 +104,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);
});
Expand All @@ -128,13 +122,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);
});
Expand All @@ -153,18 +141,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);
});
Expand Down Expand Up @@ -243,22 +228,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);
});

Expand All @@ -282,8 +274,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);
});

Expand Down Expand Up @@ -312,23 +304,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);
Expand Down