Skip to content

[Tokenizer]: Screen reader reads "Tokenizer" twice, inside focusable wrapper #10361

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

Closed
1 task done
PlutaKatarzyna opened this issue Dec 11, 2024 · 16 comments
Closed
1 task done

Comments

@PlutaKatarzyna
Copy link

Bug Description

Hi UI5 team,
We faced the issue while trying to use UI5 Tokenizer in UDEx Select Box component.
It is used to display selected values from dropdown list. For that purpose, there is a ui5-tokenizer wrapper - div element that has tabindex="0". Focusing on that wrapper cause reading relevant information, like label, but also it cause reading "Tokeniser" twice.

Sometimes it happens that when you focus on the field fot the first time, it reads it only once, but then when you move out and focus again, it is alredy read twice.

Affected Component

ui5-tokenizer , udex-select-box

Expected Behaviour

We seek for the way to avoid "Tokeniser" being read twice.

Isolated Example

https://sap.github.io/ui5-webcomponents/v1/play/#eyJpbmRleC5odG1sIjp7Im5hbWUiOiJpbmRleC5odG1sIiwiY29udGVudCI6IjwhLS0gcGxheWdyb3VuZC1mb2xkIC0tPlxuPCFET0NUWVBFIGh0bWw-XG48aHRtbCBsYW5nPVwiZW5cIj5cblxuXG48aGVhZD5cbiAgICBcbiAgICA8c3R5bGU-XG4gICAgICAqOm5vdCg6ZGVmaW5lZCkge1xuICAgICAgICBkaXNwbGF5OiBub25lO1xuICAgICAgfVxuICAgIDwvc3R5bGU-XG5cbiAgICBcbiAgICA8c3R5bGU-XG4gICAgICAqOm5vdCg6ZGVmaW5lZCkge1xuICAgICAgICBkaXNwbGF5OiBub25lO1xuICAgICAgfVxuICAgIDwvc3R5bGU-XG5cbiAgICA8bWV0YSBjaGFyc2V0PVwiVVRGLThcIj5cbiAgICA8bWV0YSBuYW1lPVwidmlld3BvcnRcIiBjb250ZW50PVwid2lkdGg9ZGV2aWNlLXdpZHRoLCBpbml0aWFsLXNjYWxlPTEuMFwiPlxuICAgIDx0aXRsZT5TYW1wbGU8L3RpdGxlPlxuICA8c3R5bGU-XG4gICAgLnVkZXgtd3JhcHBlciB7XG4gICAgYm9yZGVyOiAxcHggc29saWQgIzAwMDtcbiAgICBib3JkZXItcmFkaXVzOiAzcHg7XG4gICAgfVxuICAgIC51ZGV4LXdyYXBwZXI6Zm9jdXMge1xuICAgIGJvcmRlci13aWR0aDogM3B4O1xuICAgIGJvcmRlci1jb2xvcjogYmx1ZTtcbiAgICB9XG4gICAgLnVpNS1tdWx0aS1pbnB1dC10b2tlbml6ZXIge1xuICAgIGJvcmRlcjogMDtcbiAgICB9XG4gIDwvc3R5bGU-XG48L2hlYWQ-XG48Ym9keSBzdHlsZT1cImJhY2tncm91bmQtY29sb3I6IHZhcigtLXNhcEJhY2tncm91bmRDb2xvcik7aGVpZ2h0OiAzNTBweDtcIj5cbjwhLS0gcGxheWdyb3VuZC1mb2xkLWVuZCAtLT5cbiAgPHVpNS1idXR0b24-c29tZSBmb2N1c2FibGUgLyBub3QgcmVsZXZhbnQ8L3VpNS1idXR0b24-XG4gIDxici8-PGJyLz5cbiAgPGRpdiBjbGFzcz1cInVkZXgtd3JhcHBlclwiIHRhYmluZGV4PVwiMFwiPlxuICBcdDx1aTUtdG9rZW5pemVyXG5cdFx0Y2xhc3M9XCJ1aTUtbXVsdGktaW5wdXQtdG9rZW5pemVyXCJcblx0ICA-XG4gICAgICA8dWk1LXRva2VuIHRleHQ9XCJncmVlblwiPjwvdWk1LXRva2VuPlxuICAgICAgPHVpNS10b2tlbiB0ZXh0PVwiaGVhbHRoeVwiIHNlbGVjdGVkID48L3VpNS10b2tlbj5cbiAgICAgIDx1aTUtdG9rZW4gdGV4dD1cInZlZ2FuXCIgPjwvdWk1LXRva2VuPlxuICAgICAgPHVpNS10b2tlbiB0ZXh0PVwibG93IGZhdFwiID48L3VpNS10b2tlbj5cblx0PC91aTUtdG9rZW5pemVyPlxuPC9kaXY-XG48IS0tIHBsYXlncm91bmQtZm9sZCAtLT5cbiAgICA8c2NyaXB0IHR5cGU9XCJtb2R1bGVcIiBzcmM9XCJtYWluLmpzXCI-PC9zY3JpcHQ-XG48L2JvZHk-XG48L2h0bWw-XG48IS0tIHBsYXlncm91bmQtZm9sZC1lbmQgLS0-XG5cbiJ9LCJtYWluLmpzIjp7Im5hbWUiOiJtYWluLmpzIiwiY29udGVudCI6Ii8qIHBsYXlncm91bmQtaGlkZSAqL1xuaW1wb3J0IFwiLi9wbGF5Z3JvdW5kLXN1cHBvcnQuanNcIjtcbi8qIHBsYXlncm91bmQtaGlkZS1lbmQgKi9cbmltcG9ydCBcIkB1aTUvd2ViY29tcG9uZW50cy9kaXN0L011bHRpSW5wdXQuanNcIjtcbmltcG9ydCBcIkB1aTUvd2ViY29tcG9uZW50cy9kaXN0L1Rva2VuLmpzXCI7XG5cbmNvbnN0IG11bHRpSW5wdXQgPSBkb2N1bWVudC5nZXRFbGVtZW50QnlJZChcIm11bHRpLWlucHV0XCIpXG5cbmNvbnN0IGNyZWF0ZVRva2VuRnJvbVRleHQgPSAodGV4dCkgPT4ge1xuICAgIGxldCB0b2tlbiA9IGRvY3VtZW50LmNyZWF0ZUVsZW1lbnQoXCJ1aTUtdG9rZW5cIik7XG4gICAgdG9rZW4uc2V0QXR0cmlidXRlKFwidGV4dFwiLCB0ZXh0KTtcbiAgICB0b2tlbi5zZXRBdHRyaWJ1dGUoXCJzbG90XCIsIFwidG9rZW5zXCIpO1xuICAgIHJldHVybiB0b2tlbjtcbn07XG5cbm11bHRpSW5wdXQuYWRkRXZlbnRMaXN0ZW5lcihcImNoYW5nZVwiLCAoZXZlbnQpID0-IHtcblx0Y29uc3QgaW5wdXRWYWx1ZSA9IGV2ZW50LnRhcmdldC52YWx1ZTtcblxuICAgIGlmIChpbnB1dFZhbHVlKSB7XG5cdFx0bXVsdGlJbnB1dC5hcHBlbmRDaGlsZChjcmVhdGVUb2tlbkZyb21UZXh0KGlucHV0VmFsdWUpKTtcblx0XHRtdWx0aUlucHV0LnZhbHVlID0gXCJcIjtcbiAgICB9XG59KTtcblxubXVsdGlJbnB1dC5hZGRFdmVudExpc3RlbmVyKFwidG9rZW4tZGVsZXRlXCIsIChldmVudCkgPT4ge1xuICAgIGNvbnN0IHRva2VuID0gZXZlbnQuZGV0YWlsPy50b2tlbjtcbiAgICB0b2tlbiAmJiB0b2tlbi5yZW1vdmUoKTtcbn0pOyJ9fQ

