Fix accessibility issue #19518

Jali-Khusi · 2024-11-08T06:58:29Z

List of bugs resolved in this PR:
https://jira.tools.sap/browse/CXSPA-7980
https://jira.tools.sap/browse/CXSPA-7979
https://jira.tools.sap/browse/CXSPA-7978
https://jira.tools.sap/browse/CXSPA-7983
https://jira.tools.sap/browse/CXSPA-7987
https://jira.tools.sap/browse/CXSPA-7981

Pio-Bar

These changes would be considered breaking. We should put them behind a feature toggle like other a11y ones. They can be recognized by the a11y prefix we include for each one.

projects/storefrontlib/cms-components/navigation/navigation/navigation-ui.component.html

projects/storefrontstyles/scss/components/content/navigation/_my-account-v2-navigation.scss

Pio-Bar

Some of my older concerns are still valid, Please let me know if you have any doubts, thanks.

projects/storefrontlib/cms-components/navigation/navigation/navigation-ui.component.html

Pio-Bar · 2025-01-08T11:40:02Z

projects/storefrontlib/cms-components/navigation/navigation/navigation-ui.component.html

-                  position,
-                  listLength,


Having a proper list count is one of the requirements we need to meet. Let's make sure it's preserved where needed.
https://jira.tools.sap/browse/CXSPA-8035

Thank you for the feedback. Regarding the JIRA ticket https://jira.tools.sap/browse/CXSPA-7979 , I addressed the issue where additional information like "list with 1 item, list with 3 items" was being read out when accessing the ‘Saved Carts’ list item within the ‘Order Information’ group. I’ve updated the aria-label to ensure only relevant information is announced, removing redundant details. The list count is preserved as needed, but without unnecessary repetition for screen readers.

Let me know if further adjustments are needed

The removed translation seems not to be related to the one mentioned in the ticket. ("list with 1 item, list with 3 items"). Each option should include its count within the list ( n of n options). However, if this case is different an exception would need to be made for it. We cannot remove this from other implementations as this is a functionality we need to upkeep.

projects/assets/src/translations/en/common.json

Pio-Bar

I do have concerns about some of the changes being made to the navigation-ui component. This component is reused within Spartacus for quite important features like the main navigation. We should be especially careful modifying it since the changes impact other implementations. Some a11y improvements have been made already and if we are going to introduce new ones they can't be to the detriment of the existing ones.

I've brought to attention that some of these changes are considered breaking. We need to properly handle those by using feature flags, this mitigates any possible problems our clients might face. More information can be found on the wiki and here is a nice writeup about the new FF system for CSS changes.

Feel free to reach out to me directly or any a11y team members for help!

Pio-Bar · 2025-01-14T12:27:58Z

projects/storefrontlib/cms-components/navigation/navigation/navigation-ui.component.html

-                  position,
-                  listLength,


The removed translation seems not to be related to the one mentioned in the ticket. ("list with 1 item, list with 3 items"). Each option should include its count within the list ( n of n options). However, if this case is different an exception would need to be made for it. We cannot remove this from other implementations as this is a functionality we need to upkeep.

Pio-Bar · 2025-01-14T12:29:19Z

projects/storefrontlib/cms-components/navigation/navigation/navigation-ui.component.html

@@ -106,7 +104,6 @@
        </ng-container>
        <ng-container *cxFeature="'a11yNavigationUiKeyboardControls'">
          <button
-            aria-level="4"


If the role is set to heading we need to specify the level too.

Pio-Bar · 2025-01-14T12:33:43Z

projects/storefrontlib/cms-components/navigation/navigation/navigation-ui.component.html

-        <span
-          role="heading"
-          aria-level="4"
-          *ngIf="node.title"
-          [attr.tabindex]="-1"
-        >


These attributes are required by the main navigation in the header and provide a good a11y experience for that use case. We need to satisfy both cases, we cannot remove one in favour of the other.

Hi @Pio-Bar
Thank you for your message. As per my understanding, I have implemented the changes accordingly. Could you kindly suggest if there is an alternative approach I could take that would allow me to make adjustments without affecting the current functionality?

Pio-Bar · 2025-01-14T12:34:34Z

projects/storefrontstyles/scss/components/content/navigation/_navigation-ui.scss

-        margin-top: 10px;
-        margin-bottom: 10px;


I am afraid these changes will influence the look of other implementations as well.

I removed the margin-top: 10px; and margin-bottom: 10px; because they were causing some unwanted gaps between the list items. If you'd prefer, I can revert this change

Here is screenshot from the current bugs-acc-issue branch. If we remove the margins the main navigation will shift.

I understand this fixes the issue for this particular case. At the same time we can not break other implementations. These changes should be kept within the affected component.

Pio-Bar · 2025-01-14T12:35:21Z

projects/storefrontstyles/scss/layout/page-templates/_myaccount-v2-page.scss

+
+@media screen and (max-width: 1024px) {
+  .LeftContentSlot {
+    max-width: 26% !important;


We should not use the !important to override styles.

Pio-Bar · 2025-01-24T09:29:16Z

projects/storefrontstyles/scss/components/content/navigation/_navigation-ui.scss

-        margin-top: 10px;
-        margin-bottom: 10px;


Here is screenshot from the current bugs-acc-issue branch. If we remove the margins the main navigation will shift.

I understand this fixes the issue for this particular case. At the same time we can not break other implementations. These changes should be kept within the affected component.

Pio-Bar · 2025-01-24T09:45:20Z

projects/storefrontstyles/scss/components/content/navigation/_navigation-ui.scss

@@ -321,6 +319,7 @@

 %cx-navigation-ui {
  display: flex;
+  display: contents;


This is a significant change, not sure why flex would be overwritten here. This will surely affect the layout.

Pio-Bar · 2025-02-04T11:40:25Z

...nents/myaccount/my-account-v2/my-account-v2-navigation/my-account-v2-navigation.component.ts

+export class MyAccountV2NavigationComponent extends NavigationComponent {
+  navAriaLabel$: Observable<any> = this.name$.pipe(
+    map((name: any) => name ?? 'MyAccount View Side Navigation')
+  );
+}


Changing the CMS data would modify the outcome of the logic based on this. If the name was added the count would be present too. It's probably not what we want.

We should not use a hard coded string for comparison.

Pio-Bar · 2025-02-04T11:46:28Z

projects/storefrontlib/cms-components/navigation/navigation/navigation-ui.component.html

-                  position,
-                  listLength,
-                }
+          navAriaLabel !== ('navigation.myAccountSideNavigation' | cxTranslate)


We probably should not base logic on a translation. Maybe we can include provideOptionCount input set to true by default. This way we would have an easy way to disable the option count where needed.

Pio-Bar · 2025-02-04T12:13:38Z

projects/storefrontlib/cms-components/navigation/navigation/navigation-ui.component.html

+          [attr.role]="
+            navAriaLabel !==
+            ('navigation.myAccountSideNavigation' | cxTranslate)
+              ? 'heading'
+              : 'presentation'
+          "
+          [attr.aria-level]="
+            navAriaLabel !==
+            ('navigation.myAccountSideNavigation' | cxTranslate)
+              ? 4
+              : null
+          "
          *ngIf="node.title"
-          [attr.tabindex]="-1"
+          [attr.tabindex]="
+            navAriaLabel !==
+            ('navigation.myAccountSideNavigation' | cxTranslate)
+              ? -1
+              : 0
+          "


Should the title be stripped of its semantic value? As mentioned previously, role presentation hides the element from accessibility software. In this case it's ignored because the tab index is 0. If these are a11y improvements, the intention here is not clear to me. Are we reducing the title to just a pice of text in this case?

Fix accessibility issue

2ff2563

Jali-Khusi requested review from a team as code owners November 8, 2024 06:58

github-actions bot marked this pull request as draft November 8, 2024 06:58

prettier fix

4ec62a4

vigneshkrish8691 previously approved these changes Nov 25, 2024

View reviewed changes

Pio-Bar requested changes Dec 19, 2024

View reviewed changes

Jali-Khusi and others added 3 commits December 30, 2024 11:52

Merge branch 'develop' into bugs-acc-issue

29f539a

changes based on comment

c1bbaa8

changes based on comment

9eb4d9a

Jali-Khusi dismissed vigneshkrish8691’s stale review via 9eb4d9a December 31, 2024 10:34

Pio-Bar requested changes Jan 8, 2025

View reviewed changes

projects/storefrontlib/cms-components/navigation/navigation/navigation-ui.component.html Outdated Show resolved Hide resolved

projects/storefrontlib/cms-components/navigation/navigation/navigation-ui.component.html Outdated Show resolved Hide resolved

Pio-Bar reviewed Jan 8, 2025

View reviewed changes

projects/assets/src/translations/en/common.json Outdated Show resolved Hide resolved

Kar and others added 3 commits January 9, 2025 14:52

changes based on comment

3c8ff77

Merge branch 'develop' into bugs-acc-issue

534fcc1

prettier fix

01fcc26

Pio-Bar requested changes Jan 14, 2025

View reviewed changes

Jali-Khusi and others added 2 commits January 21, 2025 10:47

Merge branch 'develop' into bugs-acc-issue

7de3dd5

fix css

fe6cf79

Pio-Bar requested changes Jan 24, 2025

View reviewed changes

Kar added 2 commits January 25, 2025 22:41

fix css

45ec5b5

fix based on myaccount

cceccd7

Pio-Bar requested changes Feb 4, 2025

View reviewed changes

Kar and others added 2 commits February 4, 2025 21:01

fix based on comment

26fc967

Merge branch 'develop' into bugs-acc-issue

d8efe64

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix accessibility issue #19518

Fix accessibility issue #19518

Jali-Khusi commented Nov 8, 2024 •

edited

Loading

Pio-Bar left a comment

Pio-Bar left a comment

Pio-Bar Jan 8, 2025

Jali-Khusi Jan 9, 2025 •

edited

Loading

Pio-Bar Jan 14, 2025

Pio-Bar left a comment •

edited

Loading

Pio-Bar Jan 14, 2025

Pio-Bar Jan 14, 2025 •

edited

Loading

Pio-Bar Jan 14, 2025

Jali-Khusi Jan 16, 2025

Pio-Bar Jan 14, 2025

Jali-Khusi Jan 23, 2025

Pio-Bar Jan 24, 2025

Pio-Bar Jan 14, 2025

Pio-Bar Jan 24, 2025

Pio-Bar Jan 24, 2025

Pio-Bar Feb 4, 2025

Pio-Bar Feb 4, 2025

Pio-Bar Feb 4, 2025

Fix accessibility issue #19518

Are you sure you want to change the base?

Fix accessibility issue #19518

Conversation

Jali-Khusi commented Nov 8, 2024 • edited Loading

Pio-Bar left a comment

Choose a reason for hiding this comment

Pio-Bar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jali-Khusi Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pio-Bar left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pio-Bar Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jali-Khusi commented Nov 8, 2024 •

edited

Loading

Jali-Khusi Jan 9, 2025 •

edited

Loading

Pio-Bar left a comment •

edited

Loading

Pio-Bar Jan 14, 2025 •

edited

Loading