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

Hiding the menu does not hide the menu in NVDA #4

Closed
aardrian opened this issue Mar 30, 2017 · 12 comments
Closed

Hiding the menu does not hide the menu in NVDA #4

aardrian opened this issue Mar 30, 2017 · 12 comments

Comments

@aardrian
Copy link

aardrian commented Mar 30, 2017

Overview

I do not know your reporting format, but I will do my best to follow it

I opened this site for the first time. I am using the screen reader NVDA 2017.1 / Firefox 52.02. When I got to the menu, I toggled the un-labeled button to close the menu. However, while it was visually hidden the entire menu, including the collapse sub-sections, were still announced and still tab-stops.

Feature: Hide the menu from screen reader users when they toggle button to hide the menu.
  As an NVDA user…
  I want to reduce audio clutter by dismissing the menu…
  So that I can get to the content of the site.

  Scenario: Attempting to the use the site for the first time
    Given that I am an NVDA user,
    When I select the unlabeled toggle to close the navigation menu,
    Then I should no longer have the menu in my tab order for the page.

Other

There are many issues in that menu alone. I gave up even trying to get further into the site since there are no headings on the initial page and there are no landmarks. I am not filing bugs for all those. This design system needs a proper accessibility audit.

Edited to add specific items per #4 (comment)

  1. The navigation menu is not in a <nav> element (which can be used to hook state information via ARIA as well as act as a landmark navigation point — see item 7).
  2. <button class="side-nav__toggle-btn"> (the button to collapse the menu) has no accessible name; it is announced as "button".
  3. When activated, the state of the menu is not conveyed (aria-expanded might be a fit) regardless of whether it is collapsed or not.
  4. When collapsed, the menu is still in tab order. The style hiding it is transform: translateX(-100%);. display:none; would hide it from AT and keyboard use (though the toggle button would have to move out of that container).
  5. Sub-menus are never removed from focus order, whether the parent navigation item is collapsed or the entire menu is. This is because they are only hidden via an inline max-height: 0px;. I understand that animation effects are desired here, but those can still be achieved accessibly.
  6. There are no headings on the page, so a screen reader user cannot just jump into the page content. For example, you could replace all the <div>s around individual letters of the branding with an <h1> and still retain your visual design, perhaps using ARIA so it gets announced as words instead of letters. As another example, <p class="overview-page__tile--heading">Carbon design kit</p> should be <h2 class="overview-page__tile--heading">Carbon design kit</h2>
  7. There are no landmarks on the page, so a screen reader user cannot just jump into the page content nor other parts. For example, <section class="overview-page"> should probably be <main class="overview-page">. <div class="side-nav__footer"> should probably be <footer class="side-nav__footer">. <div class="side-nav"> as <nav class="side-nav"> (excluding the footer).
@marijohannessen
Copy link
Contributor

Hi, Adrian! Thank you so much for your feedback regarding this, we really appreciate it.

Because of deadlines, we put our main effort into making the Carbon components themselves accessible before focusing on the website. This is our top priority moving forward though as this is something that is very important to us. Please keep in touch, and, again, we really appreciate your feedback!

@aardrian
Copy link
Author

I might be mis-reading this, but this doesn't read like a response to try to resolve a technical bug report. This reads like nothing is going to happen with this report.

I am making the assumption that the web site is built using the Carbon components. I think it stands to reason that IBM would show off its new platform by building the site on it. Please correct me if I am wrong.

Given that getting into the components themselves is quite difficult when using a screen reader, and that therefore it is even more difficult to find issues within the components (and file reports against them let alone test them), and given that the announcement has specifically called out accessibility as a goal, then (in my opinion) it stands to reason that this issue report has identified a genuine bug in the framework that needs to be addressed.

Given that the unlabeled buttons fail WCAG 1.1.1 and WCAG 4.1.2, the non-hidden menu fails (IMO) WCAG 1.3.2, and that the lack of page structure fails WCAG 1.3.1, WCAG 2.4.1, and WCAG 2.4.6, then not only is the web site itself not WCAG 2.0 Level AA compliant, but I have to assume the components are not as well.

Which means anybody who uses Carbon in a project is very likely setting that project up to violate some state, federal, an/or international laws (which typically lean on WCAG 2.0 Level AA).

So what do you need from me to help your team resolve what is very clearly an accessibility barrier (and legal risk)?

@chrisdhanaraj
Copy link
Contributor

Hey @aardrian - just jumping in here. Sorry if it seemed we were being blase about the response - this is absolutely a high priority for us and we'll be tackling the accessibility issues on the website over the next week to get them addressed. The components themselves are pretty rigorously tested against IBM's accessibility standards (basically WCAG 2.0 Level AA), but we hadn't bombarded the website yet with the same.

The website uses the components to drive some, but not all, of the UI. The components don't yet include a complete set of all things an application could potentially use - in the website case, one of the things custom built for it was the website nav (which is the specific element failing the accessibility tests here).

But again, we'll be going through and tackling these issues this week! Thanks for highlighting them, we do seriously appreciate the check.

@aardrian
Copy link
Author

@chrisdhanaraj I appreciate the response.

The components themselves are pretty rigorously tested against IBM's accessibility standards (basically WCAG 2.0 Level AA),

I do not know what you mean by "basically" in this context. Are those standards at least WCAG 2.0 AA, perhaps with more on top? Or are they a sub-set of WCAG 2.0 AA?

but we hadn't bombarded the website yet with the same.

I am sure you understand that if the web site is inaccessible, and that is how I get to see (and review) the components, then I cannot even assess the components. I have to draw the conclusion that the components themselves are probably not accessible (even if the site is not built from the components, I assume some of the team from the components had a hand in the site).

But again, we'll be going through and tackling these issues this week!

Note that while my bug report specifically addresses the navigation, the navigation itself has a few discrete issues (unlabeled controls, items still in tab order when hidden, maybe focus management). In addition, the site does not appear to use a heading structure nor landmarks (at least on the home page).

Are you addressing all of those this week? I am not trying to be pedantic, but I think you may have unintentionally over-committed.

When you say "this week", given that it is Friday, should I assume you mean within a weeks' time?

Do you want separate issues logged for each of the points I have raised?

@chrisdhanaraj
Copy link
Contributor

Let me clarify

1. IBM Accessibility Standards
These are a superset of WCAG 2.0 compliance, we'll be aiming to hit at the minimum WCAG 2 .0 compliance on the website.

2. this week
In the next week (not this week :), my bad), we'll be tackling the accessibility issues on the navigation component of the website.

Do you want separate issues logged for each of the points I have raised?

If you could make a bulleted list in this issue and edit your original post, we can track them that way.

@aardrian
Copy link
Author

Done. If you need more detail, please let me know. I tried to avoid telling people how to code things and instead just highlighted places where things could be modified with different elements. There may be more natural locations for each of those suggestions.

Also, please bear in mind I have not gotten past the home page and there are still plenty of other issues I am not referencing here (like the missing text alternatives on the SVGs or putting role="button" on the links to the repo and design kit) as those are outside the scope of my original report.

@marijohannessen
Copy link
Contributor

@aardrian I just merged in a PR that should address all the items on your list. I won't be able to add any more fixes until Monday, but you should at least be able to get past the side navigation now.

Next week, we are going to do a full accessibility audit on the site, so if you have any helpful resources regarding SPAs and accessibility then I would love to check them out.

@aardrian
Copy link
Author

aardrian commented Apr 1, 2017

Holy cow that is so much better. I can actually jump to the main landmark, or back to the nav, or navigate by heading. Even if you had not fixed the navigation display I could still finally get around.

I think you know there are some tweaks needed, but this is a great first step for me to be able to even get in to evaluate the design patterns.

Thank you for addressing this so quickly.

Quick FYI on the navigation toggle button, it does not need tabindex="0" as it is already in the default tab order just by being a native interactive control. The <a tabindex="0" class="skip-to-content">Skip to main content</a> should not need it either if you use href="#maincontent" . Since you have a tabindex="0" on the <main> element that will allow it to accept focus from the link for all those broken WebKit browsers.

As for SPAs and accessibility, that is a tough request. Each SPA framework does things its own way, so while there are libraries that can help (like ngAria for Angular) that does not guarantee accessibility. I have not used React, but I am aware somebody made a tool to test React applications for accessibility (though it has not been updated in a while). You may also want to look at Tenon, which can be integrated into your development workflow to catch issues as you go instead of them being treated as bug reports later.

As long as those SPAs output valid HTML that makes interactive controls out of real interactive elements then you are already in good shape (ie, you should never put an onclick on a <div> without a heck of a good reason). Good page structure, valid semantic and structural HTML tends to bring accessibility along for free.

@handcoding
Copy link

Since you have a tabindex="0" on the <main> element that will allow it to accept focus from the link for all those broken WebKit browsers.

Just as a side thing—going with <main tabindex="-1"> will work just as well to quell WebKit-related weirdness relating to skip links, and by using tabindex="-1" you won’t end up creating an extra tab stop (unlike tabindex="0", which creates an extra tab stop).

(If you might like me to, I’d be happy to file this as a separate issue. Either way works for me.)

@marijohannessen
Copy link
Contributor

Thank you for the input @handcoding , I was not aware of this. @aardrian I will look into those tools, thanks! I am going to close this issue now, but feel free to open new ones if you see anything else that we should address. I am going to go through the rest of the pages on the website today.

@handcoding
Copy link

Thank you for the input @handcoding , I was not aware of this. @aardrian I will look into those tools, thanks! I am going to close this issue now, but feel free to open new ones if you see anything else that we should address.

@marijohannessen Shall I file a new issue for the tabindex="0" on <main> (since that’s still in there)?

@marijohannessen
Copy link
Contributor

I have fixed this in a PR that's getting reviewed today, I'll update this issue when that has been done :)

slipperypenguin added a commit to slipperypenguin/carbon-agave that referenced this issue Mar 5, 2018
* Updated version after merging with upstream master
mkou-i pushed a commit to mkou-i/carbon-test that referenced this issue Oct 31, 2024
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

4 participants