Steps to Reproduce

  1. Run screen reader to read focused items
  2. Focus on element that is direct wrapper of ui5-tokenizer and has tabindex=0 .
  3. As example also you can use UDEx component https://pages.github.tools.sap/sapudex/digital-design-system/?path=/docs/components-actions-select-box--docs&globals=theme:Horizon+Brand . If used ui5 component, first select any item from dropdown list, for tokens to be added into the field
  4. Reading contains "Tokeniser" twice

Simple source code to reproduce:

Log Output, Stack Trace or Screenshots

Screen.Recording.2024-12-11.at.10.47.02.mov
Screen.Recording.2024-12-11.at.14.30.14.mov

Priority

None

UI5 Web Components Version

1.24.13

Browser

Chrome

Operating System

MacOS , Windows

Additional Context

Used screen readers: Mac VoiceOver, Windows JAWS
UDEx bug ticket: https://github.tools.sap/sapudex/digital-design-system/issues/955

Organization

SAP / UDEx components team

Declaration

  • I’m not disclosing any internal or sensitive information.
@PlutaKatarzyna PlutaKatarzyna added the bug This issue is a bug in the code label Dec 11, 2024
@dmitrykostenko
Copy link

I have done an investigation in the browser and it helped me solve the problem to changing the aria-labelledby to aria-describedby
image
image

