Skip to content

Commit

Permalink
fix: restore selection should consider the window of the container (#…
Browse files Browse the repository at this point in the history
…30951)

## Summary


Fixes #30864 

Before this PR the active elemen was always taken from the global
`window`. This is incorrect if the renderer is in one window rendering
into a container element in another window. The changes in this PR adds
another code branch to use a `defaultView` of the container element if
it exists so that `restoreSelection` after a commit will actually
restore to the correct window.

## How did you test this change?

I patched these changes to the repro repo in the linked issue #39864
https://github.com/ling1726/react-child-window-focus-repro/blob/master/patches/react-dom%2B18.3.1.patch.

I followed the same repro steps in the linked issue and and could not
repro the reported problem. Attaching screen recordings below:

Before
![focus
repro](https://github.com/user-attachments/assets/81c4b4f9-08b5-4356-8251-49b909771f3f)

After

![after](https://github.com/user-attachments/assets/84883032-5558-4650-9b9a-bd4d5fd9cb13)
  • Loading branch information
ling1726 authored Sep 13, 2024
1 parent d3d4d3a commit d9c4920
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 9 deletions.
4 changes: 2 additions & 2 deletions packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ export function getPublicInstance(instance: Instance): Instance {

export function prepareForCommit(containerInfo: Container): Object | null {
eventsEnabled = ReactBrowserEventEmitterIsEnabled();
selectionInformation = getSelectionInformation();
selectionInformation = getSelectionInformation(containerInfo);
let activeInstance = null;
if (enableCreateEventHandleAPI) {
const focusedElem = selectionInformation.focusedElem;
Expand Down Expand Up @@ -357,7 +357,7 @@ export function afterActiveInstanceBlur(): void {
}

export function resetAfterCommit(containerInfo: Container): void {
restoreSelection(selectionInformation);
restoreSelection(selectionInformation, containerInfo);
ReactBrowserEventEmitterSetEnabled(eventsEnabled);
eventsEnabled = null;
selectionInformation = null;
Expand Down
14 changes: 7 additions & 7 deletions packages/react-dom-bindings/src/client/ReactInputSelection.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@ function isSameOriginFrame(iframe) {
}
}

function getActiveElementDeep() {
let win = window;
let element = getActiveElement();
function getActiveElementDeep(containerInfo) {
let win = containerInfo?.ownerDocument?.defaultView ?? window;
let element = getActiveElement(win.document);
while (element instanceof win.HTMLIFrameElement) {
if (isSameOriginFrame(element)) {
win = element.contentWindow;
Expand Down Expand Up @@ -97,8 +97,8 @@ export function hasSelectionCapabilities(elem) {
);
}

export function getSelectionInformation() {
const focusedElem = getActiveElementDeep();
export function getSelectionInformation(containerInfo) {
const focusedElem = getActiveElementDeep(containerInfo);
return {
focusedElem: focusedElem,
selectionRange: hasSelectionCapabilities(focusedElem)
Expand All @@ -112,8 +112,8 @@ export function getSelectionInformation() {
* restore it. This is useful when performing operations that could remove dom
* nodes and place them back in, resulting in focus being lost.
*/
export function restoreSelection(priorSelectionInformation) {
const curFocusedElem = getActiveElementDeep();
export function restoreSelection(priorSelectionInformation, containerInfo) {
const curFocusedElem = getActiveElementDeep(containerInfo);
const priorFocusedElem = priorSelectionInformation.focusedElem;
const priorSelectionRange = priorSelectionInformation.selectionRange;
if (curFocusedElem !== priorFocusedElem && isInDocument(priorFocusedElem)) {
Expand Down
55 changes: 55 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFiber-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,23 @@ let act;
let assertConsoleErrorDev;
let assertLog;
let root;
let JSDOM;

describe('ReactDOMFiber', () => {
let container;

beforeEach(() => {
jest.resetModules();

// JSDOM needs to be setup with a TextEncoder and TextDecoder when used standalone
// https://github.com/jsdom/jsdom/issues/2524
(() => {
const {TextEncoder, TextDecoder} = require('util');
global.TextEncoder = TextEncoder;
global.TextDecoder = TextDecoder;
JSDOM = require('jsdom').JSDOM;
})();

React = require('react');
ReactDOM = require('react-dom');
PropTypes = require('prop-types');
Expand Down Expand Up @@ -1272,4 +1283,48 @@ describe('ReactDOMFiber', () => {
});
expect(didCallOnChange).toBe(true);
});

it('should restore selection in the correct window', async () => {
// creating new JSDOM instance to get a second window as window.open is not implemented
// https://github.com/jsdom/jsdom/blob/c53efc81e75f38a0558fbf3ed75d30b78b4c4898/lib/jsdom/browser/Window.js#L987
const {window: newWindow} = new JSDOM('');
// creating a new container since the default cleanup expects the existing container to be in the document
const newContainer = newWindow.document.createElement('div');
newWindow.document.body.appendChild(newContainer);
root = ReactDOMClient.createRoot(newContainer);

const Test = () => {
const [reverse, setReverse] = React.useState(false);
const [items] = React.useState(() => ['a', 'b', 'c']);
const onClick = () => {
setReverse(true);
};

// shuffle the items so that the react commit needs to restore focus
// to the correct element after commit
const itemsToRender = reverse ? items.reverse() : items;

return (
<div>
{itemsToRender.map(item => (
<button onClick={onClick} key={item} id={item}>
{item}
</button>
))}
</div>
);
};

await act(() => {
root.render(<Test />);
});

newWindow.document.getElementById('a').focus();
await act(() => {
newWindow.document.getElementById('a').click();
});

expect(newWindow.document.activeElement).not.toBe(newWindow.document.body);
expect(newWindow.document.activeElement.innerHTML).toBe('a');
});
});

0 comments on commit d9c4920

Please sign in to comment.