Tentative focus fix in action list item #4166

pksjce · 2024-01-22T12:15:16Z

Closes https://github.com/github/accessibility-audits/issues/4641

Bug description
The bug specifically occurs in one case where the action menu does not have a visible anchor and is programmatically opened through a keyboard shortcut combination. Specifically in the code viewer, on hitting the SHIFT + CTRL + C combination, you can see the actionmenu emerge at the cursor position. This action menu can also be accessed at the line level through an anchor thats only visible on focus.
The actual bug is that the focus does not shift from the cursor position into the action menu's first item as expected.
This bug has been especially challenging because it is hard to reproduce in it's entireity. One can fiddle around with tab button and fix the focus params such that the bug becomes nonexistent. Also I was not successful in creating a minimum reproduction of the issue in a storybook. #4025 . Any advise on fixing this is welcome.

Root cause analysis
The current theory is that as per the spec on focus-visible https://drafts.csswg.org/selectors/#indicate-focus , these lines tell us what could the problem be.

"If the previously-focused element indicated focus, and a script causes focus to move elsewhere, indicate focus on the newly focused element.
Conversely, if the previously-focused element did not indicate focus, and a script causes focus to move elsewhere, don’t indicate focus on the newly focused element."

So if the focus shifts programmatically then the spec does not require to apply the focus-visible pseudo class.

Proposed fix
After much deliberation, I think our best chance is to fix the issue to our best of knowledge and see if it fixes the actual issue.
We will make sure that the first item in action menu always receives focus when it opens by more robust css. This should be the right solution.

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

Merge checklist