@plamenivanov91
Copy link
Contributor

Hello Hello @SAP/ui5-webcomponents-topic-rl,

I can confirm that Tokenizer is being read twice in the provided sapudex link following the steps. I used JAWS 2024.

There is also a suggestion how to fix the issue in the previous comment.

Is this supposed to be fixed on Tokenizer side? Could you please take over?

Regards,
Plamen Ivanov

@StefanDimitrov04
Copy link
Contributor

Hello @PlutaKatarzyna,

Thank you for raising this issue. Unfortunately, I am unable to reproduce it on Chrome version 131.0.6778.86 and JAWS version 2024.2409.2.

TokenizerJAWS.mp4

Additionally, our accessibility expert provided the following input:

Adding a tabindex to an element without proper semantics, accessible roles or attributes is fundamentally incorrect. It may confuse screen reader users and lead (like in this case) to undesired speech output and accessibility issues.
Regarding the fix proposal - elements with role=listbox are required to be labeled as per the accessibility standard, so this is the reason we add aria-labelledby. Removing the aria-labelledby will result in accessibility validation errors which is not desired.

Regards,
Stefan Dimitrov

@PlutaKatarzyna
Copy link
Author

Hi @StefanDimitrov04 ,
We were surprised to see that you cannot reproduce that, so we tried again on different screen readers. We check JAWS, Narrator, VoiceOver. For all we can reproduce that.

On JAWS we noticed that it is read correctly (tokeniser is read once), when you navigate using arrow keys. But when you navigate using Tab key, reading is broken (Tokenise read twice). Could you please check that again with navigating different way?

About the comment from accessibility experts, we cover element with tabindex with aria attributes, since we are using it as Select Box (role="combobox" aria-haspopup="listbox" aria-label="..." aria-activedescendant aria-expanded="..."), but I did not add them in playground to simplify example and point out exact source of issue. If you spot some obvious issue with our approach, please let us know.

Thanks
Kate

@elenastoyanovaa
Copy link
Contributor

Hello @PlutaKatarzyna ,

I am the team accessibility expert. I saw your Select example and I am not sure if this usage of a combobox wrapper is correct as it does not follow the current accessibility standard https://www.w3.org/TR/wai-aria-1.2/#combobox and neither of the patterns https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-select-only/. Nevertheless, we always need to have the a label in the tokenizer, because it is required always to have one. As far as I analyzed the accessibility tree in Chrome of your Select componet, the value of the component is calculated from all textual content inside the element with role=combobox + all labels. IMO nesting listbox inside combobox is incorrect. A suggestion from my side is to check our ui5-multi-combobox which also has a tokenizer and a role=combobox, but they are not nested: https://sap.github.io/ui5-webcomponents/components/MultiComboBox/
Nevertheless, from ui5-tokenizer side we do not see an issue as the standalone component is implemented as per standard and the issue occurs only on a custom wrapper, with custom accessibility on top. If you think that the implementation of your Select component is as per the accessibility standard and does not violate accessibility my suggestion is to isolate your issue with plain html and report it to the screen reader vendor.

Kind Regards,
Elena

@PlutaKatarzyna
Copy link
Author

Thank you @elenastoyanovaa for reviewing our component. We'll investigate our component approach and compare with UI5 component.

@korotkovkonstantine
Copy link

I researched the ui5 MultiComboBox 1.24.13 example (we are using 1.24.12) and found a bug in the tokenizer hidden field. This hidden field has the static value "Tokenizer".

Screenshot 2025-01-15 212124

Since the tokenizer additionally has aria-label="Tokenizer", the screen reader reads "Tokenizer" twice.

This issue doesn`t reproduce in version 2.6.2 as the hidden field inside the tokenizer has been removed.

Screenshot 2025-01-15 213152

Apart from that, Tokeniser is visible twice in Accessibility tab in version 1.24

Image

@ilhan007
Copy link
Member

@SAP/ui5-webcomponents-topic-rl @elenastoyanovaa @hristop can someone follow up it could be that a change has not been downpored to v1, since the issue is resolved in v2?

@ilhan007 ilhan007 reopened this Feb 20, 2025
@github-project-automation github-project-automation bot moved this from Completed to In progress in Maintenance - Topic RL Feb 20, 2025
@elenastoyanovaa
Copy link
Contributor

Hello,

As described in my previous replies, there is no issue on our side. The real issue is with the improper nesting of the control and it is reproducible only in custom wrapper scenario. As you may check in other ui5 web components all text that are mapped to aria-label/aria-labelledby/aria-describedby are implemented in such a way, they are a visually hidden spans, that are passed to the focused inner elements as a different kind of aria attribute.
We are currently working on a solution for you in order to fit your case, but please keep in mind that future fixes for the ui5-tokenizer will not be made in 1.24 as the ui5-tokenizer is a public component from 2.0 and it should not be used in 1.24 as it was implemented to fit internal usage inside our components. Also please follow up with my previous comments and revise your screen reader implementation of the custom components.

Kind Regards,
Elena

@korotkovkonstantine
Copy link

korotkovkonstantine commented Feb 20, 2025

Hi @elenastoyanovaa could you please look at this screenshot?

Image

the problem is in the hidden text that the tokenizer refers to, in version 2 it was fixed.

Regarding the screen reader, we are using standard NVDA, nothing special.
Thanks for your comments

Best regards,
Kostiantyn

@elenastoyanovaa
Copy link
Contributor

Hello @korotkovkonstantine ,

As I explained all static texts created to be linked to aria attributes are implemented in this way. You can check other ui5 web components as well and there accessibility tab. Many static texts will be visible, but this is not a reason for them to be announced (and they are not). The listbox is labelled with the span via aria-labelledby, it is not a second "Tokenizer", just a reference. This is a simple usage of aria-labelledby, please check the accessibility tab of this simple native html example: https://jsbin.com/wobuqemeka/edit?html,output
The resulting Accessibility tree is not incorrect, the side effect is coming from your custom implementation. I double checked with NVDA, the issue is not reproducible in our ui5-multicombobox, ui5-multiinput.
Regardless, as I said in my last comment we will perform a fix for your custom case, even though usage of private components is not supported and in future we won't perform fixes or enhancements for the ui5-tokenizer in 1.24.

Elena

@korotkovkonstantine
Copy link

korotkovkonstantine commented Feb 20, 2025

Image

@elenastoyanovaa Thanks for your help, our accessibility qa team is using NVDA 2024.4.2.35031.

I figured out why this keeps happening. The screen reader is being misused initially. If you test only with the keyboard, it doesn't happen, but if you use these steps: focus by the tab on the MultiComboBox and then click outside, the reader reads all the hidden texts.
This is not logical behavior for visually impaired users.
I need to discuss it with our QA team.

Kostiantyn

@Okiana
Copy link
Contributor

Okiana commented Feb 25, 2025

Hello @korotkovkonstantine,

As my colleague @elenastoyanovaa has written, we've provided a fix for your scenario. It's provided in 1.24.

Evdokia

@Okiana Okiana closed this as completed Feb 25, 2025
@github-project-automation github-project-automation bot moved this from In progress to Completed in Maintenance - Topic RL Feb 25, 2025
@PlutaKatarzyna
Copy link
Author

Hi @Okiana , thank you for the updates. When can we expect version 1.24.17 with this fix to be released?

@ilhan007
Copy link
Member

ilhan007 commented Feb 28, 2025

Hello @PlutaKatarzyna

every first week of the month until July (as v1 support is coming to an end) as described here

Image

3rd March is a national holiday, so 4th -7th March

@ui5-webcomponents-bot
Copy link
Collaborator

🎉 This issue has been resolved in version v1.24.17 🎉

The release is available on v1.24.17

Your semantic-release bot 📦🚀

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

No branches or pull requests

9 participants