Skip to content

fix: popover should only appear if the trigger event is focus-visible#5150

Merged
chasestarr merged 3 commits intouber:masterfrom
neel1998:popover_focus
Aug 29, 2022
Merged

fix: popover should only appear if the trigger event is focus-visible#5150
chasestarr merged 3 commits intouber:masterfrom
neel1998:popover_focus

Conversation

@neel1998
Copy link
Contributor

Fixes Popover focus issue. Popover should only appear if the last event was focus-visible

Description

Right now onFocus() of popover container doesn't check if the triggering event is focus-visible and hence calls this.open() on every focus event. However this results in unexpected behaviour. One example as shown in the video below is of a button with tooltip, which opens a Modal on click. Here when the modal is closed using backdrop click or close button click the tooltip re-appears. Which should not happen.

Before

popover_before.mov

Ideally the tooltip should not appear on any click event that closes the modal and returns focus to the button. Only when keyboard event closes modal and button gets focus-visible then only the tooltip should appear. The suggested fix provides this expected behaviour.

After

popover_after.mov

Scope

Patch: Bug Fix

@CLAassistant
Copy link

CLAassistant commented Aug 29, 2022

CLA assistant check
All committers have signed the CLA.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 29, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit dfaac43:

Sandbox Source
Basic usage Configuration

Copy link
Collaborator

@chasestarr chasestarr left a comment

Choose a reason for hiding this comment

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

Thank you

@chasestarr chasestarr merged commit 9edb774 into uber:master Aug 29, 2022
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