From b685b40d870b90a975da28c8d22ecf0ba910b1a1 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Fri, 24 Jul 2026 14:38:07 -0700 Subject: [PATCH] [Fiber] only remove properties from singletons on release (#37112) Removing attributes does not actually reset all property state on the singleton instance. It also has the side effect of wiping any 3rd party set attributes and properties that 3rd party scripts and extensions. Reimplements the singleton release to clear properties on the instance based on the last committed props for the singleton fiber. Notably there is an unfixed path with preamble contribution markers that still just clears based on attributes which covers SSR'd attributes that need to be wiped before client recovery on hydration can continue. There are gated failing tests for this as a TODO. --- .../src/client/ReactDOMComponent.js | 30 +- .../src/client/ReactDOMComponentTree.js | 5 +- .../src/client/ReactFiberConfigDOM.js | 44 ++- .../ReactDOMSingletonComponents-test.js | 266 ++++++++++++++++++ .../src/ReactFiberCommitHostEffects.js | 8 +- 5 files changed, 344 insertions(+), 9 deletions(-) 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, + ); } }