From d27dbd3c8875559121f177d76ab66b392302aca2 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 1 Apr 2019 23:34:56 +0100 Subject: [PATCH 1/2] Remove warning for event targets being direct children of event component --- .../src/client/ReactDOMHostConfig.js | 5 -- .../src/createReactNoop.js | 4 -- .../ReactFiberEvents-test-internal.js | 59 ------------------- .../src/ReactTestHostConfig.js | 4 -- 4 files changed, 72 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index 14e80522b196c..3839825e74c8e 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -200,11 +200,6 @@ export function getChildHostContextForEvent( isEventTarget: false, }; } else if (type === REACT_EVENT_TARGET_TYPE) { - warning( - parentHostContextDev.eventData !== null && - parentHostContextDev.eventData.isEventComponent, - 'validateDOMNesting: React event targets must be direct children of event components.', - ); eventData = { isEventComponent: false, isEventTarget: true, diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 585406898e2e1..d016cbccb5b0f 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -274,10 +274,6 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { ); return EVENT_COMPONENT_CONTEXT; } else if (type === REACT_EVENT_TARGET_TYPE) { - warning( - parentHostContext === EVENT_COMPONENT_CONTEXT, - 'validateDOMNesting: React event targets must be direct children of event components.', - ); return EVENT_TARGET_CONTEXT; } } diff --git a/packages/react-reconciler/src/__tests__/ReactFiberEvents-test-internal.js b/packages/react-reconciler/src/__tests__/ReactFiberEvents-test-internal.js index 9a697379d3bc7..3bcadcbd1050f 100644 --- a/packages/react-reconciler/src/__tests__/ReactFiberEvents-test-internal.js +++ b/packages/react-reconciler/src/__tests__/ReactFiberEvents-test-internal.js @@ -184,25 +184,6 @@ describe('ReactFiberEvents', () => { ); }); - it('should warn if an event target is not a direct child of an event component', () => { - const Test = () => ( - -
- - Child 1 - -
- - ); - - expect(() => { - ReactNoop.render(); - expect(Scheduler).toFlushWithoutYielding(); - }).toWarnDev( - 'Warning: validateDOMNesting: React event targets must be direct children of event components.', - ); - }); - it('should warn if an event target has an event component as a child', () => { const Test = () => ( @@ -552,26 +533,6 @@ describe('ReactFiberEvents', () => { expect(root).toMatchRenderedOutput(Child 1); }); - it('should warn if an event target is not a direct child of an event component', () => { - const Test = () => ( - -
- - Child 1 - -
- - ); - - const root = ReactTestRenderer.create(null); - expect(() => { - root.update(); - expect(Scheduler).toFlushWithoutYielding(); - }).toWarnDev( - 'Warning: validateDOMNesting: React event targets must be direct children of event components.', - ); - }); - it('should warn if an event target has an event component as a child', () => { const Test = () => ( @@ -923,26 +884,6 @@ describe('ReactFiberEvents', () => { expect(container.innerHTML).toBe('Child 1'); }); - it('should warn if an event target is not a direct child of an event component', () => { - const Test = () => ( - -
- - Child 1 - -
- - ); - - expect(() => { - const container = document.createElement('div'); - ReactDOM.render(, container); - expect(Scheduler).toFlushWithoutYielding(); - }).toWarnDev( - 'Warning: validateDOMNesting: React event targets must be direct children of event components.', - ); - }); - it('should warn if an event target has an event component as a child', () => { const Test = () => ( diff --git a/packages/react-test-renderer/src/ReactTestHostConfig.js b/packages/react-test-renderer/src/ReactTestHostConfig.js index a5ac30273baae..d35c3a197fd7e 100644 --- a/packages/react-test-renderer/src/ReactTestHostConfig.js +++ b/packages/react-test-renderer/src/ReactTestHostConfig.js @@ -139,10 +139,6 @@ export function getChildHostContextForEvent( ); return EVENT_COMPONENT_CONTEXT; } else if (type === REACT_EVENT_TARGET_TYPE) { - warning( - parentHostContext === EVENT_COMPONENT_CONTEXT, - 'validateDOMNesting: React event targets must be direct children of event components.', - ); return EVENT_TARGET_CONTEXT; } } From 57a1743829191202306602cbd42282ba26dfbd84 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 2 Apr 2019 14:21:20 +0100 Subject: [PATCH 2/2] Addressed feedback and added more test coverage + warnings Apply fiber tree traversal Apply fiber tree traversal (remove null check) Apply fiber tree traversal (remove null check) --- packages/react-art/src/ReactARTHostConfig.js | 7 +- .../src/client/ReactDOMHostConfig.js | 96 +++--- .../__tests__/TouchHitTarget-test.internal.js | 322 ++++++++++++++++++ .../src/ReactFabricHostConfig.js | 12 +- .../src/ReactNativeHostConfig.js | 12 +- .../src/createReactNoop.js | 44 ++- .../src/ReactFiberBeginWork.js | 13 +- .../src/ReactFiberCompleteWork.js | 14 +- .../src/ReactFiberHostContext.js | 30 +- .../ReactFiberEvents-test-internal.js | 150 ++++---- .../__tests__/ReactFiberHostContext-test.js | 5 +- .../src/forks/ReactFiberHostConfig.custom.js | 6 +- .../src/ReactTestHostConfig.js | 50 ++- .../shared/getElementFromTouchHitTarget.js | 85 ----- 14 files changed, 593 insertions(+), 253 deletions(-) create mode 100644 packages/react-events/src/__tests__/TouchHitTarget-test.internal.js delete mode 100644 packages/shared/getElementFromTouchHitTarget.js diff --git a/packages/react-art/src/ReactARTHostConfig.js b/packages/react-art/src/ReactARTHostConfig.js index caf2d5da18aae..347693416591f 100644 --- a/packages/react-art/src/ReactARTHostConfig.js +++ b/packages/react-art/src/ReactARTHostConfig.js @@ -340,7 +340,11 @@ export function getChildHostContext() { return NO_CONTEXT; } -export function getChildHostContextForEvent() { +export function getChildHostContextForEventComponent() { + return NO_CONTEXT; +} + +export function getChildHostContextForEventTarget() { return NO_CONTEXT; } @@ -446,6 +450,7 @@ export function handleEventComponent( export function handleEventTarget( type: Symbol | number, props: Props, + parentInstance: Container, internalInstanceHandle: Object, ) { // TODO: add handleEventTarget implementation diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index 3839825e74c8e..ef96ee8f944bc 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -45,12 +45,7 @@ import dangerousStyleValue from '../shared/dangerousStyleValue'; import type {DOMContainer} from './ReactDOM'; import type {ReactEventResponder} from 'shared/ReactTypes'; -import { - REACT_EVENT_COMPONENT_TYPE, - REACT_EVENT_TARGET_TYPE, - REACT_EVENT_TARGET_TOUCH_HIT, -} from 'shared/ReactSymbols'; -import getElementFromTouchHitTarget from 'shared/getElementFromTouchHitTarget'; +import {REACT_EVENT_TARGET_TOUCH_HIT} from 'shared/ReactSymbols'; export type Type = string; export type Props = { @@ -75,6 +70,7 @@ type HostContextDev = { eventData: null | {| isEventComponent?: boolean, isEventTarget?: boolean, + eventTargetType?: null | Symbol | number, |}, }; type HostContextProd = string; @@ -180,31 +176,46 @@ export function getChildHostContext( return getChildNamespace(parentNamespace, type); } -export function getChildHostContextForEvent( +export function getChildHostContextForEventComponent( parentHostContext: HostContext, - type: Symbol | number, ): HostContext { if (__DEV__) { const parentHostContextDev = ((parentHostContext: any): HostContextDev); const {namespace, ancestorInfo} = parentHostContextDev; - let eventData = null; + warning( + parentHostContextDev.eventData === null || + !parentHostContextDev.eventData.isEventTarget, + 'validateDOMNesting: React event targets must not have event components as children.', + ); + const eventData = { + isEventComponent: true, + isEventTarget: false, + eventTargetType: null, + }; + return {namespace, ancestorInfo, eventData}; + } + return parentHostContext; +} - if (type === REACT_EVENT_COMPONENT_TYPE) { - warning( - parentHostContextDev.eventData === null || - !parentHostContextDev.eventData.isEventTarget, - 'validateDOMNesting: React event targets must not have event components as children.', - ); - eventData = { - isEventComponent: true, - isEventTarget: false, - }; - } else if (type === REACT_EVENT_TARGET_TYPE) { - eventData = { - isEventComponent: false, - isEventTarget: true, - }; - } +export function getChildHostContextForEventTarget( + parentHostContext: HostContext, + type: Symbol | number, +): HostContext { + if (__DEV__) { + const parentHostContextDev = ((parentHostContext: any): HostContextDev); + const {namespace, ancestorInfo} = parentHostContextDev; + warning( + parentHostContextDev.eventData === null || + !parentHostContextDev.eventData.isEventComponent || + type !== REACT_EVENT_TARGET_TOUCH_HIT, + 'validateDOMNesting: cannot not be a direct child of an event component. ' + + 'Ensure is a direct child of a DOM element.', + ); + const eventData = { + isEventComponent: false, + isEventTarget: true, + eventTargetType: type, + }; return {namespace, ancestorInfo, eventData}; } return parentHostContext; @@ -238,6 +249,16 @@ export function createInstance( if (__DEV__) { // TODO: take namespace into account when validating. const hostContextDev = ((hostContext: any): HostContextDev); + if (enableEventAPI) { + const eventData = hostContextDev.eventData; + if (eventData !== null) { + warning( + !eventData.isEventTarget || + eventData.eventTargetType !== REACT_EVENT_TARGET_TOUCH_HIT, + 'Warning: validateDOMNesting: must not have any children.', + ); + } + } validateDOMNesting(type, null, hostContextDev.ancestorInfo); if ( typeof props.children === 'string' || @@ -344,6 +365,12 @@ export function createTextInstance( if (enableEventAPI) { const eventData = hostContextDev.eventData; if (eventData !== null) { + warning( + eventData === null || + !eventData.isEventTarget || + eventData.eventTargetType !== REACT_EVENT_TARGET_TOUCH_HIT, + 'Warning: validateDOMNesting: must not have any children.', + ); warning( !eventData.isEventComponent, 'validateDOMNesting: React event components cannot have text DOM nodes as children. ' + @@ -351,7 +378,8 @@ export function createTextInstance( text, ); warning( - !eventData.isEventTarget, + !eventData.isEventTarget || + eventData.eventTargetType === REACT_EVENT_TARGET_TOUCH_HIT, 'validateDOMNesting: React event targets cannot have text DOM nodes as children. ' + 'Wrap the child text "%s" in an element.', text, @@ -874,25 +902,13 @@ export function handleEventComponent( export function handleEventTarget( type: Symbol | number, props: Props, + parentInstance: Container, internalInstanceHandle: Object, ): void { if (enableEventAPI) { // Touch target hit slop handling if (type === REACT_EVENT_TARGET_TOUCH_HIT) { - // Validates that there is a single element - const element = getElementFromTouchHitTarget(internalInstanceHandle); - if (element !== null) { - // We update the event target state node to be that of the element. - // We can then diff this entry to determine if we need to add the - // hit slop element, or change the dimensions of the hit slop. - const lastElement = internalInstanceHandle.stateNode; - if (lastElement !== element) { - internalInstanceHandle.stateNode = element; - // TODO: Create the hit slop element and attach it to the element - } else { - // TODO: Diff the left, top, right, bottom props - } - } + // TODO } } } diff --git a/packages/react-events/src/__tests__/TouchHitTarget-test.internal.js b/packages/react-events/src/__tests__/TouchHitTarget-test.internal.js new file mode 100644 index 0000000000000..e47f7f3cb4714 --- /dev/null +++ b/packages/react-events/src/__tests__/TouchHitTarget-test.internal.js @@ -0,0 +1,322 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @emails react-core + */ + +'use strict'; + +let React; +let ReactNoop; +let Scheduler; +let ReactFeatureFlags; +let EventComponent; +let ReactTestRenderer; +let ReactDOM; +let ReactSymbols; +let ReactEvents; +let TouchHitTarget; + +const noOpResponder = { + targetEventTypes: [], + handleEvent() {}, +}; + +function createReactEventComponent() { + return { + $$typeof: ReactSymbols.REACT_EVENT_COMPONENT_TYPE, + props: null, + responder: noOpResponder, + }; +} + +function init() { + jest.resetModules(); + ReactFeatureFlags = require('shared/ReactFeatureFlags'); + ReactFeatureFlags.enableEventAPI = true; + React = require('react'); + Scheduler = require('scheduler'); + ReactSymbols = require('shared/ReactSymbols'); + ReactEvents = require('react-events'); +} + +function initNoopRenderer() { + init(); + ReactNoop = require('react-noop-renderer'); +} + +function initTestRenderer() { + init(); + ReactTestRenderer = require('react-test-renderer'); +} + +function initReactDOM() { + init(); + ReactDOM = require('react-dom'); +} + +describe('TouchHitTarget', () => { + describe('NoopRenderer', () => { + beforeEach(() => { + initNoopRenderer(); + EventComponent = createReactEventComponent(); + TouchHitTarget = ReactEvents.TouchHitTarget; + }); + + it('should not warn when a TouchHitTarget is used correctly', () => { + const Test = () => ( + +
+ +
+ + ); + + ReactNoop.render(); + expect(Scheduler).toFlushWithoutYielding(); + expect(ReactNoop).toMatchRenderedOutput(
); + }); + + it('should warn when a TouchHitTarget has children', () => { + const Test = () => ( + +
+ + Child 1 + +
+ + ); + + expect(() => { + ReactNoop.render(); + expect(Scheduler).toFlushWithoutYielding(); + }).toWarnDev( + 'Warning: validateDOMNesting: must not have any children.', + ); + + const Test2 = () => ( + +
+ Child 1 +
+ + ); + + expect(() => { + ReactNoop.render(); + expect(Scheduler).toFlushWithoutYielding(); + }).toWarnDev( + 'Warning: validateDOMNesting: must not have any children.', + ); + + // Should render without warnings + const Test3 = () => ( + +
+ +
+ + ); + + ReactNoop.render(); + expect(Scheduler).toFlushWithoutYielding(); + expect(ReactNoop).toMatchRenderedOutput(
); + }); + + it('should warn when a TouchHitTarget is a direct child of an event component', () => { + const Test = () => ( + + + + ); + + expect(() => { + ReactNoop.render(); + expect(Scheduler).toFlushWithoutYielding(); + }).toWarnDev( + 'Warning: validateDOMNesting: cannot not be a direct child of an event component. ' + + 'Ensure is a direct child of a DOM element.', + ); + }); + }); + + describe('TestRenderer', () => { + beforeEach(() => { + initTestRenderer(); + EventComponent = createReactEventComponent(); + TouchHitTarget = ReactEvents.TouchHitTarget; + }); + + it('should not warn when a TouchHitTarget is used correctly', () => { + const Test = () => ( + +
+ +
+ + ); + + const root = ReactTestRenderer.create(null); + root.update(); + expect(Scheduler).toFlushWithoutYielding(); + expect(root).toMatchRenderedOutput(
); + }); + + it('should warn when a TouchHitTarget has children', () => { + const Test = () => ( + +
+ + Child 1 + +
+ + ); + + const root = ReactTestRenderer.create(null); + expect(() => { + root.update(); + expect(Scheduler).toFlushWithoutYielding(); + }).toWarnDev( + 'Warning: validateDOMNesting: must not have any children.', + ); + + const Test2 = () => ( + +
+ Child 1 +
+ + ); + + expect(() => { + root.update(); + expect(Scheduler).toFlushWithoutYielding(); + }).toWarnDev( + 'Warning: validateDOMNesting: must not have any children.', + ); + + // Should render without warnings + const Test3 = () => ( + +
+ +
+ + ); + + root.update(); + expect(Scheduler).toFlushWithoutYielding(); + expect(root).toMatchRenderedOutput(
); + }); + + it('should warn when a TouchHitTarget is a direct child of an event component', () => { + const Test = () => ( + + + + ); + + const root = ReactTestRenderer.create(null); + expect(() => { + root.update(); + expect(Scheduler).toFlushWithoutYielding(); + }).toWarnDev( + 'Warning: validateDOMNesting: cannot not be a direct child of an event component. ' + + 'Ensure is a direct child of a DOM element.', + ); + }); + }); + + describe('ReactDOM', () => { + beforeEach(() => { + initReactDOM(); + EventComponent = createReactEventComponent(); + TouchHitTarget = ReactEvents.TouchHitTarget; + }); + + it('should not warn when a TouchHitTarget is used correctly', () => { + const Test = () => ( + +
+ +
+ + ); + + const container = document.createElement('div'); + ReactDOM.render(, container); + expect(Scheduler).toFlushWithoutYielding(); + expect(container.innerHTML).toBe('
'); + }); + + it('should warn when a TouchHitTarget has children', () => { + const Test = () => ( + +
+ + Child 1 + +
+ + ); + + const container = document.createElement('div'); + expect(() => { + ReactDOM.render(, container); + expect(Scheduler).toFlushWithoutYielding(); + }).toWarnDev( + 'Warning: validateDOMNesting: must not have any children.', + ); + + const Test2 = () => ( + +
+ Child 1 +
+ + ); + + expect(() => { + ReactDOM.render(, container); + expect(Scheduler).toFlushWithoutYielding(); + }).toWarnDev( + 'Warning: validateDOMNesting: must not have any children.', + ); + + // Should render without warnings + const Test3 = () => ( + +
+ +
+ + ); + + ReactDOM.render(, container); + expect(Scheduler).toFlushWithoutYielding(); + expect(container.innerHTML).toBe('
'); + }); + + it('should warn when a TouchHitTarget is a direct child of an event component', () => { + const Test = () => ( + + + + ); + + const container = document.createElement('div'); + expect(() => { + ReactDOM.render(, container); + expect(Scheduler).toFlushWithoutYielding(); + }).toWarnDev( + 'Warning: validateDOMNesting: cannot not be a direct child of an event component. ' + + 'Ensure is a direct child of a DOM element.', + ); + }); + }); +}); diff --git a/packages/react-native-renderer/src/ReactFabricHostConfig.js b/packages/react-native-renderer/src/ReactFabricHostConfig.js index bc673062307f4..c2ccde544cb98 100644 --- a/packages/react-native-renderer/src/ReactFabricHostConfig.js +++ b/packages/react-native-renderer/src/ReactFabricHostConfig.js @@ -283,11 +283,18 @@ export function getChildHostContext( } } -export function getChildHostContextForEvent( +export function getChildHostContextForEventComponent( + parentHostContext: HostContext, +) { + // TODO: add getChildHostContextForEventComponent implementation + return parentHostContext; +} + +export function getChildHostContextForEventTarget( parentHostContext: HostContext, type: Symbol | number, ) { - // TODO: add getChildHostContextForEvent implementation + // TODO: add getChildHostContextForEventTarget implementation return parentHostContext; } @@ -438,6 +445,7 @@ export function handleEventComponent( export function handleEventTarget( type: Symbol | number, props: Props, + parentInstance: Container, internalInstanceHandle: Object, ) { // TODO: add handleEventTarget implementation diff --git a/packages/react-native-renderer/src/ReactNativeHostConfig.js b/packages/react-native-renderer/src/ReactNativeHostConfig.js index 27044ef54df2f..f4b24a1c39f4b 100644 --- a/packages/react-native-renderer/src/ReactNativeHostConfig.js +++ b/packages/react-native-renderer/src/ReactNativeHostConfig.js @@ -206,11 +206,18 @@ export function getChildHostContext( } } -export function getChildHostContextForEvent( +export function getChildHostContextForEventComponent( + parentHostContext: HostContext, +) { + // TODO: add getChildHostContextForEventComponent implementation + return parentHostContext; +} + +export function getChildHostContextForEventTarget( parentHostContext: HostContext, type: Symbol | number, ) { - // TODO: add getChildHostContextForEvent implementation + // TODO: add getChildHostContextForEventTarget implementation return parentHostContext; } @@ -497,6 +504,7 @@ export function handleEventComponent( export function handleEventTarget( type: Symbol | number, props: Props, + parentInstance: Container, internalInstanceHandle: Object, ) { // TODO: add handleEventTarget implementation diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index d016cbccb5b0f..8976372bf6c48 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -24,13 +24,10 @@ import expect from 'expect'; import { REACT_FRAGMENT_TYPE, REACT_ELEMENT_TYPE, - REACT_EVENT_COMPONENT_TYPE, - REACT_EVENT_TARGET_TYPE, REACT_EVENT_TARGET_TOUCH_HIT, } from 'shared/ReactSymbols'; import warningWithoutStack from 'shared/warningWithoutStack'; import warning from 'shared/warning'; -import getElementFromTouchHitTarget from 'shared/getElementFromTouchHitTarget'; import {enableEventAPI} from 'shared/ReactFeatureFlags'; @@ -66,6 +63,7 @@ const NO_CONTEXT = {}; const UPPERCASE_CONTEXT = {}; const EVENT_COMPONENT_CONTEXT = {}; const EVENT_TARGET_CONTEXT = {}; +const EVENT_TOUCH_HIT_TARGET_CONTEXT = {}; const UPDATE_SIGNAL = {}; if (__DEV__) { Object.freeze(NO_CONTEXT); @@ -262,20 +260,32 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { return NO_CONTEXT; }, - getChildHostContextForEvent( + getChildHostContextForEventComponent(parentHostContext: HostContext) { + if (__DEV__ && enableEventAPI) { + warning( + parentHostContext !== EVENT_TARGET_CONTEXT && + parentHostContext !== EVENT_TOUCH_HIT_TARGET_CONTEXT, + 'validateDOMNesting: React event targets must not have event components as children.', + ); + return EVENT_COMPONENT_CONTEXT; + } + return parentHostContext; + }, + + getChildHostContextForEventTarget( parentHostContext: HostContext, type: Symbol | number, ) { if (__DEV__ && enableEventAPI) { - if (type === REACT_EVENT_COMPONENT_TYPE) { + if (type === REACT_EVENT_TARGET_TOUCH_HIT) { warning( - parentHostContext !== EVENT_TARGET_CONTEXT, - 'validateDOMNesting: React event targets must not have event components as children.', + parentHostContext !== EVENT_COMPONENT_CONTEXT, + 'validateDOMNesting: cannot not be a direct child of an event component. ' + + 'Ensure is a direct child of a DOM element.', ); - return EVENT_COMPONENT_CONTEXT; - } else if (type === REACT_EVENT_TARGET_TYPE) { - return EVENT_TARGET_CONTEXT; + return EVENT_TOUCH_HIT_TARGET_CONTEXT; } + return EVENT_TARGET_CONTEXT; } return parentHostContext; }, @@ -290,6 +300,12 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { rootContainerInstance: Container, hostContext: HostContext, ): Instance { + if (__DEV__ && enableEventAPI) { + warning( + hostContext !== EVENT_TOUCH_HIT_TARGET_CONTEXT, + 'validateDOMNesting: must not have any children.', + ); + } if (type === 'errorInCompletePhase') { throw new Error('Error in host config.'); } @@ -364,6 +380,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { internalInstanceHandle: Object, ): TextInstance { if (__DEV__ && enableEventAPI) { + warning( + hostContext !== EVENT_TOUCH_HIT_TARGET_CONTEXT, + 'validateDOMNesting: must not have any children.', + ); warning( hostContext !== EVENT_COMPONENT_CONTEXT, 'validateDOMNesting: React event components cannot have text DOM nodes as children. ' + @@ -415,11 +435,11 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { handleEventTarget( type: Symbol | number, props: Props, + parentInstance: Container, internalInstanceHandle: Object, ) { if (type === REACT_EVENT_TARGET_TOUCH_HIT) { - // Validates that there is a single element - getElementFromTouchHitTarget(internalInstanceHandle); + // TODO } }, }; diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index a2eea421d4ab2..e5ac3a605aeef 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -99,7 +99,8 @@ import type {SuspenseInstance} from './ReactFiberHostConfig'; import { pushHostContext, pushHostContainer, - pushHostContextForEvent, + pushHostContextForEventComponent, + pushHostContextForEventTarget, } from './ReactFiberHostContext'; import { pushProvider, @@ -1960,7 +1961,7 @@ function updateEventComponent(current, workInProgress, renderExpirationTime) { nextChildren, renderExpirationTime, ); - pushHostContextForEvent(workInProgress); + pushHostContextForEventComponent(workInProgress); return workInProgress.child; } @@ -1974,7 +1975,7 @@ function updateEventTarget(current, workInProgress, renderExpirationTime) { nextChildren, renderExpirationTime, ); - pushHostContextForEvent(workInProgress); + pushHostContextForEventTarget(workInProgress); return workInProgress.child; } @@ -2116,9 +2117,13 @@ function beginWork( break; } case EventComponent: + if (enableEventAPI) { + pushHostContextForEventComponent(workInProgress); + } + break; case EventTarget: { if (enableEventAPI) { - pushHostContextForEvent(workInProgress); + pushHostContextForEventTarget(workInProgress); } break; } diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index 1a0fd30232262..abd125d3acb78 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -784,7 +784,19 @@ function completeWork( if (enableEventAPI) { popHostContext(workInProgress); const type = workInProgress.type.type; - handleEventTarget(type, newProps, workInProgress); + let node = workInProgress.return; + let parentHostInstance = null; + // Traverse up the fiber tree till we find a host component fiber + while (node !== null) { + if (node.tag === HostComponent) { + parentHostInstance = node.stateNode; + break; + } + node = node.return; + } + if (parentHostInstance !== null) { + handleEventTarget(type, newProps, parentHostInstance, workInProgress); + } } break; } diff --git a/packages/react-reconciler/src/ReactFiberHostContext.js b/packages/react-reconciler/src/ReactFiberHostContext.js index 54598c6eed6f3..18171896f004e 100644 --- a/packages/react-reconciler/src/ReactFiberHostContext.js +++ b/packages/react-reconciler/src/ReactFiberHostContext.js @@ -16,7 +16,8 @@ import invariant from 'shared/invariant'; import { getChildHostContext, getRootHostContext, - getChildHostContextForEvent, + getChildHostContextForEventComponent, + getChildHostContextForEventTarget, } from './ReactFiberHostConfig'; import {createCursor, push, pop} from './ReactFiberStack'; @@ -96,10 +97,28 @@ function pushHostContext(fiber: Fiber): void { push(contextStackCursor, nextContext, fiber); } -function pushHostContextForEvent(fiber: Fiber): void { +function pushHostContextForEventComponent(fiber: Fiber): void { const context: HostContext = requiredContext(contextStackCursor.current); - const eventTypeof = fiber.type.$$typeof; - const nextContext = getChildHostContextForEvent(context, eventTypeof); + const nextContext = getChildHostContextForEventComponent(context); + + // Don't push this Fiber's context unless it's unique. + if (context === nextContext) { + return; + } + + // Track the context and the Fiber that provided it. + // This enables us to pop only Fibers that provide unique contexts. + push(contextFiberStackCursor, fiber, fiber); + push(contextStackCursor, nextContext, fiber); +} + +function pushHostContextForEventTarget(fiber: Fiber): void { + const context: HostContext = requiredContext(contextStackCursor.current); + const eventTargetType = fiber.type.type; + const nextContext = getChildHostContextForEventTarget( + context, + eventTargetType, + ); // Don't push this Fiber's context unless it's unique. if (context === nextContext) { @@ -130,5 +149,6 @@ export { popHostContext, pushHostContainer, pushHostContext, - pushHostContextForEvent, + pushHostContextForEventComponent, + pushHostContextForEventTarget, }; diff --git a/packages/react-reconciler/src/__tests__/ReactFiberEvents-test-internal.js b/packages/react-reconciler/src/__tests__/ReactFiberEvents-test-internal.js index 3bcadcbd1050f..25162397b0490 100644 --- a/packages/react-reconciler/src/__tests__/ReactFiberEvents-test-internal.js +++ b/packages/react-reconciler/src/__tests__/ReactFiberEvents-test-internal.js @@ -19,7 +19,7 @@ let ReactDOM; let ReactDOMServer; let ReactTestUtils; let EventTarget; -let ReactEvents; +let ReactSymbols; const noOpResponder = { targetEventTypes: [], @@ -28,19 +28,26 @@ const noOpResponder = { function createReactEventComponent() { return { - $$typeof: Symbol.for('react.event_component'), + $$typeof: ReactSymbols.REACT_EVENT_COMPONENT_TYPE, props: null, responder: noOpResponder, }; } +function createReactEventTarget() { + return { + $$typeof: ReactSymbols.REACT_EVENT_TARGET_TYPE, + type: Symbol.for('react.event_target.test'), + }; +} + function init() { jest.resetModules(); ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.enableEventAPI = true; React = require('react'); Scheduler = require('scheduler'); - ReactEvents = require('react-events'); + ReactSymbols = require('shared/ReactSymbols'); } function initNoopRenderer() { @@ -71,7 +78,7 @@ describe('ReactFiberEvents', () => { beforeEach(() => { initNoopRenderer(); EventComponent = createReactEventComponent(); - EventTarget = ReactEvents.TouchHitTarget; + EventTarget = createReactEventTarget(); }); it('should render a simple event component with a single child', () => { @@ -139,11 +146,10 @@ describe('ReactFiberEvents', () => { expect(() => { ReactNoop.render(); expect(Scheduler).toFlushWithoutYielding(); - }).toWarnDev([ + }).toWarnDev( 'Warning: validateDOMNesting: React event targets cannot have text DOM nodes as children. ' + 'Wrap the child text "Hello world" in an element.', - 'Warning: must have a single DOM element as a child. Found no children.', - ]); + ); }); it('should warn when an event target has a direct text child #2', () => { @@ -159,28 +165,29 @@ describe('ReactFiberEvents', () => { expect(() => { ReactNoop.render(); expect(Scheduler).toFlushWithoutYielding(); - }).toWarnDev([ + }).toWarnDev( 'Warning: validateDOMNesting: React event targets cannot have text DOM nodes as children. ' + 'Wrap the child text "Hello world" in an element.', - 'Warning: must have a single DOM element as a child. Found no children.', - ]); + ); }); - it('should warn when an event target has more than one child', () => { + it('should not warn if an event target is not a direct child of an event component', () => { const Test = () => ( - - Child 1 - Child 2 - +
+ + Child 1 + +
); - expect(() => { - ReactNoop.render(); - expect(Scheduler).toFlushWithoutYielding(); - }).toWarnDev( - 'Warning: must only have a single DOM element as a child. Found many children.', + ReactNoop.render(); + expect(Scheduler).toFlushWithoutYielding(); + expect(ReactNoop).toMatchRenderedOutput( +
+ Child 1 +
, ); }); @@ -300,7 +307,7 @@ describe('ReactFiberEvents', () => { _updateCounter = updateCounter; if (counter === 1) { - return null; + return 'Text!'; } return ( @@ -332,7 +339,8 @@ describe('ReactFiberEvents', () => { }); expect(Scheduler).toFlushWithoutYielding(); }).toWarnDev( - 'Warning: must have a single DOM element as a child. Found no children.', + 'Warning: validateDOMNesting: React event targets cannot have text DOM nodes as children. ' + + 'Wrap the child text "Text!" in an element.', ); }); @@ -390,7 +398,7 @@ describe('ReactFiberEvents', () => { beforeEach(() => { initTestRenderer(); EventComponent = createReactEventComponent(); - EventTarget = ReactEvents.TouchHitTarget; + EventTarget = createReactEventTarget(); }); it('should render a simple event component with a single child', () => { @@ -475,11 +483,10 @@ describe('ReactFiberEvents', () => { expect(() => { root.update(); expect(Scheduler).toFlushWithoutYielding(); - }).toWarnDev([ + }).toWarnDev( 'Warning: validateDOMNesting: React event targets cannot have text DOM nodes as children. ' + 'Wrap the child text "Hello world" in an element.', - 'Warning: must have a single DOM element as a child. Found no children.', - ]); + ); }); it('should warn when an event target has a direct text child #2', () => { @@ -496,41 +503,31 @@ describe('ReactFiberEvents', () => { expect(() => { root.update(); expect(Scheduler).toFlushWithoutYielding(); - }).toWarnDev([ + }).toWarnDev( 'Warning: validateDOMNesting: React event targets cannot have text DOM nodes as children. ' + 'Wrap the child text "Hello world" in an element.', - 'Warning: must have a single DOM element as a child. Found no children.', - ]); + ); }); - it('should warn when an event target has more than one child', () => { + it('should not warn if an event target is not a direct child of an event component', () => { const Test = () => ( - - Child 1 - Child 2 - +
+ + Child 1 + +
); const root = ReactTestRenderer.create(null); - expect(() => { - root.update(); - expect(Scheduler).toFlushWithoutYielding(); - }).toWarnDev( - 'Warning: must only have a single DOM element as a child. Found many children.', - ); - // This should not fire a warning, as this is now valid. - const Test2 = () => ( - - - Child 1 - - - ); - root.update(); + root.update(); expect(Scheduler).toFlushWithoutYielding(); - expect(root).toMatchRenderedOutput(Child 1); + expect(root).toMatchRenderedOutput( +
+ Child 1 +
, + ); }); it('should warn if an event target has an event component as a child', () => { @@ -651,7 +648,7 @@ describe('ReactFiberEvents', () => { _updateCounter = updateCounter; if (counter === 1) { - return null; + return 'Text!'; } return ( @@ -683,7 +680,8 @@ describe('ReactFiberEvents', () => { _updateCounter(counter => counter + 1); }); }).toWarnDev( - 'Warning: must have a single DOM element as a child. Found no children.', + 'Warning: validateDOMNesting: React event targets cannot have text DOM nodes as children. ' + + 'Wrap the child text "Text!" in an element.', ); }); @@ -742,7 +740,7 @@ describe('ReactFiberEvents', () => { beforeEach(() => { initReactDOM(); EventComponent = createReactEventComponent(); - EventTarget = ReactEvents.TouchHitTarget; + EventTarget = createReactEventTarget(); }); it('should render a simple event component with a single child', () => { @@ -826,11 +824,10 @@ describe('ReactFiberEvents', () => { const container = document.createElement('div'); ReactDOM.render(, container); expect(Scheduler).toFlushWithoutYielding(); - }).toWarnDev([ + }).toWarnDev( 'Warning: validateDOMNesting: React event targets cannot have text DOM nodes as children. ' + 'Wrap the child text "Hello world" in an element.', - 'Warning: must have a single DOM element as a child. Found no children.', - ]); + ); }); it('should warn when an event target has a direct text child #2', () => { @@ -847,41 +844,27 @@ describe('ReactFiberEvents', () => { const container = document.createElement('div'); ReactDOM.render(, container); expect(Scheduler).toFlushWithoutYielding(); - }).toWarnDev([ + }).toWarnDev( 'Warning: validateDOMNesting: React event targets cannot have text DOM nodes as children. ' + 'Wrap the child text "Hello world" in an element.', - 'Warning: must have a single DOM element as a child. Found no children.', - ]); + ); }); - it('should warn when an event target has more than one child', () => { + it('should not warn if an event target is not a direct child of an event component', () => { const Test = () => ( - - Child 1 - Child 2 - +
+ + Child 1 + +
); const container = document.createElement('div'); - expect(() => { - ReactDOM.render(, container); - expect(Scheduler).toFlushWithoutYielding(); - }).toWarnDev( - 'Warning: must only have a single DOM element as a child. Found many children.', - ); - // This should not fire a warning, as this is now valid. - const Test2 = () => ( - - - Child 1 - - - ); - ReactDOM.render(, container); + ReactDOM.render(, container); expect(Scheduler).toFlushWithoutYielding(); - expect(container.innerHTML).toBe('Child 1'); + expect(container.innerHTML).toBe('
Child 1
'); }); it('should warn if an event target has an event component as a child', () => { @@ -993,7 +976,7 @@ describe('ReactFiberEvents', () => { _updateCounter = updateCounter; if (counter === 1) { - return null; + return 'Text!'; } return ( @@ -1021,7 +1004,8 @@ describe('ReactFiberEvents', () => { }); expect(Scheduler).toFlushWithoutYielding(); }).toWarnDev( - 'Warning: must have a single DOM element as a child. Found no children.', + 'Warning: validateDOMNesting: React event targets cannot have text DOM nodes as children. ' + + 'Wrap the child text "Text!" in an element.', ); }); @@ -1075,7 +1059,7 @@ describe('ReactFiberEvents', () => { beforeEach(() => { initReactDOMServer(); EventComponent = createReactEventComponent(); - EventTarget = ReactEvents.TouchHitTarget; + EventTarget = createReactEventTarget(); }); it('should render a simple event component with a single child', () => { diff --git a/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.js b/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.js index 4efb059a28e2a..8ffa524bd94d2 100644 --- a/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.js +++ b/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.js @@ -31,7 +31,10 @@ describe('ReactFiberHostContext', () => { getChildHostContext: function() { return null; }, - getChildHostContextForEvent: function() { + getChildHostContextForEventComponent: function() { + return null; + }, + getChildHostContextForEventTarget: function() { return null; }, shouldSetTextContent: function() { diff --git a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js index 0926eb8cfbd1e..d1f38c65d22a3 100644 --- a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js +++ b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js @@ -41,8 +41,10 @@ export opaque type NoTimeout = mixed; // eslint-disable-line no-undef export const getPublicInstance = $$$hostConfig.getPublicInstance; export const getRootHostContext = $$$hostConfig.getRootHostContext; export const getChildHostContext = $$$hostConfig.getChildHostContext; -export const getChildHostContextForEvent = - $$$hostConfig.getChildHostContextForEvent; +export const getChildHostContextForEventComponent = + $$$hostConfig.getChildHostContextForEventComponent; +export const getChildHostContextForEventTarget = + $$$hostConfig.getChildHostContextForEventTarget; export const prepareForCommit = $$$hostConfig.prepareForCommit; export const resetAfterCommit = $$$hostConfig.resetAfterCommit; export const createInstance = $$$hostConfig.createInstance; diff --git a/packages/react-test-renderer/src/ReactTestHostConfig.js b/packages/react-test-renderer/src/ReactTestHostConfig.js index d35c3a197fd7e..8020282394c64 100644 --- a/packages/react-test-renderer/src/ReactTestHostConfig.js +++ b/packages/react-test-renderer/src/ReactTestHostConfig.js @@ -10,12 +10,7 @@ import warning from 'shared/warning'; import type {ReactEventResponder} from 'shared/ReactTypes'; -import { - REACT_EVENT_COMPONENT_TYPE, - REACT_EVENT_TARGET_TYPE, - REACT_EVENT_TARGET_TOUCH_HIT, -} from 'shared/ReactSymbols'; -import getElementFromTouchHitTarget from 'shared/getElementFromTouchHitTarget'; +import {REACT_EVENT_TARGET_TOUCH_HIT} from 'shared/ReactSymbols'; import {enableEventAPI} from 'shared/ReactFeatureFlags'; @@ -52,6 +47,7 @@ export * from 'shared/HostConfigWithNoHydration'; const EVENT_COMPONENT_CONTEXT = {}; const EVENT_TARGET_CONTEXT = {}; +const EVENT_TOUCH_HIT_TARGET_CONTEXT = {}; const NO_CONTEXT = {}; const UPDATE_SIGNAL = {}; if (__DEV__) { @@ -127,20 +123,34 @@ export function getChildHostContext( return NO_CONTEXT; } -export function getChildHostContextForEvent( +export function getChildHostContextForEventComponent( + parentHostContext: HostContext, +): HostContext { + if (__DEV__ && enableEventAPI) { + warning( + parentHostContext !== EVENT_TARGET_CONTEXT && + parentHostContext !== EVENT_TOUCH_HIT_TARGET_CONTEXT, + 'validateDOMNesting: React event targets must not have event components as children.', + ); + return EVENT_COMPONENT_CONTEXT; + } + return NO_CONTEXT; +} + +export function getChildHostContextForEventTarget( parentHostContext: HostContext, type: Symbol | number, ): HostContext { if (__DEV__ && enableEventAPI) { - if (type === REACT_EVENT_COMPONENT_TYPE) { + if (type === REACT_EVENT_TARGET_TOUCH_HIT) { warning( - parentHostContext !== EVENT_TARGET_CONTEXT, - 'validateDOMNesting: React event targets must not have event components as children.', + parentHostContext !== EVENT_COMPONENT_CONTEXT, + 'validateDOMNesting: cannot not be a direct child of an event component. ' + + 'Ensure is a direct child of a DOM element.', ); - return EVENT_COMPONENT_CONTEXT; - } else if (type === REACT_EVENT_TARGET_TYPE) { - return EVENT_TARGET_CONTEXT; + return EVENT_TOUCH_HIT_TARGET_CONTEXT; } + return EVENT_TARGET_CONTEXT; } return NO_CONTEXT; } @@ -160,6 +170,12 @@ export function createInstance( hostContext: Object, internalInstanceHandle: Object, ): Instance { + if (__DEV__ && enableEventAPI) { + warning( + hostContext !== EVENT_TOUCH_HIT_TARGET_CONTEXT, + 'validateDOMNesting: must not have any children.', + ); + } return { type, props, @@ -217,6 +233,10 @@ export function createTextInstance( internalInstanceHandle: Object, ): TextInstance { if (__DEV__ && enableEventAPI) { + warning( + hostContext !== EVENT_TOUCH_HIT_TARGET_CONTEXT, + 'validateDOMNesting: must not have any children.', + ); warning( hostContext !== EVENT_COMPONENT_CONTEXT, 'validateDOMNesting: React event components cannot have text DOM nodes as children. ' + @@ -316,10 +336,10 @@ export function handleEventComponent( export function handleEventTarget( type: Symbol | number, props: Props, + parentInstance: Container, internalInstanceHandle: Object, ) { if (type === REACT_EVENT_TARGET_TOUCH_HIT) { - // Validates that there is a single element - getElementFromTouchHitTarget(internalInstanceHandle); + // TODO } } diff --git a/packages/shared/getElementFromTouchHitTarget.js b/packages/shared/getElementFromTouchHitTarget.js deleted file mode 100644 index dea4fd7f31884..0000000000000 --- a/packages/shared/getElementFromTouchHitTarget.js +++ /dev/null @@ -1,85 +0,0 @@ -/** - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - * - * @flow - */ - -import type {Fiber} from 'react-reconciler/src/ReactFiber'; - -import {HostComponent} from 'shared/ReactWorkTags'; -import warning from 'shared/warning'; - -type HostContext = Object; - -type TextInstance = - | Text - | {| - text: string, - id: number, - hidden: boolean, - context: HostContext, - |}; - -type Instance = - | Element - | {| - type: string, - id: number, - children: Array, - text: string | null, - prop: any, - hidden: boolean, - context: HostContext, - |}; - -export default function getElementFromTouchHitTarget( - targetFiber: Fiber, -): null | Instance { - // Traverse through child fibers and find the first host components - let node = targetFiber.child; - let hostComponent = null; - - while (node !== null) { - if (node.tag === HostComponent) { - if (__DEV__) { - if (hostComponent === null) { - hostComponent = node.stateNode; - } else { - warning( - false, - ' must only have a single DOM element as a child. ' + - 'Found many children.', - ); - } - while (node !== null) { - if (node === targetFiber) { - return hostComponent; - } else if (node.sibling !== null) { - node = node.sibling; - break; - } - node = node.return; - } - } else { - return node.stateNode; - } - } else if (node.child !== null) { - node = node.child; - } else if (node.sibling !== null) { - node = node.sibling; - } else { - break; - } - } - if (__DEV__) { - warning( - hostComponent !== null, - ' must have a single DOM element as a child. ' + - 'Found no children.', - ); - } - return hostComponent; -}