Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

[SG-36350] Accessibility: Global Menu: Focus isn't correctly captured when menus are opened #36915

Merged
merged 9 commits into from
Jul 4, 2022
Merged

[SG-36350] Accessibility: Global Menu: Focus isn't correctly captured when menus are opened #36915

merged 9 commits into from
Jul 4, 2022

Conversation

gitstart-sourcegraph
Copy link
Collaborator

@gitstart-sourcegraph gitstart-sourcegraph commented Jun 9, 2022 •
edited by sourcegraph-bot
Loading

Description:

  • Update Popover implementation to stop removing MenuItem from DOM but keep it invisible when menu is not opened.
  • Update DOM and menu styles in :focus-visible state

Refs

SourceGraph Issue
GitStart ticket

Test plan

  • Check on UI to make sure the first menu item is selected by default when menu is opened
  • Keyboard focusing menu item has the common outline style

App preview:

Check out the client app preview documentation to learn more.

@cla-bot cla-bot bot added the cla-signed label Jun 9, 2022
@gitstart-sourcegraph gitstart-sourcegraph self-assigned this Jun 9, 2022
@gitstart-sourcegraph gitstart-sourcegraph linked an issue Jun 9, 2022 that may be closed by this pull request
[Accessibility]: Global <Menu>: Focus isn't moved to first item when menu is opened #36350
Open
4 tasks
@gitstart-sourcegraph gitstart-sourcegraph added the gitstart Contract partner label Jun 9, 2022
gitstart-sourcegraph
gitstart-sourcegraph commented Jun 9, 2022
View reviewed changes
Copy link
Collaborator Author

@gitstart-sourcegraph gitstart-sourcegraph left a comment

Choose a reason for hiding this comment

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

@umpox @valerybugakov

Could you take a look at this PR? We made some hacks on Popover implementation to prevent MenuList from removing from DOM when Popover is hidden.

As far as I can see these changes resolve the issue of losing focus on the first item when Menu is open and MenuList looks great with focus ring.

Just want to have your suggestions before continue fixing test issues

client/wildcard/src/components/Popover/components/popover-content/PopoverContent.tsx Show resolved Hide resolved
client/wildcard/src/components/Popover/tether/services/tether-render.ts Show resolved Hide resolved
@gitstart-sourcegraph gitstart-sourcegraph added the frontend-platform Issues related to our frontend platform, owned collectively by our frontend crew. label Jun 22, 2022
umpox
umpox reviewed Jun 24, 2022
View reviewed changes
Copy link
Contributor

@umpox umpox left a comment

Choose a reason for hiding this comment

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

Few comments, looking further into the forceHidden logic - I think it's OK though

client/wildcard/src/components/Menu/MenuItem.module.scss Outdated Show resolved Hide resolved
client/wildcard/src/components/Menu/MenuItem.tsx Outdated Show resolved Hide resolved
client/wildcard/src/components/Menu/MenuLink.tsx Outdated Show resolved Hide resolved
client/wildcard/src/components/Popover/tether/services/tether-render.ts Show resolved Hide resolved
umpox
umpox approved these changes Jul 4, 2022
View reviewed changes
Copy link
Contributor

@umpox umpox left a comment

Choose a reason for hiding this comment

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

Thanks!

@gitstart-sourcegraph gitstart-sourcegraph merged commit d30b22a into main Jul 4, 2022
@gitstart-sourcegraph gitstart-sourcegraph deleted the contractors/SG-36350 branch July 4, 2022 15:43
@vovakulikov vovakulikov mentioned this pull request Jul 5, 2022
Closed
umpox added a commit that referenced this pull request Jul 6, 2022
@umpox
Revert "[SG-36350] Accessibility: Global Menu: Focus isn't correctl…
4e125eb 
…y captured when menus are opened (#36915)"

This reverts commit d30b22a.
@umpox umpox mentioned this pull request Jul 6, 2022
Merged
umpox added a commit that referenced this pull request Jul 6, 2022
@umpox
Revert "[SG-36350] Accessibility: Global Menu: Focus isn't correctl…
7b64768 
…y captured when menus are opened" (#38298)

Revert "[SG-36350] Accessibility: Global `Menu`: Focus isn't correctly captured when menus are opened (#36915)"

This reverts commit d30b22a.
vovakulikov pushed a commit that referenced this pull request Jul 20, 2022
@gitstart-sourcegraph @gitstart @vovakulikov
[SG-36350] Accessibility: Global Menu: Focus isn't correctly captur…
9fc7adc 
…ed when menus are opened (#36915)

* feat: keep MenuList in DOM when Menu is hidden

Co-authored-by: gitstart-sourcegraph <[email protected]>
vovakulikov pushed a commit that referenced this pull request Jul 20, 2022
@umpox @vovakulikov
Revert "[SG-36350] Accessibility: Global Menu: Focus isn't correctl…
4bcd262 
…y captured when menus are opened" (#38298)

Revert "[SG-36350] Accessibility: Global `Menu`: Focus isn't correctly captured when menus are opened (#36915)"

This reverts commit d30b22a.
@sourcegraph-tracking-issue-bot sourcegraph-tracking-issue-bot mentioned this pull request Aug 2, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vovakulikov vovakulikov vovakulikov left review comments

@umpox umpox umpox approved these changes

@valerybugakov valerybugakov Awaiting requested review from valerybugakov

@oleggromov oleggromov Awaiting requested review from oleggromov

@limitedmage limitedmage Awaiting requested review from limitedmage

Assignees

@gitstart-sourcegraph gitstart-sourcegraph

Labels
cla-signed frontend-platform Issues related to our frontend platform, owned collectively by our frontend crew. gitstart Contract partner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Accessibility]: Global <Menu>: Focus isn't moved to first item when menu is opened
4 participants
@gitstart-sourcegraph @umpox @vovakulikov @gitstart