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

Axe A11y Issues Report for the Shellbar and related components #3933

Closed
1 of 4 tasks
codefactor opened this issue Sep 15, 2021 · 2 comments
Closed
1 of 4 tasks

Axe A11y Issues Report for the Shellbar and related components #3933

codefactor opened this issue Sep 15, 2021 · 2 comments
Labels

Comments

@codefactor
Copy link
Contributor

codefactor commented Sep 15, 2021

Bug Description

As part of our release cycle we ran some Axe reports and I noticed the following issues - sorry of these have already been reported! This might need to be broken up into multiple tickets.

I'll go through my findings below where I think we might be able to make a change and reduce the number of issues.

NOTE: Some of these are not hard requirements or suggestions, just my guess at trying to decipher the meaning behind these errors or what could be done to fix them.

1. Elements must only use allowed ARIA attributes

Maybe <ui5-buttons> should not have an aria-label, maybe title is enough?

This one points to several <ui5-button> tags in the <ui5-shellbar>

<ui5-button icon="sap-icon://bell" data-ui5-text="Notifications" data-ui5-stable="notifications" id="ui5wc_47-item-2" style="order:3;" class=" ui5-shellbar-button ui5-shellbar-bell-button" data-ui5-notifications-count="99+" aria-label="99+ Notifications" title="99+ Notifications" ui5-button="" icon-only="" has-icon="">

It also says...

Fix the following:
ARIA attribute is not well supported on the element and the text content will be used instead: aria-label

It seems like Axe doesn't like this web-component to have an "aria-label" attribute... I guess?
There is also a title attribute, maybe the aria-label should be removed here?

2. Headings should not be empty

secondary-title should never output empty <h2>?

This one only happens when you have "secondary-title" as not defined. It points to the empty <h2> tag for the secondary title, and successfactors never populates a secondary title for our pages.

3. Elements of role none or presentation should be flagged

custom shellbar icon buttons should have role="img" on the <svg> ?

This one comes up for shellbar icon buttons <svg>, but not all of them just the custom ones. The search button and the account menu seem fine. This one complains that the role is "presentation" but it's not focusable (i.e. because tabindex=-1) but we know it's not supposed to be focusable because it's just the <svg> and it's a child of a <button> that is focusable so probably the role should not be "presentation" it should be "img" that seems to work for some other buttons in the same level.

4. Elements should not have tabindex greater than zero

The ui5 block layer maybe should have tabindex=0?

This one comes up for popups that have a block layer underneath - for some reason the tabindex=1 for this layer, maybe it should be 0?

5. Document should not have more than one banner landmark

maybe the <ui5-list> should put a role onto the <header> so it doesn't think that's a "banner" landmark?

This one comes up for any <ui5-list> element when there is also a <ui5-shellbar> because the ui5-list adds a <header> element and so it complains that there are 2 banners - one is the shellbar with <div role="banner"> and one for the <header> for the <ui5-list>. You'd need to have a page with both to see this.

6. ARIA dialog and alertdialog nodes should have an accessible name

output with custom Popup element has aria-labelledby="ui5-popup-header" but no such element exists.

Try this yourself on the shellbar playground and click the account menu for "An Kimura" -- inside the <ui5-popover> shadowRoot you'll find the following: <section role="dialog" style="" class="ui5-popup-root" aria-modal="true" aria-labelledby="ui5-popup-header" tabindex="-1">

This is part of the Popover.hbs but the definition of "ui5-popup-header" is part of the Dialog.hbs which means if this is a Popup but not a Dialog the aria-labeledby attribute will be dangling.

7. ARIA input fields must have an accessible name

maybe there needs to be an aria-label or aria-labeledby for the <ul> of a <ui5-list>?

This is related to the <ul> output from the <ui5-list> tag, it is complaining that it doesn't have any sort of label but since it has a role="listbox" it needs a label or to be labeled by something.

8. Ensure interactive controls are not nested

It is not clear, but all the buttons inside the shellbar are coming out with this error. They are clearly interactive controls, but I don't know what it's complaining about that this is nested inside which is also interactive. Can you please check is this a known issue?

Expected Behavior

Axe should not complain about these things that we can fix.

Steps to Reproduce

Chrome "axe Dev Tools" extension - go open the extension on a page and run the report.
Most of these issues can be seen by going to the playground and running axe Dev Tools by clicking the "Scan ALL of my page" button.

Screenshot 1: shows the tool itself when you open it at first, and the button you need to press
image

Screenshot 2: shows the results of the tool
image

Above are screenshots of the tool and the results - there are 325 issues. Ideally there would be 0 issues. I've ran this tool from SuccessFactors and we only see around 16 issues.

Isolated Example

N/A - most can be seen by using the shellbar example in the playground

Context

  • UI5 Web Components version: {...}
  • OS/Platform: {...}
  • Browser: {...}
  • Affected component: {...}

Log Output / Stack Trace / Screenshots

{...}

Priority

  • Low
  • Medium
  • High
  • Very High

The priority indicates the severity of the issue. To set the appropriate priority consider the following criteria:

  • Breaks entire application or system - High or Very High
  • Accessibility issue - Medium or High
  • Functional issue - Medium or High
  • Visual issue - Low or Medium

Note: The priority might be re-evaluated by the issue processor.

Stakeholder Info (if applicable)

  • Organization: {...}
  • Bussiness impact: {...}
@ilhan007
Copy link
Member

Hello @codefactor thanks for reporting and the analysis

One of the issues is already handled (not released)
2. Headings should not be empty - Fixed with #3737

One of the issues I found it's already reported
8. Ensure interactive controls are not nested - already reported with #3927

For the rest it's best if you can create separate issues, because the issues are in components owned by different teams.
At least, you can group the issues by component - Button, List, ShellBar, Popover/Dialog

Could you split them into Button, List, ShellBar, Popover/Dialog issues?.
The current one you can use for one of the groups.

Sorry for the inconvenience,
Best, ilhan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants