Skip to content

Fix focus set for delegated and already focused elements#36010

Open
jackpope wants to merge 1 commit intofacebook:mainfrom
jackpope:setfocusfixes
Open

Fix focus set for delegated and already focused elements#36010
jackpope wants to merge 1 commit intofacebook:mainfrom
jackpope:setfocusfixes

Conversation

@jackpope
Copy link
Contributor

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.

  1. 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.

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.
@react-sizebot
Copy link

Comparing: 7b5b561...ea8b0fb

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.84 kB 6.84 kB = 1.88 kB 1.88 kB
oss-stable/react-dom/cjs/react-dom-client.production.js +0.02% 611.79 kB 611.88 kB +0.01% 108.12 kB 108.13 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.84 kB 6.84 kB = 1.88 kB 1.88 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js +0.01% 677.72 kB 677.81 kB +0.01% 119.08 kB 119.09 kB
facebook-www/ReactDOM-prod.classic.js +0.01% 697.67 kB 697.76 kB +0.01% 122.58 kB 122.60 kB
facebook-www/ReactDOM-prod.modern.js +0.01% 687.98 kB 688.08 kB +0.01% 120.96 kB 120.97 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against ea8b0fb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants