Skip to content

fix(popover): forwarded focus to close button with VoiceOver click#6660

Merged
tlabaj merged 2 commits into
patternfly:mainfrom
evwilkin:fix/4210-popover-click
Dec 6, 2021
Merged

fix(popover): forwarded focus to close button with VoiceOver click#6660
tlabaj merged 2 commits into
patternfly:mainfrom
evwilkin:fix/4210-popover-click

Conversation

@evwilkin
Copy link
Copy Markdown
Member

@evwilkin evwilkin commented Nov 30, 2021

What: Closes #4210

This PR:

  • Forwards focus to close button within popover when triggering with VoiceOver click
  • Removes the optional onTriggerEnter function passed as a prop to Popper
    • This function was duplicating the onTriggerClick functionality but targeted only at the Enter key, but that key press was triggering both functions so this seems to be unnecessary.

@patternfly-build
Copy link
Copy Markdown
Collaborator

patternfly-build commented Nov 30, 2021

@evwilkin
Copy link
Copy Markdown
Member Author

evwilkin commented Dec 1, 2021

@jschuler I know you've worked with Popper a lot and I believe wrote the code I removed here, WDYT? Want to make sure there's not an edge case I'm unaware of to keep the onTriggerEnter function.

Copy link
Copy Markdown
Contributor

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

It seems to me that that in this PR and in production I cannot get VO focus into the popover when it opens. VO focus stays on the toggle, even though it reads the contents of the popover. That is fine except in the example of close-popover-from-content-controlled where I really struggled to figure out how to use VO to hit the controlled close button.

@tlabaj tlabaj requested a review from jessiehuff December 2, 2021 16:32
@tlabaj tlabaj added the A11y label Dec 2, 2021
Copy link
Copy Markdown
Collaborator

@jschuler jschuler left a comment

Choose a reason for hiding this comment

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

@evwilkin Not sure why i added this :) seems to all work well without it!

Copy link
Copy Markdown
Contributor

@jessiehuff jessiehuff left a comment

Choose a reason for hiding this comment

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

I agree with Nicole's comment in that there are a few that don't switch focus for me (close popover from content (controlled) and the advanced example). However, I don't think that they currently switched focus through keyboard either looking at org. I'm ok with solving these here or in another issue/PR. I'm open to what you guys think! :)

Copy link
Copy Markdown
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Popover interaction inconsistency

6 participants