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

fix: a11y: arrow-key navigation stopping working after ~10 keypresses #4441

Merged
merged 7 commits into from
Jan 12, 2025

Conversation

WofWca
Copy link
Collaborator

@WofWca WofWca commented Dec 24, 2024

Reviewing commit-by-commit should be a bit easier. But basically what this does is it un-inlines the renderer functions, addressing this issue: bvaughn/react-window#420 (comment).
You can check this behavior by arrow-key navigating the contact list in "new chat", as long as your contact list is at least ~30 items long. When new items get loaded (with 'react-infinite-loader'), the parent component re-renders, and all items' DOM get re-created, losing focus.

I am not 100% sure whether we should squash the commits. They're all quite similar, but it might not be easy to see separate logical changes when squashed.

Copy link
Member

@nicodh nicodh left a comment

Choose a reason for hiding this comment

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

What issue is fixed here? How can I test if this PR solves the issue?
Which means what is the use case for the parent component being rerendered and items loosing focus?

@WofWca
Copy link
Collaborator Author

WofWca commented Jan 10, 2025

I just edited the description:

You can check this behavior by arrow-key navigating the contact list in "new chat", as long as your contact list is at least ~30 items long. When new items get loaded (with 'react-infinite-loader'), the parent component re-renders, and all items' DOM get re-created, losing focus.

Copy link
Member

@nicodh nicodh left a comment

Choose a reason for hiding this comment

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

Reviewed by testing.

Maybe squashing is fine, since most commits are applied to different files

...upon re-render.
See bvaughn/react-window#420 (comment).

This commit simply moves the renderer function definition
from inline to top-level.
...upon re-render.
See bvaughn/react-window#420 (comment).

This commit simply moves the renderer function definition
from inline to top-level.
...upon re-render.
See bvaughn/react-window#420 (comment).

This commit simply moves the renderer function definition
from inline to top-level.
...upon re-render.
See bvaughn/react-window#420 (comment).

This commit ensures that the renderer function is defined
at the top-level, and not inline.
See bvaughn/react-window#420 (comment).

This commit simply moves the renderer function definition
from inline to top-level.

This does not appear to have functional impact as of now,
but will come in handy after #4376.
See bvaughn/react-window#420 (comment).

This commit simply moves the renderer function definition
from inline to top-level.

This does not appear to have functional impact as of now,
but will come in handy after #4376.
@WofWca WofWca force-pushed the wofwca/fix-roving-tabindex-react-window branch from 9db5c71 to 0230587 Compare January 12, 2025 18:14
@WofWca WofWca merged commit 5c2cdbb into main Jan 12, 2025
14 checks passed
@WofWca WofWca deleted the wofwca/fix-roving-tabindex-react-window branch January 12, 2025 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants