Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ref to Fragment (alternative) #32465

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
20 changes: 20 additions & 0 deletions packages/react-art/src/ReactFiberConfigART.js
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,26 @@ export function createTextInstance(
return text;
}

export type FragmentInstance = null | {
_fragmentFiber: Object,
...
};

export function createFragmentInstance(parentInstance): null {
return null;
}

export function commitNewChildToFragmentInstance(
child,
fragmentInstance,
): void {
// Noop
}

export function deleteChildFromFragmentInstance(child, fragmentInstance): void {
// Noop
}

export function finalizeInitialChildren(domElement, type, props) {
return false;
}
Expand Down
209 changes: 209 additions & 0 deletions packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {getCurrentRootHostContainer} from 'react-reconciler/src/ReactFiberHostCo
import hasOwnProperty from 'shared/hasOwnProperty';
import {checkAttributeStringCoercion} from 'shared/CheckStringCoercion';
import {REACT_CONTEXT_TYPE} from 'shared/ReactSymbols';
import {OffscreenComponent} from 'react-reconciler/src/ReactWorkTags';

export {
setCurrentUpdatePriority,
Expand Down Expand Up @@ -1534,6 +1535,214 @@ export function subscribeToGestureDirection(
}
}

type EventListenerOptionsOrUseCapture =
| boolean
| {
capture?: boolean,
once?: boolean,
passive?: boolean,
signal?: AbortSignal,
...
};

type StoredEventListener = {
type: string,
listener: EventListener,
optionsOrUseCapture: void | EventListenerOptionsOrUseCapture,
};

export type FragmentInstance = {
_fragmentFiber: Fiber,
_eventListeners: void | Array<StoredEventListener>,
addEventListener(
type: string,
listener: EventListener,
optionsOrUseCapture?: EventListenerOptionsOrUseCapture,
): void,
removeEventListener(
type: string,
listener: EventListener,
optionsOrUseCapture?: EventListenerOptionsOrUseCapture,
): void,
focus(): void,
};

function FragmentInstancePseudoElement(
this: FragmentInstance,
fragmentFiber: Fiber,
) {
this._fragmentFiber = fragmentFiber;
Copy link
Collaborator

@sebmarkbage sebmarkbage Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You set this only once but that will be the initial Fiber which means that any time you traverse it, you'll observe the initial state of the tree. Because of alternate model you have a 50/50 chance of getting it right but you might actually get the alternate and not current here.

Instead, you need to update this to the new Fiber any time a new one commits. Basically in commitMutationEffectsOnFiber on the Fragment branch you update it.

Technically you only need to update it if there are Placement/Deletion in the subtree but that would hold on to an old Fiber for longer and would be confusing. So you might as well always update it.

Note that this is what we do for HostComponents too but only for props, but we should probably just update the Fiber instead.

https://github.com/facebook/react/blob/main/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js#L773

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the catch! This definitely revealed bugs once I logged the current state of the fiber tree after Placement/Deletion. I just didn't have a test yet that asserted on anything requiring the traversal of a newly added node. I've updated to set current's reference to finishedWork in commitMutationEffectsOnFiber

}
// $FlowFixMe[prop-missing]
FragmentInstancePseudoElement.prototype.addEventListener = function (
this: FragmentInstance,
type: string,
listener: EventListener,
optionsOrUseCapture?: EventListenerOptionsOrUseCapture,
): void {
if (!this._eventListeners) {
this._eventListeners = [];
}

const listeners = this._eventListeners;
// Element.addEventListener will only apply uniquely new event listeners by default. Since we
// need to collect the listeners to apply to appended children, we track them ourselves and use
// custom equality check for the options.
const isNewEventListener =
indexOfEventListener(listeners, {
type,
listener,
optionsOrUseCapture,
}) === -1;
if (isNewEventListener) {
listeners.push({type, listener, optionsOrUseCapture});
traverseFragmentInstanceChildren(
this,
this._fragmentFiber.child,
addEventListenerToChild.bind(null, type, listener, optionsOrUseCapture),
);
}
this._eventListeners = listeners;
};
function addEventListenerToChild(
type: string,
listener: EventListener,
optionsOrUseCapture?: EventListenerOptionsOrUseCapture,
child: Element,
): void {
child.addEventListener(type, listener, optionsOrUseCapture);
}
// $FlowFixMe[prop-missing]
FragmentInstancePseudoElement.prototype.removeEventListener = function (
this: FragmentInstance,
type: string,
listener: EventListener,
optionsOrUseCapture?: EventListenerOptionsOrUseCapture,
): void {
const listeners = this._eventListeners;
if (listeners === undefined || listeners.length === 0) {
return;
}
traverseFragmentInstanceChildren(
this,
this._fragmentFiber.child,
removeEventListenerFromChild.bind(
null,
type,
listener,
optionsOrUseCapture,
),
);
const index = indexOfEventListener(listeners, {
type,
listener,
optionsOrUseCapture,
});
this._eventListeners = listeners.splice(index, 1);
};
function removeEventListenerFromChild(
type: string,
listener: EventListener,
optionsOrUseCapture?: EventListenerOptionsOrUseCapture,
child: Element,
): void {
child.removeEventListener(type, listener, optionsOrUseCapture);
}
// $FlowFixMe[prop-missing]
FragmentInstancePseudoElement.prototype.focus = function (
this: FragmentInstance,
) {
traverseFragmentInstanceChildren(
this,
this._fragmentFiber.child,
setFocusIfFocusable,
);
};

function traverseFragmentInstanceChildren(
fragmentInstance: FragmentInstance,
child: Fiber | null,
fn: (child: Element) => boolean | void,
): void {
while (child !== null) {
if (child.tag === HostComponent) {
if (fn(child.stateNode)) {
return;
}
} else if (
child.tag === OffscreenComponent &&
child.memoizedState !== null
) {
// Skip hidden subtrees
} else {
traverseFragmentInstanceChildren(fragmentInstance, child.child, fn);
}
child = child.sibling;
}
}

function normalizeListenerOptions(
opts: ?EventListenerOptionsOrUseCapture,
): string {
if (opts == null) {
return '0';
}

if (typeof opts === 'boolean') {
return `c=${opts ? '1' : '0'}`;
}

return `c=${opts.capture ? '1' : '0'}&o=${opts.once ? '1' : '0'}&p=${opts.passive ? '1' : '0'}`;
}

function indexOfEventListener(
eventListeners: Array<StoredEventListener>,
eventListenerToFind: StoredEventListener,
): number {
for (let i = 0; i < eventListeners.length; i++) {
const item = eventListeners[i];
if (
item.type === eventListenerToFind.type &&
item.listener === eventListenerToFind.listener &&
normalizeListenerOptions(item.optionsOrUseCapture) ===
normalizeListenerOptions(eventListenerToFind.optionsOrUseCapture)
) {
return i;
}
}
return -1;
}

export function createFragmentInstance(fragmentFiber: Fiber): FragmentInstance {
return new (FragmentInstancePseudoElement: any)(fragmentFiber);
}

export function commitNewChildToFragmentInstance(
childElement: Element,
fragmentInstance: FragmentInstance,
): void {
const eventListeners = fragmentInstance._eventListeners;
if (eventListeners !== undefined) {
for (let i = 0; i < eventListeners.length; i++) {
const {type, listener, optionsOrUseCapture} = eventListeners[i];
childElement.addEventListener(type, listener, optionsOrUseCapture);
}
}
}

export function deleteChildFromFragmentInstance(
childElement: Element,
fragmentInstance: FragmentInstance,
): void {
const eventListeners = fragmentInstance._eventListeners;
if (eventListeners !== undefined) {
for (let i = 0; i < eventListeners.length; i++) {
const {type, listener, optionsOrUseCapture} = eventListeners[i];
childElement.removeEventListener(type, listener, optionsOrUseCapture);
}
}
}

export function clearContainer(container: Container): void {
const nodeType = container.nodeType;
if (nodeType === DOCUMENT_NODE) {
Expand Down
Loading
Loading