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

Make pop-ups accessible #1833

Merged
merged 12 commits into from
Mar 18, 2022
Merged

Conversation

jovyntls
Copy link
Contributor

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • Others, please explain:

Fixes #1745

Overview of changes:
Change the default trigger from hover to hover focus

Anything you'd like to highlight / discuss:
N/A

Testing instructions:
Using the tab key to navigate the page should cause pop-ups to be displayed when they are in focus (i.e., when they are tabbed over)

Proposed commit message: (wrap lines at 72 characters)

Improve accessibility of pop-ups

According to the bootstrap-vue docs, the default value of `hover` for
popups is impossible to trigger for keyboard users, resulting in
accessibility issues.

Let's use `hover focus` as the default value, as this provides an
additional and unobtrusive way of displaying pop-up contents.

Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

Copy link
Contributor

@tlylt tlylt left a comment

Choose a reason for hiding this comment

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

Using the tab key to navigate the page should cause pop-ups to be displayed when they are in focus (i.e., when they are tabbed over)

Tested on the preview here and worked 👍

For this part:

Name | Type | Default | Description
---- | ---- | ------- | ------
trigger | String | hover focus | How the overlay view is triggered.
Supports: click, focus, hover.

We say in the description that it supports 'click', 'focus', 'hover', but the value we use is 'hover focus', which is not part of the above. Seems a bit confusing?

@jovyntls
Copy link
Contributor Author

@tlylt That's true, thanks for the catch! I've changed it to:

Supports: click, focus, hover, or any space-separated combination of these.

Copy link
Contributor

@jonahtanjz jonahtanjz left a comment

Choose a reason for hiding this comment

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

Thanks @jovyntls

LGTM 👍

@jonahtanjz jonahtanjz added this to the 4.0 milestone Mar 17, 2022
@kaixin-hc
Copy link
Contributor

It looks good! I'm wondering if these accessibility improvements can also be applied to modals. On this page, I can't seem to activate the modals by focusing on them with the keyboard.

@jovyntls
Copy link
Contributor Author

It looks good! I'm wondering if these accessibility improvements can also be applied to modals. On this page, I can't seem to activate the modals by focusing on them with the keyboard.

It looks like the case for modals might be a bit tricky. When a modal is opened through focus, the focus moves away from the trigger. When the modal is closed, the focus automatically returns to the trigger, which makes makes the modal open again, which results in being unable to ever close the modal once it has been opened using focus, a bit like an infinite loop. (Popovers and tooltips don't have this problem because the focus remains on the popover/tooltip even after it is activated.)

One way to circumvent this could be to manually return the focus by adding a non-visible dummy element after the trigger for modals? I'm not sure if this will have any implications though

@jonahtanjz
Copy link
Contributor

I think we can open this up as an issue and move the discussion there. This can be done in a separate PR since the solution may be complex/take some time.

@jonahtanjz jonahtanjz merged commit bd7fd17 into MarkBind:master Mar 18, 2022
ryoarmanda pushed a commit to ryoarmanda/markbind that referenced this pull request Apr 4, 2022
According to the bootstrap-vue docs, the default value of `hover` for
popups is impossible to trigger for keyboard users, resulting in
accessibility issues.

Let's use `hover focus` as the default value, as this provides an
additional and unobtrusive way of displaying pop-up contents.
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.

Popover components are not accessible
4 participants