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.

Sign up for GitHub

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

Fix accessibility issue #19518

Draft
wants to merge 14 commits into
base: develop
Choose a base branch
from
+47 −16
Draft

Fix accessibility issue #19518

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
2ff2563
Fix accessibility issue
Nov 8, 2024
4ec62a4
prettier fix
Nov 11, 2024
29f539a
Merge branch 'develop' into bugs-acc-issue
Jali-Khusi Dec 30, 2024
c1bbaa8
changes based on comment
Dec 31, 2024
9eb4d9a
changes based on comment
Dec 31, 2024
3c8ff77
changes based on comment
Jan 9, 2025
534fcc1
Merge branch 'develop' into bugs-acc-issue
Jali-Khusi Jan 10, 2025
01fcc26
prettier fix
Jan 10, 2025
7de3dd5
Merge branch 'develop' into bugs-acc-issue
Jali-Khusi Jan 21, 2025
fe6cf79
fix css
Jan 24, 2025
45ec5b5
fix css
Jan 25, 2025
cceccd7
fix based on myaccount
Jan 27, 2025
26fc967
fix based on comment
Feb 4, 2025
d8efe64
Merge branch 'develop' into bugs-acc-issue
Jali-Khusi Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion feature-libs/user/account/styles/_my-account-v2-user.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
.cx-my-account-v2-user {
border: 1px solid var(--cx-color-medium);
width: 250px;
padding: 20px 5px 5px 25px;
gap: 40px;
height: 120px;
Expand Down
5 changes: 4 additions & 1 deletion projects/assets/src/translations/en/common.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,10 @@
"goTo": "Go to {{location}}",
"navigateTo": "Navigate to {{nav}}",
"scrollToTop": "Scroll back to the top of the page",
"linkItemInList": "{{title}}. {{position}} of {{listLength}}"
"linkItemInList": "{{title}}",
"linkItemInLists": "{{title}}. {{position}} of {{listLength}}",
"navigation": "navigation",
"mainNavigation": "Main Navigation"
Pio-Bar marked this conversation as resolved.
Show resolved Hide resolved
},
"searchBox": {
"placeholder": "Enter product name or SKU",
Expand Down
19 changes: 6 additions & 13 deletions projects/storefrontlib/cms-components/navigation/navigation/navigation-ui.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<nav [attr.aria-label]="navAriaLabel">
<nav [attr.aria-label]="navAriaLabel" [attr.role]="'navigation'">
<ul
Pio-Bar marked this conversation as resolved.
Show resolved Hide resolved
[attr.role]="
flyout && (node?.children?.length ?? 0) > 1 ? 'list' : 'presentation'
Expand All @@ -8,7 +8,7 @@
*ngIf="flyout && (node?.children?.length ?? 0) > 1"
class="back is-open"
>
<button (click)="back()">
<button (click)="back()" [attr.aria-label]="'common.back' | cxTranslate">
<cx-icon [type]="iconType.CARET_LEFT"></cx-icon>
Pio-Bar marked this conversation as resolved.
Show resolved Hide resolved
{{ 'common.back' | cxTranslate }}
</button>
Expand All @@ -21,7 +21,6 @@
</ul>
</nav>
<!-- we generate links in a recursive manner -->

<ng-template
#nav
let-node="node"
Expand Down Expand Up @@ -56,8 +55,6 @@
| cxTranslate
: {
title: node.title,
position,
listLength,
}
Comment on lines -59 to -60
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Jali-Khusi Jali-Khusi Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"
[url]="node.url"
Expand All @@ -68,7 +65,6 @@
[tabindex]="getTabIndex(node, depth)"
(focus)="depth || reinitializeMenu()"
(keydown.space)="toggleOpen($any($event))"
(keydown.esc)="back()"
>
Pio-Bar marked this conversation as resolved.
Show resolved Hide resolved
{{ node.title }}
</cx-generic-link>
Expand Down Expand Up @@ -103,7 +99,6 @@
</ng-container>
<ng-container *cxFeature="'a11yNavigationUiKeyboardControls'">
<button
aria-level="4"
[attr.role]="(isDesktop$ | async) && depth ? 'heading' : 'button'"
Copy link
Contributor

@Pio-Bar Pio-Bar Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

[attr.aria-haspopup]="true"
[attr.aria-expanded]="false"
Expand All @@ -125,12 +120,7 @@
</ng-container>
</ng-container>
<ng-template #title>
<span
role="heading"
aria-level="4"
*ngIf="node.title"
[attr.tabindex]="-1"
>
<span role="presentation" *ngIf="node.title" [attr.tabindex]="0">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

{{ node.title }}
Pio-Bar marked this conversation as resolved.
Show resolved Hide resolved
</span>
</ng-template>
Expand All @@ -147,6 +137,9 @@
[attr.depth]="getTotalDepth(node)"
[attr.wrap-after]="node.children.length > wrapAfter ? wrapAfter : null"
[attr.columns]="getColumnCount(node.children.length)"
[attr.role]="
flyout && (node?.children?.length ?? 0) > 1 ? 'presentation' : 'list'
"
Pio-Bar marked this conversation as resolved.
Show resolved Hide resolved
>
<ng-container *ngFor="let child of node.children; index as i">
<ng-container
Expand Down
30 changes: 28 additions & 2 deletions projects/storefrontstyles/scss/components/content/navigation/_my-account-v2-navigation.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
background: var(--cx-color-inverse);
font-family: sans-serif;
padding: 5px 5px 5px 5px;
width: 250px;
width: 100%;
a {
font-size: var(--cx-font-medium);
font-weight: bold;
Expand All @@ -20,7 +20,8 @@
cx-navigation-ui {
background: var(--cx-color-inverse);
// Node Heading
span {
span,
h4 {
text-indent: 5px;
font-weight: bold;
display: block;
Expand All @@ -30,6 +31,8 @@
width: 250px;
height: 40px;
margin-top: 20px;
margin-bottom: 0px;
line-height: normal;
}

> nav > ul {
Expand All @@ -53,3 +56,26 @@
display: inline-block;
}
}

@media screen and (max-width: 544px) {
cx-navigation-ui h4,
span {
width: 100% !important;
font-size: 14px;
}
Pio-Bar marked this conversation as resolved.
Show resolved Hide resolved
cx-my-account-v2-navigation a {
width: 100% !important;
height: auto !important;
padding: 19px 0 19px 2px !important;
font-size: 12px !important;
}
}
@media screen and (max-width: 444px) {
cx-navigation-ui h4,
span {
font-size: 11px;
}
cx-my-account-v2-navigation a {
font-size: 10px !important;
}
}
2 changes: 0 additions & 2 deletions projects/storefrontstyles/scss/components/content/navigation/_navigation-ui.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,6 @@
padding-top: 10px;
padding-bottom: 12px;
padding-inline-start: 0;
margin-top: 10px;
margin-bottom: 10px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
gap-list

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Screenshot 2025-01-24 at 10 17 39

}
ul.childs > li > button:hover {
color: var(--cx-color-text);
Expand Down
37 changes: 37 additions & 0 deletions projects/storefrontstyles/scss/layout/page-templates/_myaccount-v2-page.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,40 @@
max-width: 75%;
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not use the !important to override styles.

}

.RightContentSlot {
max-width: 73% !important;
}
}
@media screen and (max-width: 855px) {
.LeftContentSlot {
max-width: 34% !important;
}

.RightContentSlot {
max-width: 64% !important;
}
}
@media screen and (max-width: 544px) {
.LeftContentSlot {
max-width: 34% !important;
}

.RightContentSlot {
max-width: 64% !important;
}
}
@media screen and (max-width: 444px) {
.LeftContentSlot {
max-width: 36% !important;
}

.RightContentSlot {
max-width: 63% !important;
}
}
Loading