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

Popover: Visually indicate focus effect on popover content #3057

Merged
merged 1 commit into from
Oct 27, 2017

Conversation

aduth
Copy link
Member

@aduth aduth commented Oct 18, 2017

This pull request seeks to add styling to indicate the popover content as focus target, used as fallback when there is no tabbable items within the opened popover.

Focus

In master, when tabbing to the Publish button and pressing Space, the popover opens but it is not clear where focus is. In fact, the focus is on the popover content, but this would not be known until trying to tab.

Open questions:

  • Why is the publish button "focus on open" behavior not causing the Visibility button toggle to be focused?
  • Should popover "focus on open" only take effect when triggered by keyboard events? With these changes, the focus halo styles are shown when clicking the publish button.
  • Should popover have a non-negative tab index? (i.e. reachable by tabbing) Previously it did, though since the popover is rendered elsewhere in the element tree, the tab position was not consecutive with adjacent siblings elements. With "focus on open" behavior, combined with focus return on closing, it may be the expectation that keyboard focus would be managed as the relationship between the popover and the opening trigger element (should probably be reflected with ownership ARIA attributes).
    • This is related to the tabIndex override from Tooltip, where the assumption is that the tooltip should not be tabbable, but effectively it was not anyways because the popover is rendered out of sequence to where the tooltip is shown

@aduth aduth added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Oct 18, 2017
@codecov
Copy link

codecov bot commented Oct 18, 2017

Codecov Report

Merging #3057 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3057      +/-   ##
==========================================
- Coverage   32.59%   32.57%   -0.02%     
==========================================
  Files         209      209              
  Lines        5965     5964       -1     
  Branches     1055     1054       -1     
==========================================
- Hits         1944     1943       -1     
  Misses       3391     3391              
  Partials      630      630
Impacted Files Coverage Δ
components/tooltip/index.js 83.87% <ø> (ø) ⬆️
components/popover/index.js 83.33% <100%> (-0.18%) ⬇️

Continue to review full report at Codecov.

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

@jasmussen
Copy link
Contributor

Nice!

Is there a way to do this that is general to more areas than just the popover? I had this visual idea a while back, to indicate which area receives focus: https://codepen.io/joen/pen/Nvjror

If yes, then would that make sense to also look at in the context of #467?

@aduth
Copy link
Member Author

aduth commented Oct 18, 2017

Why is the publish button "focus on open" behavior not causing the Visibility button toggle to be focused?

Noted that this was an issue with capabilities checks in the PublishDropdown component rendering static elements on the initial mount because data is not available at the first render, resolved separately in #3058.

@aduth
Copy link
Member Author

aduth commented Oct 19, 2017

Is there a way to do this that is general to more areas than just the popover?

Most browsers would have the default focus ring style applied to any :focus element, but it appears in core styles this is explicitly disabled:

https://github.com/WordPress/WordPress/blob/ea21ad6cc17b7aae5fc47f7f5cbc7edde8ad53ec/wp-admin/css/common.css#L255-L258

It's unclear to me the intention here.

@aduth aduth merged commit ffa33d3 into master Oct 27, 2017
@aduth aduth deleted the update/popover-focus-style branch October 27, 2017 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants