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

SF_ACC Successfactors: ui5-responsive-popover has A11y issue for mobile web #2787

Closed
mydavidzhang opened this issue Feb 5, 2021 · 4 comments · Fixed by #2823
Closed

SF_ACC Successfactors: ui5-responsive-popover has A11y issue for mobile web #2787

mydavidzhang opened this issue Feb 5, 2021 · 4 comments · Fixed by #2823
Assignees
Labels

Comments

@mydavidzhang
Copy link

Describe the bug
When ui5-responsive-popover is mobile web mode it has A11y issue in unit test case

expect(received).toHaveNoViolations(expected)

    Expected the HTML found at $('ui5-responsive-popover,ui5-button[icon="decline"],button') to have no violations:

    <button type="button" class="ui5-button-root" data-sap-focus-ref="" part="button" tabindex="0"><!----><ui5-icon class="ui5-button-icon" style="" name="decline" show-tooltip="" ui5-icon=""></ui5-icon><span class="ui5-button-text" id="ui5wc_5-content"><bdi><slot></slot></bdi></span><!----></button>

    Received:

    "Buttons must have discernible text (button-name)"

    Fix any of the following:
      Element does not have inner text that is visible to screen readers
      aria-label attribute does not exist or is empty
      aria-labelledby attribute does not exist, references elements that do not exist or references elements that are empty
      Element's default semantics were not overridden with role="presentation"
      Element's default semantics were not overridden with role="none"
      Element has no title attribute or the title attribute is empty

    You can find more information on this issue here: 
    https://dequeuniversity.com/rules/axe/3.5/button-name?application=axeAPI

    ────────

    Expected the HTML found at $('ui5-responsive-popover,ui5-dialog,h2') to have no violations:

    <h2 class="ui5-popup-header-text"><!----><!----></h2>

    Received:

    "Headings must not be empty (empty-heading)"

    Fix any of the following:
      Element does not have text that is visible to screen readers

    You can find more information on this issue here: 
    https://dequeuniversity.com/rules/axe/3.5/empty-heading?application=axeAPI

    Expected the HTML found at $('ui5-responsive-popover,ui5-title,h5') to have no violations:

    <h5 class="ui5-title-root"><span id="ui5wc_4-inner"><slot></slot></span></h5>

    Received:

    "Headings must not be empty (empty-heading)"

    Fix any of the following:
      Element does not have text that is visible to screen readers

    You can find more information on this issue here: 
    https://dequeuniversity.com/rules/axe/3.5/empty-heading?application=axeAPI

    ────────

    Expected the HTML found at $('ui5-responsive-popover,ui5-title,h5') to have no violations:

    <h5 class="ui5-title-root"><span id="ui5wc_4-inner"><slot></slot></span></h5>

    Received:

    "Heading levels should only increase by one (heading-order)"

    Fix any of the following:
      Heading order invalid

    You can find more information on this issue here: 
    https://dequeuniversity.com/rules/axe/3.5/heading-order?application=axeAPI

      45 |   expect(overflowButtonPopover.childNodes.length).toEqual(NB_OF_BUTTONS);
      46 | 
    > 47 |   await expect(container).toHaveNoA11yViolations();

To reproduce
Steps to reproduce the behavior:
Create unit test for ui5-responsive-popover

Expected behavior
No A11y issue

Context

  • UI5 Web Components version 0.29.1

Affected components
ui5-responsive-popove

Organization:
Successfactors

Priority:
High because block unit tests.

@unazko unazko self-assigned this Feb 7, 2021
@ilhan007
Copy link
Member

ilhan007 commented Feb 8, 2021

Hello @mydavidzhang can you provide the exact markup so we can recreate your set up and look for those DOM elements and what they potentially miss. I can see that there are some ui5-title, ui5-button in the test, which might nothing to do with the ui5-responsive-popover itself. In order to investigate these errors we will need the markup to see what you are testing, for
example I expect to provide us something like this:

<ui5-responsive-popover>
		<div slot="header">
			<ui5-title>Hello World</ui5-title>
		</div>

		<div>
			<ui5-label>Email: </ui5-label>
		</div>

		<div slot="footer">
			<ui5-button>Subscribe</ui5-button>
		</div>
</ui5-responsive-popover>

@mydavidzhang
Copy link
Author

Thanks @ilhan007,
This issue happens when in mobile web version which shows empty dialog title with a close x button.
Please see below UI and source code which has been marked 1, 2, and 3 issues there.
Screen Shot 2021-02-08 at 9 37 04 AM
Screen Shot 2021-02-08 at 9 36 24 AM
Thanks

@ilhan007
Copy link
Member

ilhan007 commented Feb 9, 2021

Hello @SAP/ui5-webcomponents-topic-rd could you investigate the following ACC errors, related to the ResponsivePopover (and Dialog):

On mobile, the ResponsivePopover renders Dialog and displays Title and Button in header that causes some errors.
To test the setup you can use an empty ResponsivePopover that is opened (as dialog) in mobile .

If no headerText is provided, the Title (can be found in the ResponsivePopover.hbs) remains empty and it causes this error

  1. "Headings must not be empty (empty-heading)"
    Fix any of the following: Element does not have text that is visible to screen readers
    <h5 class="ui5-title-root"><span id="ui5wc_4-inner"><slot></slot></span></h5>

Same goes for the title rendered in the Dialog.hbs

  1. "Headings must not be empty (empty-heading)"
    Fix any of the following: Element does not have text that is visible to screen readers
    <h2 class="ui5-popup-header-text">{{headerText}}</h2>

Also, having h2 and then h5 seems to violate another ACC rule

  1. "Heading levels should only increase by one (heading-order)"
    Fix any of the following: Heading order invalid

The button (rendered in ResponsivePopover.hbs) seems not not have accessible name

  1. "Buttons must have discernible text (button-name)"

    Fix any of the following:
    Element does not have inner text that is visible to screen readers
    aria-label attribute does not exist or is empty
    aria-labelledby attribute does not exist, references elements that do not exist or references elements that are empty
    Element's default semantics were not overridden with role="presentation"
    Element's default semantics were not overridden with role="none"
    Element has no title attribute or the title attribute is empty

<button type="button" class="ui5-button-root" data-sap-focus-ref="" part="button" tabindex="0"><!----><ui5-icon class="ui5-button-icon" style="" name="decline" show-tooltip="" ui5-icon=""></ui5-icon><span class="ui5-button-text" id="ui5wc_5-content"><bdi><slot></slot></bdi></span><!----></button>

@ilhan007 ilhan007 added the bug This issue is a bug in the code label Feb 9, 2021
@ilhan007 ilhan007 removed their assignment Feb 9, 2021
@dimovpetar dimovpetar self-assigned this Feb 11, 2021
@dimovpetar dimovpetar removed their assignment Feb 12, 2021
@georgimkv georgimkv self-assigned this Feb 15, 2021
@georgimkv
Copy link
Contributor

Hi @mydavidzhang
The pull request #2823 will fix the reported errors in the ResponsivePopover.
The reported accessibility error for Dialog has already been solved by #2770.

ilhan007 pushed a commit that referenced this issue Feb 18, 2021
- When the header-text attribute is not provided, the Dialog (on mobile)
  does not render an empty title element
- header-text attribute results in a title with level H2
- Added aria-label to the "Close dialog" button

Fixes: #2787
@mydavidzhang mydavidzhang changed the title Successfactors: ui5-responsive-popover has A11y issue for mobile web SF_ACC Successfactors: ui5-responsive-popover has A11y issue for mobile web Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Completed
Development

Successfully merging a pull request may close this issue.

6 participants