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

Accessibility issues #765

Open
XhmikosR opened this issue Jan 3, 2016 · 6 comments
Open

Accessibility issues #765

XhmikosR opened this issue Jan 3, 2016 · 6 comments

Comments

@XhmikosR
Copy link

XhmikosR commented Jan 3, 2016

The latest version throws a few accessibility errors. An example is your first one using html_codesniffer using WCAG2AA:

This button element does not have a name available to an accessibility API. Valid names are: title attribute, element content.
Principle: Robust
Techniques: H91
@jackmoore
Copy link
Owner

Of the warnings, the only one I am personally concerned about fixing is the one you quoted. The rest are styling concerns that I'll leave up to the user and their CSS.

The error you quoted has to do with buttons that are hidden, without content, but still exist in the DOM. When the buttons are visible there is no error because they meet the criteria at that point (have content that counts as a name).

So the real fix here would be for me to rework things so that the buttons aren't in the DOM at all when they are not being used. The reason for the hidden buttons in the DOM is so that the browser will go ahead and load the background images, avoiding the flash of unstyled content when Colorbox is first opened. I assume screen readers are intelligent enough not to read out hidden elements, so I doubt fixing the error would help anyone in practice.

@XhmikosR
Copy link
Author

XhmikosR commented Jan 5, 2016

True that but at the moment there are not so tech savvy people (bosses and
customers) who just look at the reports without knowing what happens behind
the scenes.

So, I wonder if you could apply the same fix for all buttons. If you think
it's a bug we could open an issue with html_codesniffer.

Thanks!
On Jan 5, 2016 22:22, "Jack Moore" [email protected] wrote:

Of the warnings, the only one I am personally concerned about fixing is
the one you quoted. The rest are styling concerns that I'll leave up to the
user and their CSS.

The error you quoted has to do with buttons that are hidden, without
content, but still exist in the DOM. When the buttons are visible there is
no error because they meet the criteria at that point (have content that
counts as a name).

So the real fix here would be for me to rework things so that the buttons
aren't in the DOM at all when they are not being used. The reason for the
hidden buttons in the DOM is so that the browser will go ahead and load the
background images, avoiding the flash of unstyled content when Colorbox is
first opened. I assume screen readers are intelligent enough not to read
out hidden elements, so I doubt fixing the error would help anyone in
practice.


Reply to this email directly or view it on GitHub
#765 (comment).

@trussd
Copy link

trussd commented Aug 10, 2016

Does anyone have a patch, hack or instructions for eliminating this accessibility error? I'm in the public sector here, and our site has received a complaint letter. At this point I need to fix these errors, or I have to replace Colorbox altogether. Thanks for any help.

@mxcosu
Copy link

mxcosu commented Nov 29, 2016

@jackmoore
Would you consider a pull request to just add content to those buttons? This is a quick 5 minute fix, and a lot of people in universities and other public sector would be thankful.
As @trussd @XhmikosR have mentioned, some of us work in the public sector and it's better to report no accessibility.

The appropriate smart loading fix would take significant time to study the whole codebase. The other one I already have a quick fix.

thanks

jameswilson added a commit to BluesparkLabs/colorbox that referenced this issue Mar 28, 2017
The aproach here adds button text from colorbox defaults at page
load time to avoid the following errors reported by accessibility
tests:

* wave.webaim.org:

    A button is empty or has no value text.

* HTML_CodeSniffer WCAG 2.0 Level AAA:

    This button element does not have a name available to an
    accessibility API. Valid names are: title attribute, element
    content.
jameswilson added a commit to BluesparkLabs/colorbox that referenced this issue Mar 28, 2017
The approach here adds button text from colorbox defaults at page
load time to avoid the following errors reported by accessibility
tests:

* wave.webaim.org:

      A button is empty or has no value text.

* HTML_CodeSniffer WCAG 2.0 Level AAA:

      This button element does not have a name available to an
      accessibility API. Valid names are: title attribute, element
      content.
jameswilson added a commit to BluesparkLabs/colorbox that referenced this issue Mar 28, 2017
This is a formal approach to fixing jackmoore#765 with proper use of Aria
attributes as an alternative to PR jackmoore#821.

Changes include:

*  Add `aria-hidden` attributes on the dialog box and navigational
   buttons.

*  Update the `aria-hidden` value to `true` or `false` at the
   appropriate times.

*  Add `aria-label` attributes to navigational buttons, using the
   colorbox default settings at page load time, and then update the
   value when a colorbox is loaded.

*  Add `aria-labelledby` and `aria-describedby` attributes to the
   div containing `role='dialog'`.
@jameswilson
Copy link

jameswilson commented Mar 28, 2017

I've just created two PRs that address this issue in different ways:

#821 - Does the bare minimum. Adds button text to pass both the original HTML_CodeSniffer test reported here as well as the similar test on http://wave.webaim.org

#822 - Implements improved Accessibility with proper Aria attributes. This solution actually does not pass HTML_CodeSniffer, but passes the http://wave.webaim.org I guess the proper solution would maybe be a combination of these two, but since they both touch the same lines, there will be a merge conflict on the second one, after the first is merged.

Edit: #822 has been fixed to pass both HTML_CodeSniffer and http://wave.webaim.org , I'll close #821 .

Edit 2: #832 adds upon #822 with some additional accessibility tweaks.

@lkmorlan
Copy link

I assume screen readers are intelligent enough not to read out hidden elements

Unless the item is display: none, it will get read. I just got a complaint from a screen reader user about these buttons being read out.

joewhitsitt added a commit to joewhitsitt/colorbox that referenced this issue Nov 12, 2020
cdubz added a commit to CascadePublicMedia/colorbox that referenced this issue Dec 14, 2021
* Aria Accessibility compliance. fixes jackmoore#765

This is a formal approach to fixing jackmoore#765 with proper use of Aria
attributes as an alternative to PR jackmoore#821.

Changes include:

*  Add `aria-hidden` attributes on the dialog box and navigational
   buttons.

*  Update the `aria-hidden` value to `true` or `false` at the
   appropriate times.

*  Add `aria-label` attributes to navigational buttons, using the
   colorbox default settings at page load time, and then update the
   value when a colorbox is loaded.

*  Add `aria-labelledby` and `aria-describedby` attributes to the
   div containing `role='dialog'`.

* Pass HTML_CodeSniffer WCAG 2.0 tests

* Cleanup unneeded aria-labels, fix autoreading of dialog by using ARIA live regions and ensure focus is on the first element in the dialog when it opens

* Fix spelling of 'aria-label'

* Cleanup unneeded aria-labels, fix autoreading of dialog by using ARIA live regions and ensure focus is on the first element in the dialog when it opens

* Reworked focus trapping

* Updated minified version

Co-authored-by: James Wilson <[email protected]>
Co-authored-by:  <[email protected]>
Co-authored-by: Bram Duvigneau <[email protected]>
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

No branches or pull requests

6 participants