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

Button: does not show the arial-label correctly to nvda screen reader when icon is full-screen #3699

Closed
gurkirpalgill opened this issue Aug 19, 2021 · 18 comments · Fixed by #4034

Comments

@gurkirpalgill
Copy link

Describe the bug
When Button is used with icon value set to full-screen, nvda screen reader two different texts:
Enter Full Screen graphic clickable button Expand Onboarding Details Screen

Instead of Full Screen above it should be Expand Onboarding Details Screen as it is for other icon values.

Isolated Example

              <Button
                disabled={disableFullScreenBtn}
                key={"fullScreen"}
                data-testid="fullScreenIcon"
                icon="full-screen"
                design={ButtonDesign.Transparent}
                onClick={throttle(onFullScreenClick, setDisableExitFullScreenBtn)}
                tooltip={i18n.getText("ONBOARDING_DETAILS_EXPAND_SCREEN")}
                arial-label={i18n.getText("ONBOARDING_DETAILS_EXPAND_SCREEN")}
              />

Screenshots
Screen Shot 2021-08-18 at 5 01 26 PM

UI5 Web Components for React Information
@ui5/webcomponents version:~0.31.9
@ui5/webcomponents-react version: ^0.131.9
Operating System: Windows 10
Browser: Chrome

Additional context
nvda screen reader is used

@MarcusNotheis
Copy link
Collaborator

Hey @gurkirpalgill,
I just saw that you are using arial-label in the sample and not aria-label. Is this a typo only here in the example or is it in your implementation as well?

@gurkirpalgill
Copy link
Author

gurkirpalgill commented Aug 19, 2021

Hey @gurkirpalgill,
I just saw that you are using arial-label in the sample and not aria-label. Is this a typo only here in the example or is it in your implementation as well?

It's not a typo, I tried to use accessibleName instead of aria-label, but then typescript gives this error:
Screen Shot 2021-08-19 at 9 18 42 AM

@Lukas742
Copy link
Collaborator

Lukas742 commented Aug 20, 2021

Hi @gurkirpalgill

what Marcus meant is that you wrote arial-label with an "l" instead of aria-label, but since it seems like you are using typescript, I guess this is a typo just in the example. Also the accessibleName is only available with rc.15 of the @ui5/webcomponents, so if you want to use this prop instead, they would need to downport it to the respective sf branch.

For the issue itself, I'll forward it to the UI5 Web Components repo, since the affected component is developed by our colleagues there.

@Lukas742 Lukas742 transferred this issue from SAP/ui5-webcomponents-react Aug 20, 2021
@gurkirpalgill
Copy link
Author

my mistake, that's a typo. issue still exists when using nvda screen reader. screen reader is reading the same text twice.

Screen Shot 2021-08-20 at 12 12 37 AM

another example, when the focus is on ">" icon of flexible column layout and Enter key is pressed

Screen Shot 2021-08-20 at 12 04 38 AM

Please downport "accessibleName" as additional non breaking option to be consistent with the documentation.

@unazko
Copy link
Contributor

unazko commented Aug 20, 2021

Hello @gurkirpalgill,

Could you please edit the following snipped:
https://codesandbox.io/s/strange-davinci-boc66?file=/src/index.js
so the described issue is reproducible.
The mentioned @ui5/webcomponents and @ui5/webcomponents-react are already chosen.
Also have you tried reproducing the issue via jaws 2019 or jaws 2021 screen reader?

Best Regards,
Boyan

@ilhan007 ilhan007 added SAP SF and removed SAP SF labels Aug 27, 2021
@gurkirpalgill
Copy link
Author

@unazko
Edited your snipped and saved as:

https://codesandbox.io/s/adoring-faraday-5zlkp
of full screen:
https://5zlkp.csb.app/

As you can see in the screenshot below, the issue is reproducible.

Screen Shot 2021-09-07 at 5 11 27 PM

@georgimkv
Copy link
Contributor

Hi @SAP/ui5-webcomponents-topic-b
Can you have a look at this?

Kind regards,
Georgi

@gurkirpalgill
Copy link
Author

any updates on this?

@Neeeko
Copy link

Neeeko commented Sep 24, 2021

hi, our release is due October 1st, do you have any updates on this issue? Thanks!

@unazko
Copy link
Contributor

unazko commented Sep 27, 2021

Hello @Neeeko and @SAP/ui5-webcomponents-topic-p colleagues,

According to the specification the icon only button should have a tooltip and an aria-label and they both will get announced. In the described case they have the same texts and double announcement appears when NVDA is used. JAWS pronounces the aria-label text only as the tooltip has the same text.

A possible workaround for the ui5-flexible-column-layout component is to remove the aria-label attribute from the expand/collapse button:
aria-label="{{accStartArrowText}}" and aria-label="{{accEndArrowText}}"
in FlexibleColumnLayout.hs. This is an icon only button with the same tooltip and aria-label so there should be no
unwanted side effects from the change.

Best Regards,
Boyan

@unazko unazko removed their assignment Sep 27, 2021
@unazko unazko added TOPIC P and removed TOPIC B labels Sep 27, 2021
@nnaydenow nnaydenow self-assigned this Sep 28, 2021
@nnaydenow
Copy link
Contributor

Hi @gurkirpalgill,

Could you check if the issue persist in newer version where accessible-name property is introduced?

Regards,
Nayden

@unazko
Copy link
Contributor

unazko commented Sep 29, 2021

Hello @nnaydenow,

I forgot to mention that this issue isn't reproducible in the newer versions of the project.
The reason is that in the ui5-button ariaLabel property is renamed to accessibleName.
ui5-flexible-column-layout though in the FlexibleColumnLayout.hs is still suppling to its ui5-button element an aria-label attribute instead of the new accessible-name attribute.
The final result is if the aria-label isn't given as argument at all.
So unfortunately the issue isn't fixed in the master branch as this is the way the ui5-button works, but gets hidden instead.

Best Regards,
Boyan

@nnaydenow
Copy link
Contributor

Hi @unazko ,

Even if I change the attribute (will link PR soon) the tooltip is read too when I test the issue with NVDA.

You can check this snippix:
https://jsbin.com/zaketakufa/1/edit?html,output

Seems like screen reader specific issue.

@dobrinyonkov could you check this?

Regards,
Nayden

@unazko
Copy link
Contributor

unazko commented Sep 29, 2021

Hi @nnaydenow,

Yes, this is expected. That is why the suggested workaround from our team accessibility expert is to remove the aria-label attribute.

Best Regards,
Boyan

@dobrinyonkov
Copy link
Contributor

Hi all,

I would say that the suggested solution is the right thing to do. There shouldn't be title and aria-label attributes with the same value as it will get announced twice. Some screen readers might be smart enough to detect this and read the value once, some might not.
Especially necessary for icon-only buttons is the tooltip (title attribute). So we should be OK to remove the redundant aria-label attrtibute.

Regards,
Dobrin

@ilhan007
Copy link
Member

ilhan007 commented Oct 6, 2021

We have recently merged a fix.
We will close the issue once the fix is released.

@gurkirpalgill
Copy link
Author

@ilhan007 when will it be released in successfactors versions?

@ilhan007
Copy link
Member

@gurkirpalgill it is released with 0.31.21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Completed
Development

Successfully merging a pull request may close this issue.

10 participants