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

WB-1654: Popover trigger element is stealing focus from other focusable elements [FIX] #2162

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

jandrade
Copy link
Member

@jandrade jandrade commented Jan 5, 2024

Summary:

As soon as a popover is closed, we return focus to its trigger element (or
element that caused the popover to open). This works fine in most cases, but
there are situations where we don’t want to do that.

For example, when there are multiple Popovers within the same context and the
user opens a separate popover, the previous opened popover closes, but in this
case the focus is being moved from the new popover to the trigger element of the
previous popover.

This PR fixes this by detecting if the element that caused the popover to be
dismissed is an element that can be focused, and if so, we let the focus flow
naturally to that element. If not, we return focus to the trigger element.

Issue: https://khanacademy.atlassian.net/browse/WB-1654

Test plan:

  • Open the popover docs page.
  • Scroll down to the first story.
  • Click on the popover trigger button and verify that the popover opens.
  • Now click on the popover trigger button of the second story and wait until
    the first popover closes and the second popover opens.
  • Verify that the focus is not being moved to the trigger element of the
    first popover.
BEFORE AFTER
https://github.com/Khan/wonder-blocks/assets/843075/3024ed9c-491e-4963-9b1c-53ae3034870e https://github.com/Khan/wonder-blocks/assets/843075/a0a2fc3a-9656-4505-a20e-6ee2e795ec83

@jandrade jandrade self-assigned this Jan 5, 2024
Copy link

changeset-bot bot commented Jan 5, 2024

🦋 Changeset detected

Latest commit: 77b8fc2

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

This PR includes changesets to release 1 package
Name Type
@khanacademy/wonder-blocks-popover 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

@khan-actions-bot
Copy link
Contributor

Gerald

Required Reviewers
  • @Khan/wonder-blocks for changes to .changeset/clever-trees-confess.md, packages/wonder-blocks-popover/src/components/focus-manager.tsx, packages/wonder-blocks-popover/src/components/popover-event-listener.ts, packages/wonder-blocks-popover/src/components/popover.tsx, packages/wonder-blocks-popover/src/util/util.ts, packages/wonder-blocks-popover/src/components/__tests__/popover.test.tsx, packages/wonder-blocks-popover/src/util/__tests__/util.test.tsx

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

@khan-actions-bot khan-actions-bot requested a review from a team January 5, 2024 22:36
Copy link
Contributor

github-actions bot commented Jan 5, 2024

Size Change: +67 B (0%)

Total Size: 92.6 kB

Filename Size Change
packages/wonder-blocks-popover/dist/es/index.js 4.82 kB +67 B (+1%)
ℹ️ View Unchanged
Filename Size
packages/wonder-blocks-accordion/dist/es/index.js 3.76 kB
packages/wonder-blocks-banner/dist/es/index.js 1.53 kB
packages/wonder-blocks-birthday-picker/dist/es/index.js 1.72 kB
packages/wonder-blocks-breadcrumbs/dist/es/index.js 1.13 kB
packages/wonder-blocks-button/dist/es/index.js 4.27 kB
packages/wonder-blocks-cell/dist/es/index.js 2.24 kB
packages/wonder-blocks-clickable/dist/es/index.js 3.24 kB
packages/wonder-blocks-color/dist/es/index.js 1.15 kB
packages/wonder-blocks-core/dist/es/index.js 3.7 kB
packages/wonder-blocks-data/dist/es/index.js 6.33 kB
packages/wonder-blocks-dropdown/dist/es/index.js 12.3 kB
packages/wonder-blocks-form/dist/es/index.js 5.34 kB
packages/wonder-blocks-grid/dist/es/index.js 1.36 kB
packages/wonder-blocks-i18n/dist/es/index.js 4.54 kB
packages/wonder-blocks-icon-button/dist/es/index.js 3.21 kB
packages/wonder-blocks-icon/dist/es/index.js 1.06 kB
packages/wonder-blocks-labeled-field/dist/es/index.js 72 B
packages/wonder-blocks-layout/dist/es/index.js 1.88 kB
packages/wonder-blocks-link/dist/es/index.js 2.54 kB
packages/wonder-blocks-modal/dist/es/index.js 5.53 kB
packages/wonder-blocks-pill/dist/es/index.js 1.19 kB
packages/wonder-blocks-progress-spinner/dist/es/index.js 1.51 kB
packages/wonder-blocks-search-field/dist/es/index.js 1.55 kB
packages/wonder-blocks-spacing/dist/es/index.js 158 B
packages/wonder-blocks-switch/dist/es/index.js 2.06 kB
packages/wonder-blocks-testing/dist/es/index.js 3.94 kB
packages/wonder-blocks-theming/dist/es/index.js 1.23 kB
packages/wonder-blocks-timing/dist/es/index.js 1.78 kB
packages/wonder-blocks-toolbar/dist/es/index.js 862 B
packages/wonder-blocks-tooltip/dist/es/index.js 5.05 kB
packages/wonder-blocks-typography/dist/es/index.js 1.49 kB

compressed-size-action

Copy link
Contributor

github-actions bot commented Jan 5, 2024

npm Snapshot: Published

🎉 Good news!! We've packaged up the latest commit from this PR (fa4bf14) and published all packages with changesets to npm.

You can install the packages in webapp by running:

./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2162"

Packages can also be installed manually by running:

yarn add @khanacademy/wonder-blocks-<package-name>@PR2162

Copy link

codecov bot commented Jan 5, 2024

Codecov Report

Merging #2162 (77b8fc2) into main (163cfca) will increase coverage by 1.37%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2162      +/-   ##
==========================================
+ Coverage   94.90%   96.27%   +1.37%     
==========================================
  Files         245      245              
  Lines       28442    28459      +17     
  Branches     2344     2396      +52     
==========================================
+ Hits        26992    27399     +407     
+ Misses       1450     1060     -390     
Files Coverage Δ
...er-blocks-popover/src/components/focus-manager.tsx 96.80% <ø> (-0.07%) ⬇️
...s-popover/src/components/popover-event-listener.ts 100.00% <100.00%> (ø)
...s/wonder-blocks-popover/src/components/popover.tsx 100.00% <100.00%> (ø)
packages/wonder-blocks-popover/src/util/util.ts 100.00% <100.00%> (ø)

... and 41 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 163cfca...77b8fc2. Read the comment docs.

Copy link
Contributor

github-actions bot commented Jan 5, 2024

A new build was pushed to Chromatic! 🚀

https://5e1bf4b385e3fb0020b7073c-vyrihhlonw.chromatic.com/

Chromatic results:

Metric Total
Captured snapshots 7
Tests with visual changes 0
Total stories 403
Inherited (not captured) snapshots [TurboSnap] 312
Tests on the build 319

Copy link
Member

@jeresig jeresig left a comment

Choose a reason for hiding this comment

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

oof, this is tricky! thanks for the fix.

@jandrade jandrade merged commit e5dd621 into main Jan 8, 2024
25 of 27 checks passed
@jandrade jandrade deleted the popover-return-focus branch January 8, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants