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 blur() and focusLast() to fragment instances #32654

Merged
merged 3 commits into from
Mar 18, 2025

Conversation

jackpope
Copy link
Contributor

focus() was added in #32465. Here we add focusLast() and blur(). I also extended focus to take options.

focus will focus the first focusable element. focusLast will focus the last focusable element. We could consider a focusFirst naming or even the focusWithin used by test selector APIs as well.

blur will only have an effect if the current document.activeElement is one of the fragment children.

this: FragmentInstanceType,
focusOptions?: FocusOptions,
) {
traverseFragmentInstanceReverse(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively this could be a bit simpler to reuse the traverseFragmentInstance to collect the child nodes and then loop through them applying focus. The tradeoff would be an array allocation and an additional pass through the list, though I don't expect this to get hit too hard and we'd likely break early on the second loop.

Copy link
Collaborator

@sebmarkbage sebmarkbage Mar 18, 2025

Choose a reason for hiding this comment

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

Mainly for code size I think it's probably worth the array allocation in this case since it's a very rare case that doesn't pay for itself.

The other reason is that if this is a very long list of items, then you might hit stack overflow since you need a recursive function for each sibling to allocate the temporary memory anyway. If we had a reverse pointers it would be another story but we don't. So you have to visit all one way or another.

@react-sizebot
Copy link

react-sizebot commented Mar 17, 2025

Comparing: 0237295...6e80692

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.68 kB 6.68 kB +0.05% 1.83 kB 1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 517.29 kB 517.29 kB = 92.26 kB 92.26 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.69 kB 6.69 kB +0.11% 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 605.66 kB 605.55 kB = 107.45 kB 107.43 kB
facebook-www/ReactDOM-prod.classic.js +0.11% 652.02 kB 652.72 kB +0.14% 114.84 kB 115.00 kB
facebook-www/ReactDOM-prod.modern.js +0.11% 642.30 kB 643.00 kB +0.14% 113.26 kB 113.42 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-server/cjs/react-server.production.js = 129.82 kB 129.55 kB = 22.56 kB 22.52 kB
facebook-www/ReactDOMServer-dev.modern.js = 374.85 kB 371.10 kB = 67.22 kB 66.71 kB
facebook-www/ReactDOMServerStreaming-dev.modern.js = 366.55 kB 362.80 kB = 65.90 kB 65.41 kB

Generated by 🚫 dangerJS against 6e80692

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

I'd prefer the temporary array over the extra code and risk of stack overflow.

@jackpope jackpope merged commit c69a5fc into facebook:main Mar 18, 2025
194 checks passed
@jackpope jackpope deleted the fragment-refs-focus branch March 18, 2025 15:58
github-actions bot pushed a commit that referenced this pull request Mar 18, 2025
 was added in #32465.
Here we add  and . I also extended  to take
options.

 will focus the first focusable element.  will focus
the last focusable element. We could consider a  naming or
even the  used by test selector APIs as well.

 will only have an effect if the current
is one of the fragment children.

DiffTrain build for [c69a5fc](c69a5fc)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants