From fc957a9456285c2cabf61e90abce30f9e0c41780 Mon Sep 17 00:00:00 2001 From: skiritsis Date: Tue, 28 Nov 2017 00:40:32 +0200 Subject: [PATCH 1/7] Rewrite SelectEventPlugin-test to test behavior using Public API --- .../SelectEventPlugin-test.internal.js | 48 ++++++++++--------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js b/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js index 51795d9b48fd..0766e7d115b5 100644 --- a/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js +++ b/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js @@ -58,34 +58,36 @@ describe('SelectEventPlugin', () => { expect(mouseup).toBe(null); }); - it('should extract if an `onSelect` listener is present', () => { - class WithSelect extends React.Component { - render() { - return ; - } - } - - var cb = jest.fn(); - - var rendered = ReactTestUtils.renderIntoDocument( - , + it('should register event only if the `onSelect` listener is present', () => { + var select = jest.fn(); + var onSelect = event => { + expect(typeof event).toBe('object'); + expect(event.type).toBe('select'); + expect(event.target).toBe(node); + select(event.currentTarget); + }; + + var childContainer = document.createElement('div'); + var childNode = ReactDOM.render( + , + childContainer, ); - var node = ReactDOM.findDOMNode(rendered); + document.body.appendChild(childContainer); - node.selectionStart = 0; - node.selectionEnd = 0; + var node = ReactDOM.findDOMNode(childNode); node.focus(); + expect(select.mock.calls.length).toBe(0); - var focus = extract(node, 'topFocus'); - expect(focus).toBe(null); + var nativeEvent = document.createEvent('Event'); + nativeEvent.initEvent('mousedown', true, true); + childNode.dispatchEvent(nativeEvent); + expect(select.mock.calls.length).toBe(0); - var mousedown = extract(node, 'topMouseDown'); - expect(mousedown).toBe(null); + nativeEvent = document.createEvent('Event'); + nativeEvent.initEvent('mouseup', true, true); + childNode.dispatchEvent(nativeEvent); + expect(select.mock.calls.length).toBe(1); - var mouseup = extract(node, 'topMouseUp'); - expect(mouseup).not.toBe(null); - expect(typeof mouseup).toBe('object'); - expect(mouseup.type).toBe('select'); - expect(mouseup.target).toBe(node); + document.body.removeChild(childContainer); }); }); From e4fbaa42dcdf5f3730b544c4bb6a531467d0df6d Mon Sep 17 00:00:00 2001 From: skiritsis Date: Tue, 28 Nov 2017 01:37:14 +0200 Subject: [PATCH 2/7] Minor refactor --- .../SelectEventPlugin-test.internal.js | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js b/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js index 0766e7d115b5..2799245ced8e 100644 --- a/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js +++ b/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js @@ -16,6 +16,8 @@ var ReactTestUtils; var SelectEventPlugin; describe('SelectEventPlugin', () => { + var container; + function extract(node, topLevelEvent) { return SelectEventPlugin.extractEvents( topLevelEvent, @@ -32,6 +34,14 @@ describe('SelectEventPlugin', () => { // TODO: can we express this test with only public API? ReactDOMComponentTree = require('../../client/ReactDOMComponentTree'); SelectEventPlugin = require('../SelectEventPlugin').default; + + container = document.createElement('div'); + document.body.appendChild(container); + }); + + afterEach(() => { + document.body.removeChild(container); + container = null; }); it('should skip extraction if no listeners are present', () => { @@ -67,27 +77,21 @@ describe('SelectEventPlugin', () => { select(event.currentTarget); }; - var childContainer = document.createElement('div'); - var childNode = ReactDOM.render( + var node = ReactDOM.render( , - childContainer, + container, ); - document.body.appendChild(childContainer); - - var node = ReactDOM.findDOMNode(childNode); node.focus(); expect(select.mock.calls.length).toBe(0); var nativeEvent = document.createEvent('Event'); nativeEvent.initEvent('mousedown', true, true); - childNode.dispatchEvent(nativeEvent); + node.dispatchEvent(nativeEvent); expect(select.mock.calls.length).toBe(0); nativeEvent = document.createEvent('Event'); nativeEvent.initEvent('mouseup', true, true); - childNode.dispatchEvent(nativeEvent); + node.dispatchEvent(nativeEvent); expect(select.mock.calls.length).toBe(1); - - document.body.removeChild(childContainer); }); }); From ae31befe7f9eccc2aa0d95a6dcaea0b2a0e8a116 Mon Sep 17 00:00:00 2001 From: skiritsis Date: Tue, 28 Nov 2017 01:52:01 +0200 Subject: [PATCH 3/7] Make sure that we test that "focus" event is ignored Use newer API when creating events --- .../__tests__/SelectEventPlugin-test.internal.js | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js b/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js index 2799245ced8e..cf305b7a6751 100644 --- a/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js +++ b/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js @@ -82,15 +82,22 @@ describe('SelectEventPlugin', () => { container, ); node.focus(); + + var nativeEvent = new MouseEvent('focus', { + bubbles: true, + cancelable: true, + }); + node.dispatchEvent(nativeEvent); expect(select.mock.calls.length).toBe(0); - var nativeEvent = document.createEvent('Event'); - nativeEvent.initEvent('mousedown', true, true); + nativeEvent = new MouseEvent('mousedown', { + bubbles: true, + cancelable: true, + }); node.dispatchEvent(nativeEvent); expect(select.mock.calls.length).toBe(0); - nativeEvent = document.createEvent('Event'); - nativeEvent.initEvent('mouseup', true, true); + nativeEvent = new MouseEvent('mouseup', {bubbles: true, cancelable: true}); node.dispatchEvent(nativeEvent); expect(select.mock.calls.length).toBe(1); }); From 8d7fff5a48b2e3920ac7da142448fbcbeec5c046 Mon Sep 17 00:00:00 2001 From: skiritsis Date: Wed, 29 Nov 2017 23:19:37 +0200 Subject: [PATCH 4/7] Rewrote the other test to use Public API as well --- .../SelectEventPlugin-test.internal.js | 55 +++++++------------ 1 file changed, 21 insertions(+), 34 deletions(-) diff --git a/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js b/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js index cf305b7a6751..afb903f10801 100644 --- a/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js +++ b/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js @@ -11,29 +11,13 @@ var React; var ReactDOM; -var ReactDOMComponentTree; -var ReactTestUtils; -var SelectEventPlugin; describe('SelectEventPlugin', () => { var container; - function extract(node, topLevelEvent) { - return SelectEventPlugin.extractEvents( - topLevelEvent, - ReactDOMComponentTree.getInstanceFromNode(node), - {target: node}, - node, - ); - } - beforeEach(() => { React = require('react'); ReactDOM = require('react-dom'); - ReactTestUtils = require('react-dom/test-utils'); - // TODO: can we express this test with only public API? - ReactDOMComponentTree = require('../../client/ReactDOMComponentTree'); - SelectEventPlugin = require('../SelectEventPlugin').default; container = document.createElement('div'); document.body.appendChild(container); @@ -44,31 +28,34 @@ describe('SelectEventPlugin', () => { container = null; }); - it('should skip extraction if no listeners are present', () => { - class WithoutSelect extends React.Component { - render() { - return ; - } - } + it('should verify that `onSelect` fails to fire when no `topMouseUp` has yet occurred to unset the flag', () => { + var select = jest.fn(); + var onSelect = event => select(event.currentTarget); - var rendered = ReactTestUtils.renderIntoDocument(); - var node = ReactDOM.findDOMNode(rendered); + var node = ReactDOM.render( + , + container, + ); node.focus(); - // It seems that .focus() isn't triggering this event in our test - // environment so we need to ensure it gets set for this test to be valid. - var fakeNativeEvent = function() {}; - fakeNativeEvent.target = node; - ReactTestUtils.simulateNativeEventOnNode('topFocus', node, fakeNativeEvent); + var nativeEvent = new MouseEvent('focus', { + bubbles: true, + cancelable: true, + }); + node.dispatchEvent(nativeEvent); - var mousedown = extract(node, 'topMouseDown'); - expect(mousedown).toBe(null); + nativeEvent = new MouseEvent('mousedown', { + bubbles: true, + cancelable: true, + }); + node.dispatchEvent(nativeEvent); - var mouseup = extract(node, 'topMouseUp'); - expect(mouseup).toBe(null); + nativeEvent = new MouseEvent('keyup', {bubbles: true, cancelable: true}); + node.dispatchEvent(nativeEvent); + expect(select.mock.calls.length).toBe(0); }); - it('should register event only if the `onSelect` listener is present', () => { + it('should verify that `onSelect` fires when the listener is present', () => { var select = jest.fn(); var onSelect = event => { expect(typeof event).toBe('object'); From e1856a019fc5ac91640ce12d132c9b4b2700145e Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 29 Nov 2017 22:05:38 +0000 Subject: [PATCH 5/7] Tweak the test --- .../SelectEventPlugin-test.internal.js | 47 ++++++++++++------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js b/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js index afb903f10801..d7827c7cff47 100644 --- a/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js +++ b/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js @@ -28,34 +28,49 @@ describe('SelectEventPlugin', () => { container = null; }); - it('should verify that `onSelect` fails to fire when no `topMouseUp` has yet occurred to unset the flag', () => { + // See https://github.com/facebook/react/pull/3639 for details. + it('does not get confused when dependent events are registered independently', () => { var select = jest.fn(); var onSelect = event => select(event.currentTarget); + // Pass `onMouseDown` so React registers a top-level listener. var node = ReactDOM.render( - , + , container, ); node.focus(); - var nativeEvent = new MouseEvent('focus', { - bubbles: true, - cancelable: true, - }); - node.dispatchEvent(nativeEvent); + // Trigger `mousedown` and `mouseup`. Note that + // React is not currently listening to `mouseup`. + node.dispatchEvent( + new MouseEvent('mousedown', { + bubbles: true, + cancelable: true, + }), + ); + node.dispatchEvent( + new MouseEvent('mouseup', { + bubbles: true, + cancelable: true, + }), + ); - nativeEvent = new MouseEvent('mousedown', { - bubbles: true, - cancelable: true, - }); - node.dispatchEvent(nativeEvent); + // Now subscribe to `onSelect`. + ReactDOM.render(, container); + node.focus(); - nativeEvent = new MouseEvent('keyup', {bubbles: true, cancelable: true}); - node.dispatchEvent(nativeEvent); - expect(select.mock.calls.length).toBe(0); + // This triggers a `select` event in our polyfill. + node.dispatchEvent( + new KeyboardEvent('keydown', {bubbles: true, cancelable: true}), + ); + + // Verify that it doesn't get "stuck" waiting for + // a `mouseup` event that it has "missed" because + // a top-level listener didn't exist yet. + expect(select.mock.calls.length).toBe(1); }); - it('should verify that `onSelect` fires when the listener is present', () => { + it('should fire `onSelect` when a listener is present', () => { var select = jest.fn(); var onSelect = event => { expect(typeof event).toBe('object'); From 9c7bdb601a0301773a398c364e3fe9b2dd842316 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 29 Nov 2017 22:05:54 +0000 Subject: [PATCH 6/7] Remove -internal suffix --- ...lectEventPlugin-test.internal.js => SelectEventPlugin-test.js} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename packages/react-dom/src/events/__tests__/{SelectEventPlugin-test.internal.js => SelectEventPlugin-test.js} (100%) diff --git a/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js b/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.js similarity index 100% rename from packages/react-dom/src/events/__tests__/SelectEventPlugin-test.internal.js rename to packages/react-dom/src/events/__tests__/SelectEventPlugin-test.js From 275b2f4bf23a1546c35bda66c1a234d779aa14c6 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 29 Nov 2017 22:07:46 +0000 Subject: [PATCH 7/7] Oops --- .../src/events/__tests__/SelectEventPlugin-test.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.js b/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.js index d7827c7cff47..f01a406d409f 100644 --- a/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.js +++ b/packages/react-dom/src/events/__tests__/SelectEventPlugin-test.js @@ -31,7 +31,12 @@ describe('SelectEventPlugin', () => { // See https://github.com/facebook/react/pull/3639 for details. it('does not get confused when dependent events are registered independently', () => { var select = jest.fn(); - var onSelect = event => select(event.currentTarget); + var onSelect = event => { + expect(typeof event).toBe('object'); + expect(event.type).toBe('select'); + expect(event.target).toBe(node); + select(event.currentTarget); + }; // Pass `onMouseDown` so React registers a top-level listener. var node = ReactDOM.render( @@ -56,7 +61,7 @@ describe('SelectEventPlugin', () => { ); // Now subscribe to `onSelect`. - ReactDOM.render(, container); + ReactDOM.render(, container); node.focus(); // This triggers a `select` event in our polyfill.