Added/updated tests
Added/updated documentation
Added/updated previews (Storybook)
Changes are SSR compatible
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge
(GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

changeset-bot · 2024-01-22T12:15:20Z

🦋 Changeset detected

Latest commit: ecb8af8

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

github-actions · 2024-01-22T12:17:50Z

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	113.01 KB (+0.01% 🔺)
packages/react/dist/browser.umd.js	113.67 KB (+0.01% 🔺)

joshblack · 2024-01-26T21:04:39Z

@pksjce was trying to re-create this issue again on dotcom and am having trouble getting the shortcut to trigger the menu 🤔 I just get the screen jumping all the way to the bottom. Was curious if others are observing the same or if it was just me.

TylerJDev · 2024-01-26T21:25:31Z

@joshblack, yup I'm getting the same behavior. I've asked this in one of the slack channels to see if this functionality was removed or not.

pksjce · 2024-02-01T11:03:48Z

Hi @TylerJDev @joshblack , I see that they seemed to have fixed this. Though I'm still having some challenges getting it working. I'm trying to get a review lab out so we can test the fix

TylerJDev · 2024-02-02T14:28:25Z

@pksjce, @joshblack, I'm now able to open the menu in code view. Let me know if you need an additional look within the review lab too!

TylerJDev · 2024-02-02T15:03:57Z

I also noticed a bug that this PR seems to fix. Opening up an ActionMenu within Safari via mouse and navigating with arrow keys won't produce any focus indication.

Screen.Recording.2024-02-02.at.9.35.02.AM.mov

Video description: I'm in a Storybook story for ActionMenu on Safari, and I open the menu with my mouse. Afterwards I navigate through the menu using both up/down arrow keys. As I navigate through the menu, the focus indicator is not present. It only appears when I press any other key outside of up/down arrow keys.

@pksjce, @joshblack, could y'all try reproducing this to see if it's the same behavior for all of us? Here are the reproduction steps:

Open up any Storybook story for ActionMenu in Safari
Open the menu with your mouse
As soon as the menu is open, try navigating the menu with either Up Arrow or Down Arrow
Observe if focus indicator is present or not

joshblack · 2024-02-06T19:08:31Z

@TylerJDev is this an implementation behavior of Safari where it does not show focus if the menu is opened with a click and then keyboard is used? The list items clearly have focus, and :focus-visible styling works in Safari, but it does not seem to be triggered in this case 🤔

The fix above totally works because the focus-visible polyfill adds the appropriate classes/attributes so the styling kicks in but was wondering if this was a behavior that Safari has implemented or is something else at play.

Bumps [@babel/cli](https://github.com/babel/babel/tree/HEAD/packages/babel-cli) from 7.22.10 to 7.23.9. - [Release notes](https://github.com/babel/babel/releases) - [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md) - [Commits](https://github.com/babel/babel/commits/v7.23.9/packages/babel-cli) --- updated-dependencies: - dependency-name: "@babel/cli" dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [terser](https://github.com/terser/terser) from 5.17.6 to 5.27.0. - [Changelog](https://github.com/terser/terser/blob/master/CHANGELOG.md) - [Commits](terser/terser@v5.17.6...v5.27.0) --- updated-dependencies: - dependency-name: terser dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…4186) Bumps [@storybook/source-loader](https://github.com/storybookjs/storybook/tree/HEAD/code/lib/source-loader) from 7.6.7 to 7.6.10. - [Release notes](https://github.com/storybookjs/storybook/releases) - [Changelog](https://github.com/storybookjs/storybook/blob/next/CHANGELOG.md) - [Commits](https://github.com/storybookjs/storybook/commits/v7.6.10/code/lib/source-loader) --- updated-dependencies: - dependency-name: "@storybook/source-loader" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Improve aria-labels for example icon buttons and add guidance * test(vrt): update snapshots --------- Co-authored-by: joshblack <[email protected]>

* chore(deps): bump @github/combobox-nav from 2.1.7 to 2.3.1 Bumps [@github/combobox-nav](https://github.com/github/combobox-nav) from 2.1.7 to 2.3.1. - [Release notes](https://github.com/github/combobox-nav/releases) - [Commits](github/combobox-nav@v2.1.7...v2.3.1) --- updated-dependencies: - dependency-name: "@github/combobox-nav" dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * test: add fallback for Element#scrollIntoView * chore: fix eslint warning --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Josh Black <[email protected]>

* oof * Create thirty-mirrors-switch.md * test(vrt): update snapshots * fix missing var * fix parens * test(vrt): update snapshots --------- Co-authored-by: langermank <[email protected]>

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

joshblack

LGTM to fix this in the interim while we figure out what on earth is going on 😅

pksjce requested review from a team and broccolinisoup January 22, 2024 12:15

pksjce requested review from TylerJDev and joshblack January 22, 2024 12:15

pksjce temporarily deployed to github-pages January 22, 2024 12:19 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4166 January 22, 2024 12:20 Inactive

TylerJDev mentioned this pull request Jan 22, 2024

refactor(project): drop focus-visible polyfill #4168

Closed

1 task

pksjce added 2 commits January 25, 2024 20:52

Tentative focus fix in action list item

ee0053e

Update snapshots

a2beb45

pksjce force-pushed the pk/fix-action-menu-foxus branch from fba7e40 to a2beb45 Compare January 25, 2024 09:59

pksjce temporarily deployed to github-pages January 25, 2024 10:02 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4166 January 25, 2024 10:03 Inactive

Create purple-bees-warn.md

4c061fd

pksjce temporarily deployed to github-pages January 25, 2024 10:08 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4166 January 25, 2024 10:08 Inactive

Merge branch 'main' into pk/fix-action-menu-foxus

5f01c80

pksjce deployed to github-pages January 29, 2024 03:46 — with GitHub Actions View deployment

github-actions bot temporarily deployed to storybook-preview-4166 January 29, 2024 03:46 Inactive

dependabot bot and others added 4 commits February 8, 2024 12:03

Improve aria-labels for example icon buttons and add guidance (#4191)

5d47efb

* Improve aria-labels for example icon buttons and add guidance * test(vrt): update snapshots --------- Co-authored-by: joshblack <[email protected]>

dependabot bot and others added 5 commits February 8, 2024 12:03

Add missing CSS vars to legacy-theme (#4193)

0df9985

* oof * Create thirty-mirrors-switch.md * test(vrt): update snapshots * fix missing var * fix parens * test(vrt): update snapshots --------- Co-authored-by: langermank <[email protected]>

Version Packages (#4183)

be64070

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Merge branch 'main' into pk/fix-action-menu-foxus

fc9f7bd

Merge branch 'main' into pk/fix-action-menu-foxus

4382e3c

pksjce deployed to github-pages February 8, 2024 01:12 — with GitHub Actions View deployment

github-actions bot temporarily deployed to storybook-preview-4166 February 8, 2024 01:13 Inactive

Merge branch 'main' into pk/fix-action-menu-foxus

ecb8af8

pksjce deployed to github-pages February 8, 2024 11:37 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-4166 February 8, 2024 11:37 View deployment

joshblack approved these changes Feb 8, 2024

View reviewed changes

TylerJDev approved these changes Feb 8, 2024

View reviewed changes

pksjce added this pull request to the merge queue Feb 9, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 9, 2024

pksjce removed the request for review from broccolinisoup February 12, 2024 00:13

pksjce added this pull request to the merge queue Feb 12, 2024

Merged via the queue into main with commit c66b808 Feb 12, 2024

pksjce deleted the pk/fix-action-menu-foxus branch February 12, 2024 00:23

primer bot mentioned this pull request Feb 11, 2024

Release tracking #4222

Merged

TylerJDev mentioned this pull request Mar 1, 2024

Focus-visible ActionMenu link fix #4334

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Tentative focus fix in action list item #4166

Tentative focus fix in action list item #4166

Uh oh!

pksjce commented Jan 22, 2024

Uh oh!

changeset-bot bot commented Jan 22, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Jan 22, 2024 •

edited

Loading

Uh oh!

joshblack commented Jan 26, 2024

Uh oh!

TylerJDev commented Jan 26, 2024

Uh oh!

pksjce commented Feb 1, 2024

Uh oh!

TylerJDev commented Feb 2, 2024

Uh oh!

TylerJDev commented Feb 2, 2024

Uh oh!

joshblack commented Feb 6, 2024

Uh oh!

joshblack left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Tentative focus fix in action list item #4166

Tentative focus fix in action list item #4166

Uh oh!

Conversation

pksjce commented Jan 22, 2024

Rollout strategy

Merge checklist

Uh oh!

changeset-bot bot commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

joshblack commented Jan 26, 2024

Uh oh!

TylerJDev commented Jan 26, 2024

Uh oh!

pksjce commented Feb 1, 2024

Uh oh!

TylerJDev commented Feb 2, 2024

Uh oh!

TylerJDev commented Feb 2, 2024

Uh oh!

joshblack commented Feb 6, 2024

Uh oh!

joshblack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

changeset-bot bot commented Jan 22, 2024 •

edited

Loading

github-actions bot commented Jan 22, 2024 •

edited

Loading