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

BS4/Bootstrap-Vue Accessibility recommendations #333

Closed
tmorehouse opened this issue May 3, 2017 · 35 comments
Closed

BS4/Bootstrap-Vue Accessibility recommendations #333

tmorehouse opened this issue May 3, 2017 · 35 comments

Comments

@tmorehouse
Copy link
Member

@tmorehouse tmorehouse commented May 3, 2017

Bootstrap V4 is addressing some of the accessibility issues: twbs/bootstrap#22549, Of which bootstrap-vue has started to already address.

Here is little list of items that are complete or need work for aria and/or keyboard navigation (to be updated as we progress through them):

Components - ARIA / Keyboard Navigation

  • b-form-radio input wrapped in label (which negates need for for on label)
  • b-form-checkbox input wrapped in label (which negates need for for on label)
  • b-progress ARIA progress attributes
  • b-modal attributes aria-labelledby and aria-describedby. Section roles & semantic elements. (PR #247)
  • b-modal Focus first input/button on open (PR #247). Allow page author to specify input to be focused when opened via @shown event (PR #378)
  • b-modal focus the modal content when opened
  • b-tabs & b-tab attributes aria-controls and aria-labelelledby. Section roles (PR #339)
  • b-tabs & b-tab keyboard navigation (PR #339)
  • b-alert has aria role="alert", aria-live and aria-atomic (PR #340)
  • b-form-file Needs focus styling in custom-file-input mode for keyboard users (PR #1033)
  • b-form-fieldset attribute for when ID supplied on ,form-control. Has role="alert", aria-live, aria-atomic on feedback. and aria-describedby (PR #340)
  • b-breadcrumb should have role of navigation (PR #340) Add aria-current to active crumb (PR #526)
  • b-input-group needs role="group" (PR #340)
  • b-button-group role="group", make new toolbar component (PR #367)
  • b-button-toolbar role="toolbar" + optional keyboard navigation (PR #367)
  • b-nav & b-nav-link aria attributes (PR #358)
  • b-dropdown keyboard navigation (PR #274)
  • b-nav-dropdown aria- attributes (PR #358)
  • b-nav-dropdown open on ENTER or SPACE (see Issue #348) (PR #349)
  • b-nav-toggle aria- attributes (PR #358, #410, #411, #412)
  • b-nav & b-nav-link keyboard navigation (leave as standard TAB key navigation)
  • b-collapse Reflect expanded/collapsed state on trigger element (PR #358 & #519)
  • b-pagination aria-[label|current|setsize|posinset] attributes. (Note: aria-controls should be set manually in the <b-pagination>markup) (PR #364)
  • b-pagination keyboard navigation (PR #364 & #377)
  • b-carousel aria-[controls|label|current|setsize|posinset] attributes and roles. (PR #380)
  • b-carousel keyboard left/right control (PR #380, #420)
  • b-carousel Pause slide scrolling on mouseenter and resume on mouseout.
  • b-tooltip aria- attributes (aria-live, aria-describedby, etc
  • b-popover aria- attributes (aria-live, aria-describedby, etc. Also needs ability to change role from tooltip to popover or dialog
    • Popovers can have some issues with regards to accessibility, especially for interactive popover content, and possibly should have a role other than tooltip (configurable). twbs/bootstrap#18618

Directives

  • v-b-modal return focus to open trigger element.
  • v-b-toggle attributes aria-controls and aria-expanded. (PR #519)

Documentation

  • Create ARIA best practices for Bootstrap-Vue - sections added to each compoent
  • Add documentation where needed
@tmorehouse tmorehouse self-assigned this May 3, 2017
@tmorehouse

This comment has been minimized.

Copy link
Member Author

@tmorehouse tmorehouse commented May 3, 2017

Also found a discussion on autofocus and modal open:

twbs/bootstrap#22402

Which brings up a good point about autofocusing any input/textarea when on a mobile device with virtual keyboard. I might revisit the autofocus selectors in modal.vue to only look for buttons, select or the [autofocus]:first attribute, and if none found, then just focus the .modal-content

@LaurenceRLewis

This comment has been minimized.

Copy link

@LaurenceRLewis LaurenceRLewis commented May 6, 2017

Nice work. If I find any isdues in regards to accessibility I will post my findings. 😁

@tmorehouse

This comment has been minimized.

Copy link
Member Author

@tmorehouse tmorehouse commented May 8, 2017

The list (#333 (comment)) is getting shorter :)

@tmorehouse

This comment has been minimized.

Copy link
Member Author

@tmorehouse tmorehouse commented May 9, 2017

With respect to ID requirements, I am now leaning more towards the page author providing a unique Id on each component, and the component taking advantage of the provided ID to generate sub IDs as needed., and moving away from auto generated IDs (removing requirement for mixins/generate-id), with the possibility of logging warning messages (in devMode only) that certain components are missing IDs

@pi0

This comment has been minimized.

Copy link
Member

@pi0 pi0 commented May 17, 2017

Nice work so far :) Did you notice ARIA problems with NavBar ? (Tested using this chrome extension)

image

@tmorehouse

This comment has been minimized.

Copy link
Member Author

@tmorehouse tmorehouse commented May 17, 2017

@pio I'll take a look and make sure the Attributes are correct with Navbar. Was this using the doc fiddle for NavBar?

@pi0

This comment has been minimized.

Copy link
Member

@pi0 pi0 commented May 17, 2017

Troy, Screenshot above was from docs main page. This may be just because of incorrect usage in docs :))

@tmorehouse

This comment has been minimized.

Copy link
Member Author

@tmorehouse tmorehouse commented May 17, 2017

OK, I'll check both.

@tmorehouse

This comment has been minimized.

Copy link
Member Author

@tmorehouse tmorehouse commented May 17, 2017

Is docs using latest release or master?

@pi0

This comment has been minimized.

Copy link
Member

@pi0 pi0 commented May 17, 2017

Latest. I'm going to release 0.16.0 in five minutes :))

@tmorehouse

This comment has been minimized.

Copy link
Member Author

@tmorehouse tmorehouse commented May 17, 2017

OK I'll wait until next release to check.

@pi0

This comment has been minimized.

Copy link
Member

@pi0 pi0 commented May 17, 2017

Released. Still having same problems. (Here is nav.vue implementation)

@tmorehouse

This comment has been minimized.

Copy link
Member Author

@tmorehouse tmorehouse commented May 17, 2017

Ah, I see that b-collapse is not rendering the ID on it's root div. I'll create a quick PR for this.

@tmorehouse

This comment has been minimized.

Copy link
Member Author

@tmorehouse tmorehouse commented May 17, 2017

@pi0 PR #410

@tmorehouse

This comment has been minimized.

Copy link
Member Author

@tmorehouse tmorehouse commented May 17, 2017

PR #411 fixes where the aria-expanded attribute appears for nav-collapse

@tmorehouse

This comment has been minimized.

Copy link
Member Author

@tmorehouse tmorehouse commented May 18, 2017

carousel may have a small bug when clicking the indicator buttons during a transition.

Update: fixed in PR #420

@tmorehouse

This comment has been minimized.

Copy link
Member Author

@tmorehouse tmorehouse commented May 23, 2017

PR #449 removed the generate-id mixin requirements for form input components. Users, if they want ARIA compliance, will need to set an ID prop on the components.

@tmorehouse

This comment has been minimized.

Copy link
Member Author

@tmorehouse tmorehouse commented Jun 17, 2017

PR #519 adds ARIA attributes to v-b-toggle directive

@tmorehouse tmorehouse modified the milestones: v0.17.0, v1.0.0 Jun 17, 2017
@LaurenceRLewis

This comment has been minimized.

Copy link

@LaurenceRLewis LaurenceRLewis commented Jun 18, 2017

I disagree that b-breadcrumb should have role of navigation. I think the supported aria-current=location is sufficient. A role navigation or HTML5 native nav is overkill for breadcrumbs. Although it could be argued breadcrumbs are a navigation, I believe the breadcrumbs purpose is to show location in the site architecture relative to the main landing page (Home). :-).

@tmorehouse

This comment has been minimized.

Copy link
Member Author

@tmorehouse tmorehouse commented Jun 19, 2017

@dleesalestreamsoft interesting point. Maybe allowing the user to change the role as a prop would suffice, depending on use case, And a prop that can be used to select semantic element is used as a wrapper.

I'll look into aria-current attribute on b-breadcrumb

@LaurenceRLewis

This comment has been minimized.

Copy link

@LaurenceRLewis LaurenceRLewis commented Jun 19, 2017

@tmorehouse

This comment has been minimized.

Copy link
Member Author

@tmorehouse tmorehouse commented Jun 19, 2017

@Decrepidos what about using aria-current="page" instead of aria-current="location"?

Or maybe just aria-current="true"? As breadcrumbs might not always be used to specify a page or location? And the current value in breadcrumbs is not a link (in bootstrap)

https://tink.uk/using-the-aria-current-attribute/

@LaurenceRLewis

This comment has been minimized.

Copy link

@LaurenceRLewis LaurenceRLewis commented Jun 19, 2017

@LaurenceRLewis

This comment has been minimized.

Copy link

@LaurenceRLewis LaurenceRLewis commented Jun 19, 2017

I notice you posted the link. Sorry can't seem to edit post on the phone 🤔

@tmorehouse

This comment has been minimized.

Copy link
Member Author

@tmorehouse tmorehouse commented Jun 19, 2017

@Decrepidos I've defaulted it to location but have created a prop where the user can change it to another value such as page or true in PR #526

@LaurenceRLewis

This comment has been minimized.

Copy link

@LaurenceRLewis LaurenceRLewis commented Jun 19, 2017

@tmorehouse to clarify my personal view is page for navigation. Location for breadcrumbs and true for elements like carousel slide buttons. 😁

@LaurenceRLewis

This comment has been minimized.

Copy link

@LaurenceRLewis LaurenceRLewis commented Jun 19, 2017

Perfect solution 😁

@tmorehouse tmorehouse changed the title BS4 Accessibility recommendations BS4/Bootstrap-Vue Accessibility recommendations Sep 1, 2017
@tvld

This comment has been minimized.

Copy link

@tvld tvld commented Dec 10, 2017

Google Lighthouse for PWA, also giving:

[aria-*] attributes are not valid or misspelled.
Assistive technologies, like screen readers, can't interpret ARIA attributes with invalid names. Learn more.
View failing elements
<div role="listitem" 
    class="carousel-item active carousel-item-left"
    aria-current="true"
    aria-posinset="2" 
    aria-setsize="4" 
    tabindex="-1" 
    aria-controlledby="carousel1___BV_indicator_2_"
    aria-hidden="false">
@tmorehouse

This comment has been minimized.

Copy link
Member Author

@tmorehouse tmorehouse commented Dec 10, 2017

Some validators will complain about aria-current. Which attribute is it complaining about?

@tvld

This comment has been minimized.

Copy link

@tvld tvld commented Dec 11, 2017

Unfortunately, it does not say which one, but aria-controlledby is the only one not found on https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques

@LaurenceRLewis

This comment has been minimized.

Copy link

@LaurenceRLewis LaurenceRLewis commented Dec 11, 2017

Thats because it does not exist. I suggest it should be aria-controls 😁

@tmorehouse

This comment has been minimized.

Copy link
Member Author

@tmorehouse tmorehouse commented Dec 11, 2017

Hmm, I'll look into that one.

@tmorehouse

This comment has been minimized.

Copy link
Member Author

@tmorehouse tmorehouse commented Dec 11, 2017

Should be fixed in next release via PR #1448

@tmorehouse

This comment has been minimized.

Copy link
Member Author

@tmorehouse tmorehouse commented Dec 17, 2017

@tvld & @Decrepidos this has been fixed in release v1.4.0

@LaurenceRLewis

This comment has been minimized.

Copy link

@LaurenceRLewis LaurenceRLewis commented Dec 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Roadmap
In Progress
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.