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

[TreeView] Check if the active element is a treeitem #4484

Merged
merged 7 commits into from
Apr 24, 2024

Conversation

JelloBagel
Copy link
Contributor

@JelloBagel JelloBagel commented Apr 10, 2024

When navigating through the TreeView, the focus should be on the list item. However, when a user clicks on a secondary action inside the list, the user is no longer able to focus back on the list item when navigating to the list. This PR is to fix the focus in TreeView when active element is not a treeitem

Before
  1. Navigate through the list with tab.
  2. Focus is on the list item (expected)
  3. Click on the secondary action
  4. Try to navigate to the list
  5. Focus is on the secondary action (unexpected)
Screen.Recording.2024-04-09.at.5.50.02.PM.mov
After
  1. Click on the secondary action
  2. Try to navigate to the list
  3. Focus is on the list item
Untitled.mov

Changelog

New

Changed

Ensures the document.activeElement is of role=treeitem before returning focus to the TreeView listitem

Removed

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

@JelloBagel JelloBagel requested a review from a team as a code owner April 10, 2024 00:18
Copy link

changeset-bot bot commented Apr 10, 2024

🦋 Changeset detected

Latest commit: b3c1107

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Apr 10, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 87.98 KB (-0.07% 🔽)
packages/react/dist/browser.umd.js 88.28 KB (-0.1% 🔽)

@github-actions github-actions bot temporarily deployed to storybook-preview-4484 April 10, 2024 00:22 Inactive
@siddharthkp siddharthkp added the no-breaking-changes: need to test This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Apr 16, 2024
@siddharthkp
Copy link
Member

Hi!

Sorry, I don't understand the before video and how this change would fix it.

Can you please describe what's happening in the video and what should happen after. Thanks!

@JelloBagel
Copy link
Contributor Author

@siddharthkp I updated the PR description. Let me know if it needs anymore clarification

@siddharthkp
Copy link
Member

siddharthkp commented Apr 19, 2024

Thank you adding more context ❤️

Can you also please add a story and test in primer/react so that we don't create a regression in the future, thank you! I'm happy to work on this together if you'd like

@@ -891,6 +898,57 @@ export const InitialFocus: Story = () => (
</div>
)

export const FocusManagement: Story = () => (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@siddharthkp added a story and test in 4df63b8

Screen.Recording.2024-04-19.at.12.24.32.PM.mov

Copy link
Member

Choose a reason for hiding this comment

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

Thank you! ❤️

@siddharthkp
Copy link
Member

@siddharthkp siddharthkp added no-breaking-changes: confirmed Tested this change doesn't break any existing instances in applications using changed components and removed no-breaking-changes: need to test This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Apr 24, 2024
@github-actions github-actions bot temporarily deployed to storybook-preview-4484 April 24, 2024 14:39 Inactive
@siddharthkp siddharthkp added this pull request to the merge queue Apr 24, 2024
Merged via the queue into main with commit 1c847bd Apr 24, 2024
30 checks passed
@siddharthkp siddharthkp deleted the jellobagel-treeitem branch April 24, 2024 15:04
@primer primer bot mentioned this pull request Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-breaking-changes: confirmed Tested this change doesn't break any existing instances in applications using changed components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants