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 list scrolling in Commit Reachability Dialog #17421

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

tidy-dev
Copy link
Contributor

Description

We found that the list in the unreachable commits dialog had an odd bug where it continually moved focus to the first list item.

Screen.Recording.2023-09-21.at.16.56.39.mov

This was due to an accessibility fix that is meant to focus the first list item if no selected rows are provided.

The reason it impacts this list in particular is that we simply send in [] for selected rows and do not manage selected rows. The expectation of the above code is that we after setting focus to the first index, we bubble up a selection change and store that in a parent component. Then, the code to move to the first index would not fire again because the selected rows would no longer be [] .. but here we are just always sending in the [] instead of managing it.

This bit of logic scrolling logic fires on the onFocusWithinChanged which makes sense as to why scrolling works when you are not actively focusing the list.

Other notes:

  • We could further discuss making the onSelectionChange with documentation that parent component selected row management is required in the List and SectionList component required to ensure this kind of bug doesn't repeat itself.... or another way to address the focus first element issue if require parent component selected row management is not the route we want.
  • The only other unmanaged selected row lists are in the test-notifications.tsx, I did not test or address those as they are not user facing and was looking to target this PR fix.

Screenshots

Screen.Recording.2023-09-21.at.12.12.14.PM.mov

Release notes

Notes: [Fixed] Scrolling works as expected in the "Commit Reachability" dialog.

Copy link
Member

@sergiou87 sergiou87 left a comment

Choose a reason for hiding this comment

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

Looks good and works perfectly, thanks for taking care of this!! ❤️

@tidy-dev tidy-dev merged commit 5ac8425 into development Sep 22, 2023
8 checks passed
@tidy-dev tidy-dev deleted the Fix-list-glitch-in-unreachable-commits-dialog branch September 22, 2023 11:07
@BryanQuizlet
Copy link

Would this have happened to change this behavior? #17673 The window used to scroll, allowing me to select large blocks of text very quickly. Now it doesn't scroll at all.

@tidy-dev tidy-dev added the accessibility Issues related to accessibility improvements label Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Issues related to accessibility improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants