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

Jump to bottom

fix: (CXSPA-8034) remove aria label from title dropdown #19325

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from
Draft

fix: (CXSPA-8034) remove aria label from title dropdown #19325

wants to merge 5 commits into from

Conversation

StanislavSukhanov
Copy link
Contributor

@StanislavSukhanov StanislavSukhanov requested review from a team as code owners October 3, 2024 08:08
@github-actions github-actions bot marked this pull request as draft October 3, 2024 08:08
@StanislavSukhanov StanislavSukhanov marked this pull request as ready for review October 3, 2024 08:08
@cypress Cypress
Copy link

cypress bot commented Oct 3, 2024

spartacus    Run #45110

Run Properties:  status check passed Passed #45110  •  git commit 301c2b12f3 ℹ️: Merge 930a3b8b0e6d05e67acddf328dd3da325bf2faea into bc29bb55a496410e282a048374f8...
Project spartacus
Branch Review feature/CXSPA-8034-user-register-remove-aria-label-from-title-dropdown
Run status status check passed Passed #45110
Run duration 11m 24s
Commit git commit 301c2b12f3 ℹ️: Merge 930a3b8b0e6d05e67acddf328dd3da325bf2faea into bc29bb55a496410e282a048374f8...
Committer Stanislav Sukhanov
View all properties for this run ↗︎
Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 3
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 125
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎

@StanislavSukhanov StanislavSukhanov self-assigned this Oct 3, 2024
Pio-Bar
Pio-Bar previously approved these changes Oct 3, 2024
View reviewed changes
Zeyber
Zeyber previously approved these changes Oct 4, 2024
View reviewed changes
@github-actions github-actions bot marked this pull request as draft October 9, 2024 20:42
@developpeurweb
Copy link
Contributor

@StanislavSukhanov for some reason the combobox now gets identified as a "Link".
Also, in VoiceOver, we lost the item count narration.

link instead of combo

Please compare this with another optional combobox like the one on the Shipping Address step during Checkout. Let's please bring back the item count for VoiceOver and rework the code to make JAWS narrate the element as "edit combo".

edit combo

developpeurweb
developpeurweb requested changes Oct 9, 2024
View reviewed changes
Copy link
Contributor

@developpeurweb developpeurweb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Pls see previously attached screenshots

@Pio-Bar
Copy link
Contributor

Pio-Bar commented Oct 10, 2024

Screenshot 2024-10-10 at 16 26 18

Here's my JAWS output. It's not in the history, but I can hear the word "link" after tabbing to the input.
FYI @StanislavSukhanov The removed cxNgSelectA11y directive does more than just add an aria-label. This is why the count is missing on voice over probably.

@StanislavSukhanov StanislavSukhanov dismissed stale reviews from Pio-Bar and Zeyber via aede108 October 11, 2024 07:56
@StanislavSukhanov StanislavSukhanov force-pushed the feature/CXSPA-8034-user-register-remove-aria-label-from-title-dropdown branch from fbba011 to aede108 Compare October 11, 2024 07:56
@StanislavSukhanov StanislavSukhanov marked this pull request as ready for review October 11, 2024 07:56
@StanislavSukhanov
Copy link
Contributor Author

Hi @Pio-Bar. Thanks for the advice, I did suspected that too. I've put back the directive. The items count is present again in the Voice Over.

@github-actions GitHub Actions
Copy link
Contributor

Merge Checks Failed

Please push a commit to re-trigger the build. 
To push an empty commit you can use `git commit --allow-empty -m "Trigger Build"`

Pio-Bar
Pio-Bar previously approved these changes Oct 11, 2024
View reviewed changes
Zeyber
Zeyber requested changes Oct 16, 2024
View reviewed changes
feature-libs/user/profile/assets/translations/en/userProfile.json Outdated
@@ -30,7 +30,6 @@
"confirmNewPassword": "Confirm New Password",
"resetPassword": "Reset Password",
"createAccount": "Create an account",
"title": "Title",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about removing this translation key as it would constitute a breaking change. Can you confirm if the key has only been introduced to set the aria-label on ng-select? And nowhere else in the past?

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 @Zeyber! I have checked in the repository and couldn't find such combination other than in a place it was removed from.
Please let me know if I need to revert this change. Thanks a lot 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Zeyber reverted, fixed.

@StanislavSukhanov StanislavSukhanov force-pushed the feature/CXSPA-8034-user-register-remove-aria-label-from-title-dropdown branch from aede108 to 92714c2 Compare October 16, 2024 12:38
@github-actions github-actions bot marked this pull request as draft October 16, 2024 12:39
@StanislavSukhanov
Copy link
Contributor Author

@developpeurweb It is fixed. Could you please have a look on that again? Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zeyber Zeyber Zeyber approved these changes

@developpeurweb developpeurweb developpeurweb approved these changes

@Pio-Bar Pio-Bar Pio-Bar left review comments

Assignees

@StanislavSukhanov StanislavSukhanov

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@StanislavSukhanov @developpeurweb @Pio-Bar @Zeyber