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.

Sign up for GitHub

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

Jump to bottom

Fix toolbar stealing focus #6888

Merged
merged 7 commits into from
Mar 20, 2024
Merged

Fix toolbar stealing focus #6888

merged 7 commits into from
Mar 20, 2024

Conversation

fcollonval
Copy link
Contributor

@fcollonval fcollonval commented Jan 9, 2024 •
edited
Loading

Pull Request

📖 Description

If a toolbar element (e.g. a button) action results in changing the focus to an element somewhere in the page, the active toolbar element should not be focused by the toolbar.

This can be achieved by calling preventDefault() on the event.

🎫 Issues

Fixes #6842

👩‍💻 Reviewer Notes

📑 Test Plan

I added a test with a toolbar button that set the focus to an external element. The test is against click and mouse down events.

✅ Checklist

General

  • I have included a change request file using $ yarn change
  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

⏭ Next Steps

@fcollonval
Copy link
Contributor Author

Thanks @scomea for #6899 - I'll wait for it to update this PR with the utility method you introduced.

@fcollonval fcollonval marked this pull request as ready for review February 20, 2024 10:28
@fcollonval fcollonval requested a review from scomea February 20, 2024 10:29
@fcollonval
Copy link
Contributor Author

@scomea Would you mind having another look at this one?

scomea
scomea requested changes Mar 18, 2024
View reviewed changes
Copy link
Collaborator

@scomea scomea left a comment

Choose a reason for hiding this comment

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

Just the change comments, otherwise ok. Correct those and we'll merge.

change/@microsoft-fast-foundation-65238abc-a169-4169-a830-9dcdb4e9af99.json Outdated Show resolved Hide resolved
change/@microsoft-fast-foundation-65238abc-a169-4169-a830-9dcdb4e9af99.json Outdated Show resolved Hide resolved
@fcollonval
Copy link
Contributor Author

Thanks @scomea and @bheston for the review

@bheston bheston merged commit 5b2b801 into microsoft:master Mar 20, 2024
5 checks passed
@fcollonval fcollonval deleted the patch-1 branch March 20, 2024 10:25
@tlmii tlmii mentioned this pull request Mar 26, 2024
Closed
@chrisdholt chrisdholt mentioned this pull request Apr 5, 2024
Merged
9 tasks
janechu pushed a commit that referenced this pull request Jun 10, 2024
@fcollonval @scomea @janechu
Fix toolbar stealing focus (#6888)
bdb5f4e 
* Fix toolbar stealing focus

* Implement suggestion by @scomea

* Add test

* Use 'getRootActiveElement'

* Change files

* Apply review comment

---------

Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
Co-authored-by: Stephane Comeau <stephcomeau@msn.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scomea scomea scomea approved these changes

@bheston bheston bheston approved these changes

@EisenbergEffect EisenbergEffect Awaiting requested review from EisenbergEffect

@chrisdholt chrisdholt Awaiting requested review from chrisdholt

@radium-v radium-v Awaiting requested review from radium-v

@KingOfTac KingOfTac Awaiting requested review from KingOfTac

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Issue with focus on a toolbar clicked button
3 participants
@fcollonval @scomea @bheston