diff --git a/packages/react-dom-bindings/src/client/ReactDOMComponent.js b/packages/react-dom-bindings/src/client/ReactDOMComponent.js index 1e8db9c833ba..59d725bde1e3 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMComponent.js +++ b/packages/react-dom-bindings/src/client/ReactDOMComponent.js @@ -379,12 +379,18 @@ export function trapClickOnNonInteractiveElement(node: HTMLElement) { // listener on the target node. // https://www.quirksmode.org/blog/archives/2010/09/click_event_del.html // Just set it using the onclick property so that we don't have to manage any - // bookkeeping for it. Not sure if we need to clear it when the listener is - // removed. + // bookkeeping for it. HostSingleton release clears the property only if it + // still points to this noop. // TODO: Only do this for the relevant Safaris maybe? node.onclick = noop; } +export function clearClickListener(node: HTMLElement) { + if (node.onclick === noop) { + node.onclick = null; + } +} + const xlinkNamespace = 'http://www.w3.org/1999/xlink'; const xmlNamespace = 'http://www.w3.org/XML/1998/namespace'; @@ -1489,6 +1495,26 @@ export function setInitialProperties( } } +export type SingletonType = 'html' | 'head' | 'body'; + +const emptyProps = {}; + +export function clearSingletonProperties( + domElement: Element, + tag: SingletonType, + props: Object, +): void { + // This is equivalent to updating to empty props for tags without + // tag-specific update logic. Host singletons are limited to html, head, and + // body, so they always use this generic path. + for (const propKey in props) { + const propValue = props[propKey]; + if (props.hasOwnProperty(propKey) && propValue != null) { + setProp(domElement, tag, propKey, null, emptyProps, propValue); + } + } +} + export function updateProperties( domElement: Element, tag: string, diff --git a/packages/react-dom-bindings/src/client/ReactDOMComponentTree.js b/packages/react-dom-bindings/src/client/ReactDOMComponentTree.js index 71b8630450f0..d501f06693e7 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMComponentTree.js +++ b/packages/react-dom-bindings/src/client/ReactDOMComponentTree.js @@ -69,10 +69,12 @@ const internalPropsMap: | Map = new PossiblyWeakMap(); export function detachDeletedInstance(node: Instance): void { + // Don't delete the event listener set. The native event listeners it tracks + // remain attached to the node, so this bookkeeping needs to last for the + // lifetime of the node to prevent duplicate listeners if it is reused. if (enableInternalInstanceMap) { internalInstanceMap.delete(node); internalPropsMap.delete(node); - delete (node as any)[internalEventHandlersKey]; delete (node as any)[internalEventHandlerListenersKey]; delete (node as any)[internalEventHandlesSetKey]; delete (node as any)[internalRootNodeResourcesKey]; @@ -85,7 +87,6 @@ export function detachDeletedInstance(node: Instance): void { // these fields are relevant. delete (node as any)[internalInstanceKey]; delete (node as any)[internalPropsKey]; - delete (node as any)[internalEventHandlersKey]; delete (node as any)[internalEventHandlerListenersKey]; delete (node as any)[internalEventHandlesSetKey]; } diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js index 373a9e698e1b..fb3e1abb4d5b 100644 --- a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js +++ b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js @@ -77,15 +77,18 @@ import {compareDocumentPositionForEmptyFragment} from 'shared/ReactDOMFragmentRe export {detachDeletedInstance}; import {hasRole} from './DOMAccessibilityRoles'; +import type {SingletonType} from './ReactDOMComponent'; import { setInitialProperties, updateProperties, + clearSingletonProperties, hydrateProperties, hydrateText, diffHydratedProperties, getPropsFromElement, diffHydratedText, trapClickOnNonInteractiveElement, + clearClickListener, } from './ReactDOMComponent'; import {hydrateInput} from './ReactDOMInput'; import {hydrateTextarea} from './ReactDOMTextarea'; @@ -1269,18 +1272,18 @@ function clearHydrationBoundary( // then it contributed to the html tag and we need to reset it. const ownerDocument = parentInstance.ownerDocument; const documentElement: Element = ownerDocument.documentElement as any; - releaseSingletonInstance(documentElement); + clearSingletonPreambleContribution(documentElement); } else if (data === PREAMBLE_CONTRIBUTION_HEAD) { const ownerDocument = parentInstance.ownerDocument; const head: Element = ownerDocument.head as any; - releaseSingletonInstance(head); + clearSingletonPreambleContribution(head); // We need to clear the head because this is the only singleton that can have children that // were part of this boundary but are not inside this boundary. clearHead(head); } else if (data === PREAMBLE_CONTRIBUTION_BODY) { const ownerDocument = parentInstance.ownerDocument; const body: Element = ownerDocument.body as any; - releaseSingletonInstance(body); + clearSingletonPreambleContribution(body); } } // $FlowFixMe[incompatible-type] we bail out when we get a null @@ -4844,7 +4847,40 @@ export function acquireSingletonInstance( updateFiberProps(instance, props); } -export function releaseSingletonInstance(instance: Instance): void { +export function releaseSingletonInstance( + instance: Instance, + type: SingletonType, + props: Props, +): void { + // Remove the attributes and property-backed state owned by this Fiber. + clearSingletonProperties(instance, type, props); + + // These properties aren't cleared by updateProperties when their next + // value is null. Normally that is handled by replacing/removing the host + // instance, but a singleton cannot be removed. + // TODO: HostSingleton updates do not currently schedule ContentReset when + // dangerouslySetInnerHTML becomes undefined, so an ordinary update can leave + // the previous HTML in place. This only handles the release path. + if (props.dangerouslySetInnerHTML != null) { + instance.textContent = ''; + } + clearClickListener(instance as any as HTMLElement); + + // Only remove state that was represented by this Fiber's props. Attributes + // added imperatively while React owned the singleton must be preserved. + detachDeletedInstance(instance); +} + +function clearSingletonPreambleContribution(instance: Instance): void { + // This path is only used when clearing a dehydrated boundary that contains a + // Fizz preamble contribution marker. The marker tells us which singleton the + // boundary contributed to, but it does not include the contributed props and + // there is no HostSingleton Fiber to provide them. We therefore cannot tell + // which attributes came from React and which were added imperatively by a + // script or third party. For now, clearing every attribute is an accepted + // edge case. + // TODO: Include the contributed properties in the marker so this cleanup can + // remove only the attributes owned by the boundary. const attributes = instance.attributes; while (attributes.length) { instance.removeAttributeNode(attributes[0]); diff --git a/packages/react-dom/src/__tests__/ReactDOMSingletonComponents-test.js b/packages/react-dom/src/__tests__/ReactDOMSingletonComponents-test.js index d887972e92ca..4155c8548566 100644 --- a/packages/react-dom/src/__tests__/ReactDOMSingletonComponents-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMSingletonComponents-test.js @@ -225,6 +225,272 @@ describe('ReactDOM HostSingleton', () => { ); }); + it('resets property-backed state when a singleton is released', async () => { + const root = ReactDOMClient.createRoot(document); + const head = document.head; + const body = document.body; + + root.render( + + {}} /> + {}} + style={{color: 'red'}} + dangerouslySetInnerHTML={{__html: '
managed content
'}} + /> + , + ); + await waitForAll([]); + + expect(document.body).toBe(body); + expect(head.onclick).not.toBe(null); + expect(body.onclick).not.toBe(null); + expect(body.textContent).toBe('managed content'); + expect(body.getAttribute('data-react-owned')).toBe('true'); + expect(body.style.color).toBe('red'); + + // Simulate an inline script or third-party code adding its own attribute, + // style, and click listener while React owns the singleton. + const externalClickHandler = jest.fn(); + body.setAttribute('data-external', 'true'); + body.style.backgroundColor = 'blue'; + body.onclick = externalClickHandler; + + root.render(); + await waitForAll([]); + + expect(document.head).toBe(head); + expect(document.body).toBe(body); + expect(head.onclick).toBe(null); + expect(body.onclick).toBe(externalClickHandler); + expect(body.textContent).toBe(''); + expect(body.hasAttribute('data-react-owned')).toBe(false); + expect(body.getAttribute('data-external')).toBe('true'); + expect(body.style.color).toBe(''); + expect(body.style.backgroundColor).toBe('blue'); + }); + + // @gate TODO + it('clears dangerouslySetInnerHTML when it becomes undefined', async () => { + const root = ReactDOMClient.createRoot(document); + const body = document.body; + const undefinedHTML = undefined; + + root.render( + + + managed content'}} + /> + , + ); + await waitForAll([]); + expect(body.textContent).toBe('managed content'); + + root.render( + + + + , + ); + await waitForAll([]); + + expect(body.textContent).toBe(''); + }); + + // @gate TODO + it('clears dangerouslySetInnerHTML when __html becomes undefined', async () => { + const root = ReactDOMClient.createRoot(document); + const body = document.body; + + root.render( + + + managed content'}} + /> + , + ); + await waitForAll([]); + expect(body.textContent).toBe('managed content'); + + root.render( + + + + , + ); + await waitForAll([]); + + expect(body.textContent).toBe(''); + }); + + it('updates dangerouslySetInnerHTML on a singleton', async () => { + const root = ReactDOMClient.createRoot(document); + const body = document.body; + + root.render( + + + first'}} /> + , + ); + await waitForAll([]); + expect(body.innerHTML).toBe('
first
'); + + root.render( + + + second'}} /> + , + ); + await waitForAll([]); + + expect(body.innerHTML).toBe('second'); + }); + + it('replaces singleton children with dangerouslySetInnerHTML', async () => { + const root = ReactDOMClient.createRoot(document); + const body = document.body; + + root.render( + + + +
managed child
+ + , + ); + await waitForAll([]); + expect(body.innerHTML).toBe('
managed child
'); + + root.render( + + + managed HTML'}} /> + , + ); + await waitForAll([]); + + expect(body.innerHTML).toBe('managed HTML'); + }); + + // @gate TODO + it('replaces dangerouslySetInnerHTML with singleton children', async () => { + const root = ReactDOMClient.createRoot(document); + const body = document.body; + + root.render( + + + managed content'}} + /> + , + ); + await waitForAll([]); + expect(body.innerHTML).toBe('
managed content
'); + + root.render( + + + + managed child + + , + ); + await waitForAll([]); + + expect(body.innerHTML).toBe('managed child'); + }); + + // @gate TODO + it('preserves imperative attributes when acquiring a singleton', async () => { + const body = document.body; + body.setAttribute('data-external', 'true'); + + const root = ReactDOMClient.createRoot(document); + root.render( + + + + , + ); + await waitForAll([]); + + expect(document.body).toBe(body); + expect(body.getAttribute('data-external')).toBe('true'); + }); + + // @gate TODO + it('preserves imperative attributes when clearing a preamble contribution', async () => { + const body = document.body; + body.setAttribute('data-react-owned', 'true'); + body.setAttribute('data-external', 'true'); + // This is the shape Fizz emits when a completed Suspense boundary + // contributes props to the body singleton. + body.innerHTML = '
server
'; + + ReactDOMClient.hydrateRoot( + document, + + + + + client + + + , + { + onRecoverableError() {}, + }, + ); + await waitForAll([]); + + expect(body.textContent).toBe('client'); + expect(body.getAttribute('data-external')).toBe('true'); + }); + + it('does not duplicate native listeners when a singleton is reacquired', async () => { + const root = ReactDOMClient.createRoot(document); + const body = document.body; + const onScroll = jest.fn(); + + root.render( + + + + , + ); + await waitForAll([]); + + body.dispatchEvent(new document.defaultView.Event('scroll')); + expect(onScroll).toHaveBeenCalledTimes(1); + + root.render( + + + , + ); + await waitForAll([]); + + body.dispatchEvent(new document.defaultView.Event('scroll')); + expect(onScroll).toHaveBeenCalledTimes(1); + + root.render( + + + + , + ); + await waitForAll([]); + + expect(document.body).toBe(body); + body.dispatchEvent(new document.defaultView.Event('scroll')); + expect(onScroll).toHaveBeenCalledTimes(2); + }); + it('renders into html, head, and body persistently so the node identities never change and extraneous styles are retained', async () => { // Server render some html that will get replaced with a client render await actIntoEmptyDocument(() => { diff --git a/packages/react-reconciler/src/ReactFiberCommitHostEffects.js b/packages/react-reconciler/src/ReactFiberCommitHostEffects.js index 043db3fdcacc..75129c1e8c9b 100644 --- a/packages/react-reconciler/src/ReactFiberCommitHostEffects.js +++ b/packages/react-reconciler/src/ReactFiberCommitHostEffects.js @@ -826,8 +826,14 @@ export function commitHostSingletonRelease(releasingWork: Fiber) { releasingWork, releaseSingletonInstance, releasingWork.stateNode, + releasingWork.type, + releasingWork.memoizedProps, ); } else { - releaseSingletonInstance(releasingWork.stateNode); + releaseSingletonInstance( + releasingWork.stateNode, + releasingWork.type, + releasingWork.memoizedProps, + ); } }