Skip to content

Commit c80a075

Browse files
authored
Fix focus set for delegated and already focused elements (facebook#36010)
I found two focus bugs when working on documentation for Fragment Refs. 1) If an element delegates focus handling, it will return false from setFocusIfFocusable even though a focus event has occured on a different element. The fix for this is a document level event listener rather than only listening on the current element. For example, if you have a form with multiple nested label>inputs. Calling focus on the label will focus its input but not fire an event on the label. setFocusIfFocusable returns false and you end up continuing to attempt focus down the form tree. 2) If an element is already focused, setFocusIfFocusable will return false. The fix for this is checking the document's activeElement with an early return. In the same form example, if the first input is already focused and you call fragmentInstance.focus() at the form level, the second input would end up getting focused since the focus event on the first is not triggered.
1 parent 8f41506 commit c80a075

File tree

2 files changed

+111
-3
lines changed

2 files changed

+111
-3
lines changed

packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4520,18 +4520,30 @@ export function setFocusIfFocusable(
45204520
//
45214521
// We could compare the node to document.activeElement after focus,
45224522
// but this would not handle the case where application code managed focus to automatically blur.
4523+
const element = ((node: any): HTMLElement);
4524+
4525+
// If this element is already the active element, it's focusable and already
4526+
// focused. Calling .focus() on it would be a no-op (no focus event fires),
4527+
// so we short-circuit here.
4528+
if (element.ownerDocument.activeElement === element) {
4529+
return true;
4530+
}
4531+
45234532
let didFocus = false;
45244533
const handleFocus = () => {
45254534
didFocus = true;
45264535
};
45274536

4528-
const element = ((node: any): HTMLElement);
45294537
try {
4530-
element.addEventListener('focus', handleFocus);
4538+
// Listen on the document in the capture phase so we detect focus even when
4539+
// it lands on a different element than the one we called .focus() on. This
4540+
// happens with <label> elements (focus delegates to the associated input)
4541+
// and shadow hosts with delegatesFocus.
4542+
element.ownerDocument.addEventListener('focus', handleFocus, true);
45314543
// $FlowFixMe[method-unbinding]
45324544
(element.focus || HTMLElement.prototype.focus).call(element, focusOptions);
45334545
} finally {
4534-
element.removeEventListener('focus', handleFocus);
4546+
element.ownerDocument.removeEventListener('focus', handleFocus, true);
45354547
}
45364548

45374549
return didFocus;

packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -255,6 +255,102 @@ describe('FragmentRefs', () => {
255255
expect(document.activeElement.id).toEqual('child-b');
256256
document.activeElement.blur();
257257
});
258+
259+
// @gate enableFragmentRefs
260+
it('keeps focus on the first focusable child if already focused', async () => {
261+
const fragmentRef = React.createRef();
262+
const root = ReactDOMClient.createRoot(container);
263+
264+
function Test() {
265+
return (
266+
<Fragment ref={fragmentRef}>
267+
<a id="child-a" href="/">
268+
A
269+
</a>
270+
<a id="child-b" href="/">
271+
B
272+
</a>
273+
</Fragment>
274+
);
275+
}
276+
277+
await act(() => {
278+
root.render(<Test />);
279+
});
280+
281+
// Focus the first child manually
282+
document.getElementById('child-a').focus();
283+
expect(document.activeElement.id).toEqual('child-a');
284+
285+
// Calling fragment.focus() should keep focus on child-a,
286+
// not skip to child-b
287+
await act(() => {
288+
fragmentRef.current.focus();
289+
});
290+
expect(document.activeElement.id).toEqual('child-a');
291+
document.activeElement.blur();
292+
});
293+
294+
// @gate enableFragmentRefs
295+
it('keeps focus on a nested child if already focused', async () => {
296+
const fragmentRef = React.createRef();
297+
const root = ReactDOMClient.createRoot(container);
298+
299+
function Test() {
300+
return (
301+
<Fragment ref={fragmentRef}>
302+
<div>
303+
<input id="nested-input" />
304+
</div>
305+
<a id="sibling-link" href="/">
306+
Link
307+
</a>
308+
</Fragment>
309+
);
310+
}
311+
312+
await act(() => {
313+
root.render(<Test />);
314+
});
315+
316+
// Focus the nested input manually
317+
document.getElementById('nested-input').focus();
318+
expect(document.activeElement.id).toEqual('nested-input');
319+
320+
// Calling fragment.focus() should keep focus on nested-input
321+
await act(() => {
322+
fragmentRef.current.focus();
323+
});
324+
expect(document.activeElement.id).toEqual('nested-input');
325+
document.activeElement.blur();
326+
});
327+
328+
// @gate enableFragmentRefs
329+
it('focuses the first focusable child in a fieldset', async () => {
330+
const fragmentRef = React.createRef();
331+
const root = ReactDOMClient.createRoot(container);
332+
333+
function Test() {
334+
return (
335+
<Fragment ref={fragmentRef}>
336+
<fieldset>
337+
<legend>Shipping</legend>
338+
<input id="street" name="street" />
339+
<input id="city" name="city" />
340+
</fieldset>
341+
</Fragment>
342+
);
343+
}
344+
345+
await act(() => {
346+
root.render(<Test />);
347+
});
348+
await act(() => {
349+
fragmentRef.current.focus();
350+
});
351+
expect(document.activeElement.id).toEqual('street');
352+
document.activeElement.blur();
353+
});
258354
});
259355

260356
describe('focusLast()', () => {

0 commit comments

Comments
 (0)