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

SelectPanel2: Keyboard navigation for list #3768

Merged
merged 49 commits into from
Oct 26, 2023

Conversation

siddharthkp
Copy link
Member

@siddharthkp siddharthkp commented Sep 26, 2023


Before focus zone for list:

before-list-focus-zone.mov

After focus zone for list:

after-list-focus-zone.mov

Notice how the list remembers the last item that had focus and jumps back to that 😇 (thanks @T-Hugs who wrote the original behaviors)

@github-actions github-actions bot temporarily deployed to storybook-preview-3768 September 27, 2023 21:00 Inactive
@siddharthkp siddharthkp changed the title (WIP) SelectPanel2: Keyboard navigation SelectPanel2: Keyboard navigation Sep 27, 2023
@siddharthkp siddharthkp changed the title SelectPanel2: Keyboard navigation SelectPanel2: Keyboard navigation for list Sep 27, 2023
@siddharthkp siddharthkp marked this pull request as ready for review September 29, 2023 12:45
@siddharthkp siddharthkp requested a review from a team September 29, 2023 12:45
@T-Hugs
Copy link
Contributor

T-Hugs commented Sep 29, 2023

I'm glad they're still useful :)

Base automatically changed from drafts-selectpanel-simplify-actionlist to main October 2, 2023 12:23
@siddharthkp siddharthkp temporarily deployed to github-pages October 2, 2023 17:59 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3768 October 2, 2023 17:59 Inactive
Copy link
Member

@broccolinisoup broccolinisoup left a comment

Choose a reason for hiding this comment

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

Code looks good! Two things to note from my test: (Let me know if I misunderstood anything 🙌🏻 )

  1. Tested it on Chrome and tried selecting the item with the space key. (Not sure if it is configured yet but it was my intuition to select with the space key.) When the item is selected with the space key, the list shifts.
select-with-space.mp4

Vide description: Moving the focus with tab key to one of the selectable list items and presses the space key. The list jumps and the selected item is not visible anymore.

  1. Tested it on Firefox. When the focus is moved to the list, the focus ring is not visible. It becomes visible if I go to the footer with tab and come back to the list.
firefox-focus-ring.mp4

Vide description: Moving the focus with tab key to the list zone However when the focus is in the list zone, there is no visual indicator to display the current focus. The expected focus ring comes back on the list item after moving to and back from the footer area.

@siddharthkp
Copy link
Member Author

When the item is selected with the space key, the list shifts.

Yes!

That is a bug in ActionList that I'm tracking in the epic come back to.

@siddharthkp siddharthkp changed the title SelectPanel2: Keyboard navigation for list Experimental SelectPanel2: Keyboard navigation for list Oct 19, 2023
@broccolinisoup
Copy link
Member

Tested it on Firefox. When the focus is moved to the list, the focus ring is not visible. It becomes visible if I go to the footer with tab and come back to the list.

Just want to resurface my other question in case it got lost. (reference) Let me know if you address it in a different PR too 🙌🏻

Copy link
Member

@broccolinisoup broccolinisoup left a comment

Choose a reason for hiding this comment

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

Tested it on Firefox. When the focus is moved to the list, the focus ring is not visible. It becomes visible if I go to the footer with tab and come back to the list.

Just want to resurface my other question in case it got lost. (#3768 (review)) Let me know if you address it in a different PR too 🙌🏻

@siddharthkp
Copy link
Member Author

siddharthkp commented Oct 24, 2023

Tested it on Firefox. When the focus is moved to the list, the focus ring is not visible. It becomes visible if I go to the footer with tab and come back to the list.

Wow! Thanks for flaging this again, I missed this in a hurry. It's strange because the ul list element is getting tabIndex in firefox but not in webkit 🤔

Let me know if you address it in a different PR too 🙌🏻

Yes. I'd like to merge this one first. Keeping it open is making me nervous 😅

I think I will try to reproduce the firefox bug with primer/behaviors first and then decide if the fix is in primer/react or primer/behaviors

@siddharthkp siddharthkp changed the title Experimental SelectPanel2: Keyboard navigation for list SelectPanel2: Keyboard navigation for list Oct 24, 2023
Copy link
Collaborator

@pksjce pksjce left a comment

Choose a reason for hiding this comment

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

Tested this branch and I can confirm the same two issues already noted.

  1. When you hit Spacebar to select an item in the list, it scrolls down. Enter key works properly.
  2. On firefox, the focus ring is lost when the focus hits the listitems. Pressing down key results in focus ring coming back and shifting to the next expected item.

Another issue adjacent to the above is
In Chrome and Safari, the list "remembers" the last focused item, but in case of Firefox it starts from the beginning of the list. It may be related to the above focus issue.
Happy for this to be taken up in a separate PR.

All of the rest looks great to me! Love how the focus remaining firmly inside the overlay!

@siddharthkp siddharthkp added this pull request to the merge queue Oct 26, 2023
Merged via the queue into main with commit 49dfc0e Oct 26, 2023
34 checks passed
@siddharthkp siddharthkp deleted the drafts-selectpanel-keyboard branch October 26, 2023 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changeset This change does not need a changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants