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: wrong tab order for popover contents #23120

Closed
vindl opened this issue Mar 8, 2018 · 7 comments
Closed

Popover: wrong tab order for popover contents #23120

vindl opened this issue Mar 8, 2018 · 7 comments
Labels
Accessibility (a11y) [Closed] Stale Components [Type] Bug When a feature is broken and / or not performing as intended

Comments

@vindl
Copy link
Member

vindl commented Mar 8, 2018

See p7jreA-1wH-p2 for related discussion.

Steps to reproduce

  1. Open up any page that uses Popover component (see instructions here for example Checkout: Cart icon has no focus state, can't select checkout via keyboard #21659)
  2. Use keyboard only to navigate to Popover and open it.
  3. Press Tab key to focus Popover content.

What I expected

Items inside Popover to be focused first.

What happened instead

Items in the background were focused, and Popover content comes in last in tab ordering.

Please refer to #21659 for one special case of this issue.

@vindl vindl added [Type] Bug When a feature is broken and / or not performing as intended Accessibility (a11y) Components labels Mar 8, 2018
@ryelle
Copy link
Member

ryelle commented Mar 8, 2018

I decided to dive into this for a hack week project, and wow it's complicated. I tried two approaches, neither worked well so I'm not submitting a PR 🙁 You can see the first attempt using react-modal, and the second attempt, injecting the popover into the correct place in the source order. The problem with react-modal was more to do with the various ways we use popovers (+ a scrolling bug), while the trouble with the second approach was getting the position-detecting logic to work consistently.

So maybe we should try something different. We have a few different kinds of pop-up content, which all use Popover as a base.

  1. The tooltip/InfoPopover use: this is purely information - like the plans page ℹ️ icon, which describes each plan feature
  2. The Popover with actions: this needs some kind of user interaction - like the card checkout popover, which has the "Checkout" button, I would also put the author switcher, site popover, etc in this bucket
  3. The PopoverMenu/EllipsisMenu/SplitButton/etc: A list of actions or links - like the actions on a page or post
  4. The ephemeral tooltip, which only displays on hover - this is the Tooltip component, which uses Popover but styles it differently, and generally uses mouseenter/mouseleave events to display.

I think trying to use the same base component for all 4 of these cases is not the right approach. Each has slightly different interaction patterns, which is more obvious if you try to map it to ARIA interaction patterns. react-modal sets the popover container to the dialog role, and along with the focus trap + an aria-hidden attribute, makes it so that keyboard & screen reader users can't interact with the page while the popover is open. But dialogs are meant to have a focusable element inside them, some action to take. Which makes sense in case 2, but not 1. For case 1, we want something more like the "Toggletips" pattern described here.

Additionally, the PopoverMenu is using the menu/menuitem roles, which is fine for action buttons, but does get confusing if added to a link (more info). This component in particular I think should be split from the Popover, and instead of injecting an absolutely positioned item into RootChild should be put inline with the rest of the HTML. This way we can keep the tab order correct without having to worry about managing focus locations.

Finally are Tooltips – generally these are not accessible at all via keyboard, since they're only available on hover. I think the approach in "tooltips as auxiliary info" would work, but would require only text content in the tooltips (which we don't enforce, but is probably the case? the most complex tooltips seem to be on stats). This would also separate out Tooltip from the base Popover.

Given all the above, I think the approach to making our popovers accessible should be to move away from one parent Popover component as much as possible:

  • Update Tooltip to not use Popover, make the parent element focusable, show it on focus as well as hover, and add the aria-describedby code to announce it for screen readers
  • Update PopoverMenu to not use Popover, instead inlining the menu to preserve tab order
  • Update InfoPopover to not use Popover, instead to follow the "toggletips" example linked above
  • Finally we can revisit Popover, which should only be used for "complex" pop-up content, like the checkout screen, or comment user info, and could be changed to a dialog using react-modal.

That said, this is definitely a larger project than 1 person's week - I'd be happy to team up with a few interested people to talk it out more & maybe try the approach I outlined above.

@aduth
Copy link
Contributor

aduth commented May 29, 2018

Accessible popovers have been the source of much distress for me in the past, so I'd love to help in any way I can to avoid similar headaches in your pending adventure 😄

I'd point to WordPress/gutenberg#5595 as an example of my own previous attempts and failings in Gutenberg, which was eventually closed after some embattled back-and-forths. Specifically, I might hope that my concluding overview at WordPress/gutenberg#5595 (comment) could serve as a useful reference, as I'm certain many of the components which exist in Gutenberg have similar parallels in Calypso as well. I actually came to a different conclusion: Noting that a base Popover component can be a good thing, but not in the sense of advanced usage we've come to expect from it (i.e. stripping it of most of its current behaviors).

This component in particular I think should be split from the Popover, and instead of injecting an absolutely positioned item into RootChild should be put inline with the rest of the HTML. This way we can keep the tab order correct without having to worry about managing focus locations.

On the accessibility front, this certainly does make things simpler. But see my comment at WordPress/gutenberg#5595 (comment) for some cautions to be aware of:

And it's not a matter of the other bugs being the more easily addressed issues than recreating accessible behaviors via slotted rendering. There's simply no known solution to issues affecting stacking context with relation to popovers being shown above other elements on the page (and any such solution may require entire refactors of the administrative markup and CSS, potentially breaking plugins in the process) and unavoidable future bugs and maintenance headaches caused by inadvertent cascade of CSS. Even beyond this, it still wouldn't just work with keyboard behaviors because, for performance reasons, the popover content isn't within the DOM until it's expected to be shown, to avoid rendering and reconciling the potential thousands of nodes that could be made visible at any given point in time.

@stale

This comment has been minimized.

@sirreal

This comment has been minimized.

@diegohaz
Copy link
Contributor

I was taking notes on accessibility issues I was finding while working on other stuff, and I just encountered this issue with Popover.

I think we can say that a keyboard user can't access the content of the popover as when they reach it at the end of the document it's completely out of context.

I'd like to suggest taking a look at this Popover implementation, which handles modal and non-modal states (modal prop), blocking body scroll (preventBodyScroll prop) and conditionally rendered content.

@github-actions
Copy link

This issue is stale because it has been 180 days with no activity. You can keep the issue open by adding a comment. If you do, please provide additional context and explain why you’d like it to remain open. You can also close the issue yourself — if you do, please add a brief explanation and apply one of relevant issue close labels.

@cuemarie
Copy link

👋 Hey folks! Since this issue has been inactive for quite some time, KitKat has made the decision to close it.

  • Internal reference, for more on this decision: pdqkMK-14B-p2

If you think this issue warrants another look, here are some next steps!

  1. Report anew: A new report with more current details and steps to replicate may be the best way to renew attention on this issue. Feel free to refer back to this closed issue in your report!
  2. Reopen: If you feel the issue still matches the context/history here, you can also reopen the issue and add fresh logs, screenshots and steps to reproduce.

Thanks for your involvement!

📌 ADDITIONAL NOTES
Current efforts to audit a11y issues will include filing new issues for popover problems as they're found, so if this issue persists, it'll be re-reported with updated details in the future!

@cuemarie cuemarie closed this as not planned Won't fix, can't repro, duplicate, stale Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility (a11y) [Closed] Stale Components [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

No branches or pull requests

7 participants