From c7129ce1f0bba7d04e9d5fce806aaeed40730b17 Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Tue, 22 Nov 2016 14:39:30 -0800 Subject: [PATCH] Stopgap fix for element disabling (#8387) Fix for #8308. This is a bad hack -- EventPluginHub.getListener isn't even DOM-specific -- but this works for now and lets us release 15.4.1. --- scripts/fiber/tests-failing.txt | 13 +++-- scripts/fiber/tests-passing.txt | 15 +++--- .../shared/eventPlugins/SimpleEventPlugin.js | 24 +-------- .../__tests__/SimpleEventPlugin-test.js | 53 ++++++++++++++++--- .../shared/shared/event/EventPluginHub.js | 41 +++++++++++++- 5 files changed, 101 insertions(+), 45 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 1da530c083bd7..23726bdeccb9a 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -71,14 +71,19 @@ src/renderers/dom/shared/eventPlugins/__tests__/ChangeEventPlugin-test.js src/renderers/dom/shared/eventPlugins/__tests__/SimpleEventPlugin-test.js * A non-interactive tags clicks bubble when disabled -* should not forward clicks when it starts out disabled +* triggers parent captured click events when target is a child of a disabled elements +* should forward clicks when it becomes not disabled * should not forward clicks when it becomes disabled -* should not forward clicks when it starts out disabled +* should work correctly if the listener is changed +* should forward clicks when it becomes not disabled * should not forward clicks when it becomes disabled -* should not forward clicks when it starts out disabled +* should work correctly if the listener is changed +* should forward clicks when it becomes not disabled * should not forward clicks when it becomes disabled -* should not forward clicks when it starts out disabled +* should work correctly if the listener is changed +* should forward clicks when it becomes not disabled * should not forward clicks when it becomes disabled +* should work correctly if the listener is changed src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js * should control a value in reentrant events diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 080b89a7dfcbd..9a54b3c411b83 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -696,18 +696,17 @@ src/renderers/dom/shared/eventPlugins/__tests__/SelectEventPlugin-test.js src/renderers/dom/shared/eventPlugins/__tests__/SimpleEventPlugin-test.js * A non-interactive tags click when disabled +* does not register a click when clicking a child of a disabled element +* triggers click events for children of disabled elements +* triggers captured click events for children of disabled elements * should forward clicks when it starts out not disabled -* should forward clicks when it becomes not disabled -* should work correctly if the listener is changed +* should not forward clicks when it starts out disabled * should forward clicks when it starts out not disabled -* should forward clicks when it becomes not disabled -* should work correctly if the listener is changed +* should not forward clicks when it starts out disabled * should forward clicks when it starts out not disabled -* should forward clicks when it becomes not disabled -* should work correctly if the listener is changed +* should not forward clicks when it starts out disabled * should forward clicks when it starts out not disabled -* should forward clicks when it becomes not disabled -* should work correctly if the listener is changed +* should not forward clicks when it starts out disabled * does not add a local click to interactive elements * adds a local click listener to non-interactive elements diff --git a/src/renderers/dom/shared/eventPlugins/SimpleEventPlugin.js b/src/renderers/dom/shared/eventPlugins/SimpleEventPlugin.js index 76a8e529c2b45..40fe66f11d98e 100644 --- a/src/renderers/dom/shared/eventPlugins/SimpleEventPlugin.js +++ b/src/renderers/dom/shared/eventPlugins/SimpleEventPlugin.js @@ -138,25 +138,6 @@ var topLevelEventsToDispatchConfig: {[key: TopLevelTypes]: DispatchConfig} = {}; topLevelEventsToDispatchConfig[topEvent] = type; }); -function isInteractive(tag) { - return ( - tag === 'button' || tag === 'input' || - tag === 'select' || tag === 'textarea' - ); -} - -function shouldPreventMouseEvent(inst) { - if (inst) { - var disabled = inst._currentElement && inst._currentElement.props.disabled; - - if (disabled) { - return isInteractive(inst._tag); - } - } - - return false; -} - var SimpleEventPlugin: PluginModule = { eventTypes: eventTypes, @@ -232,10 +213,7 @@ var SimpleEventPlugin: PluginModule = { case 'topMouseDown': case 'topMouseMove': case 'topMouseUp': - // Disabled elements should not respond to mouse events - if (shouldPreventMouseEvent(targetInst)) { - return null; - } + // TODO: Disabled elements should not respond to mouse events /* falls through */ case 'topMouseOut': case 'topMouseOver': diff --git a/src/renderers/dom/shared/eventPlugins/__tests__/SimpleEventPlugin-test.js b/src/renderers/dom/shared/eventPlugins/__tests__/SimpleEventPlugin-test.js index 9aa5670e448eb..5f4a533ea4de2 100644 --- a/src/renderers/dom/shared/eventPlugins/__tests__/SimpleEventPlugin-test.js +++ b/src/renderers/dom/shared/eventPlugins/__tests__/SimpleEventPlugin-test.js @@ -17,16 +17,14 @@ describe('SimpleEventPlugin', function() { var ReactDOM; var ReactTestUtils; - var onClick = jest.fn(); + var onClick; function expectClickThru(element) { - onClick.mockClear(); ReactTestUtils.SimulateNative.click(ReactDOM.findDOMNode(element)); expect(onClick.mock.calls.length).toBe(1); } function expectNoClickThru(element) { - onClick.mockClear(); ReactTestUtils.SimulateNative.click(ReactDOM.findDOMNode(element)); expect(onClick.mock.calls.length).toBe(0); } @@ -40,6 +38,8 @@ describe('SimpleEventPlugin', function() { React = require('React'); ReactDOM = require('ReactDOM'); ReactTestUtils = require('ReactTestUtils'); + + onClick = jest.fn(); }); it('A non-interactive tags click when disabled', function() { @@ -53,7 +53,48 @@ describe('SimpleEventPlugin', function() { ); var child = ReactDOM.findDOMNode(element).firstChild; - onClick.mockClear(); + ReactTestUtils.SimulateNative.click(child); + expect(onClick.mock.calls.length).toBe(1); + }); + + it('does not register a click when clicking a child of a disabled element', function() { + var element = ReactTestUtils.renderIntoDocument( + + ); + var child = ReactDOM.findDOMNode(element).querySelector('span'); + + ReactTestUtils.SimulateNative.click(child); + expect(onClick.mock.calls.length).toBe(0); + }); + + it('triggers click events for children of disabled elements', function() { + var element = ReactTestUtils.renderIntoDocument( + + ); + var child = ReactDOM.findDOMNode(element).querySelector('span'); + + ReactTestUtils.SimulateNative.click(child); + expect(onClick.mock.calls.length).toBe(1); + }); + + it('triggers parent captured click events when target is a child of a disabled elements', function() { + var element = ReactTestUtils.renderIntoDocument( +
+ +
+ ); + var child = ReactDOM.findDOMNode(element).querySelector('span'); + + ReactTestUtils.SimulateNative.click(child); + expect(onClick.mock.calls.length).toBe(1); + }); + + it('triggers captured click events for children of disabled elements', function() { + var element = ReactTestUtils.renderIntoDocument( + + ); + var child = ReactDOM.findDOMNode(element).querySelector('span'); + ReactTestUtils.SimulateNative.click(child); expect(onClick.mock.calls.length).toBe(1); }); @@ -124,10 +165,6 @@ describe('SimpleEventPlugin', function() { describe('iOS bubbling click fix', function() { // See http://www.quirksmode.org/blog/archives/2010/09/click_event_del.html - beforeEach(function() { - onClick.mockClear(); - }); - it('does not add a local click to interactive elements', function() { var container = document.createElement('div'); diff --git a/src/renderers/shared/shared/event/EventPluginHub.js b/src/renderers/shared/shared/event/EventPluginHub.js index e39c1d3a6a99a..efd13e71693d4 100644 --- a/src/renderers/shared/shared/event/EventPluginHub.js +++ b/src/renderers/shared/shared/event/EventPluginHub.js @@ -48,6 +48,31 @@ var executeDispatchesAndReleaseTopLevel = function(e) { return executeDispatchesAndRelease(e, false); }; +function isInteractive(tag) { + return ( + tag === 'button' || tag === 'input' || + tag === 'select' || tag === 'textarea' + ); +} + +function shouldPreventMouseEvent(name, type, props) { + switch (name) { + case 'onClick': + case 'onClickCapture': + case 'onDoubleClick': + case 'onDoubleClickCapture': + case 'onMouseDown': + case 'onMouseDownCapture': + case 'onMouseMove': + case 'onMouseMoveCapture': + case 'onMouseUp': + case 'onMouseUpCapture': + return !!(props.disabled && isInteractive(type)); + default: + return false; + } +} + /** * This is a unified interface for event plugins to be installed and configured. * @@ -97,13 +122,25 @@ var EventPluginHub = { */ getListener: function(inst, registrationName) { var listener; + + // TODO: shouldPreventMouseEvent is DOM-specific and definitely should not + // live here; needs to be moved to a better place soon if (typeof inst.tag === 'number') { // TODO: This is not safe because we might want the *other* Fiber's // props depending on which is the current one. - listener = inst.memoizedProps[registrationName]; + const props = inst.memoizedProps; + listener = props[registrationName]; + if (shouldPreventMouseEvent(registrationName, inst.type, props)) { + return null; + } } else { - listener = inst._currentElement.props[registrationName]; + const props = inst._currentElement.props; + listener = props[registrationName]; + if (shouldPreventMouseEvent(registrationName, inst._currentElement.type, props)) { + return null; + } } + invariant( !listener || typeof listener === 'function', 'Expected %s listener to be a function, instead got type %s',