Accessibility enhancement for the whole shop accounts section #47144
Conversation
github-actions
bot
added
the
plugin: woocommerce
naman03malhotra
force-pushed
the
imp/account-i18n-43635
branch
from
9c345d4
to
193b127
Compare
naman03malhotra
requested review from
a team and
jorgeatorres
and removed request for
a team
|
Hi @chihsuan, @moon0326, @woocommerce/ghidorah Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
naman03malhotra
added
focus: accessibility
naman03malhotra
force-pushed
the
imp/account-i18n-43635
branch
from
4c323b9
to
1894ca7
Compare
There was a problem hiding this comment.
Hi @naman03malhotra!
Thanks for picking this up. This seems to be working correctly for block themes, but not for other themes (such as Storefront).
The notices look different (and have different markup) in those:
Also, I wonder if there was a different way to solve this that was more "generic" than adding a script specifically for the accounts page. Not that I'm against that, but similar behavior (though not identical) is implemented in the checkout blocks, for example, when after an error the page scrolls to the notice (though IMHO it doesn't obtain the focus). And I can see this being necessary in other places.
It'd be good to ask Ghidorah for some feedback. What to do you think?
| @@ -27,7 +27,7 @@ | |||
|
|
|||
| ?> | |||
|
|
|||
| <div class="wc-block-components-notice-banner is-error" role="alert" <?php echo $multiple ? '' : wc_get_notice_data_attr( $notices[0] ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped ?>> | |||
There was a problem hiding this comment.
Why are we removing the role="alert" attribute here? Most (or all?) of the block-notices templates have that, so I guess there must be a reason or at least we should revaluate for consistency.
There was a problem hiding this comment.
Why are we removing the role="alert" attribute here? Most (or all?) of the block-notices templates have that, so I guess there must be a reason or at least we should revaluate for consistency.
That's a good point. For some reason, the other day while I was testing, having role="alert" didn't allow the screen reader to read the multi-line messages, but when I tested again on Safari (and Chrome) today, it was working fine, so I believe it depends on the browser as well.
Another important thing is that role="alert" is not supposed to be used in a full page reload scenario.
Ref: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/alert_role
That said,
- since it is a block template, I am assuming that it will be used in scenarios where we update the message inside the error container dynamically.
- A bunch of e2e tests also depend on this attribute, keeping all the factors in mind, it makes sense to keep this attribute.
Thanks for the detailed review @jorgeatorres, great catch! I've made the changes so that it works on both types of themes.
Good point, so I checked how that works. Actually, they are submitting the form for placing an order using an Ajax call and then updating an |
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. Test this pull request with WordPress Playground. Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit. |
Good idea, I've requested their feedback. |
There was a problem hiding this comment.
So, the most reliable way I found to grab the screen reader's attention after a full page reload was to bring the alert element in focus.
Thanks for sharing @naman03malhotra 🙏 , I did some investigation and agree with you that bringing the alert element seems the best way to grab the screen reader's attention.
I tested with VoiceOver on macOS Safari, and it works as expected with block themes. However, it doesn't work very well with classic themes such as "StoreFront". I tried a few times and sometimes it reads the alert message, but sometimes it doesn't. (It did focus the alert element.)
Screen.Recording.2024-05-14.at.15.15.18.mov
Additionally, I tested it on Chrome. I noticed that VoiceOver only read "Error:, and 1 more item, alert, main" and then stopped. It would be great if we could find a solution for this, but if not, I think we can go with this for now and improve it later.
Screen.Recording.2024-05-14.at.14.03.40.mov
I also checked how WordPress handles this on the login page, and it seems that they use aria-described by to associate the alert message with the input field and auto-focus on the input field when the page is loaded. I think this is a good approach, but I can see that we will need to make some changes to our code to make it work with our current implementation, so I don't think we can do this now. And if we want to do this, we should also consider if we can submit with Ajax directly.
| if (notices.length > 0) { | ||
| setTimeout(function() { | ||
| $(notices[0]).attr('tabindex', '-1').focus(); | ||
| }, 1000); |
There was a problem hiding this comment.
Could you explain why this specific delay is needed? 🙏
There was a problem hiding this comment.
So I am queuing the focus event at last of the event queue to override any other focus events in case of critical error. In my case, "Skip to content" was being focused just after the error, resulting in the voiceover breaking the message.
I've removed 1000 delay, and just kept the default timeout.
There was a problem hiding this comment.
@naman03malhotra Thanks for the explanation! That makes sense. Would you mind adding a comment to the code? That would be helpful for future developers.
There was a problem hiding this comment.
Would you mind adding a comment to the code?
Good suggestion, I will add
There was a problem hiding this comment.
Added here: 068a944
|
Thank you @chihsuan for an in-depth review 🙇
Hmm, that's strange, it is working well for me at least on Safari. Check the screencast below Screen.Recording.2024-05-20.at.21.00.26.mov
I also observed that issue; it looks like it is a Chrome thing; I tried to fiddle around to find a solution, so it looks like removing role="alert" solves the issue, but it causes problems in other browsers. Also, from all the testing, it seems like voice-over for Mac is most compatible with Safari, which I think most folks would be using while leveraging accessibility features.
+1 to this. I believe that we can have more robust accessibility with dynamic content submission instead of a full page reload, however that would require more code change. That said, I believe this change will give some relief to folks who need features to be accessible. Later, we can plan to change our accounts section to submit data using Ajax. |
naman03malhotra
force-pushed
the
imp/account-i18n-43635
branch
from
9883324
to
60e0b97
Compare
There was a problem hiding this comment.
Thanks for the addressing the feedback! I tested again with safari and it seems to be working fine. 👍
Code looks good to me except for the comment. I'd recommend testing with other browsers on Windows as well but I think this is good to go.
Code looks good to me except for the comment. Pre-approving!
Submission Review Guidelines:
If you the above steps in
trunkthe voice ready will not read out the errors.Changes proposed in this Pull Request:
This PR solves the Accessibility issues for the whole shop accounts section and closes several issues reported by the Author.
Closes #43635 , #43659 , #43636 , #46559
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
accountssection of the shop.CMD + F5). There must be other keys for other OS.Changelog entry
Significance
Type
Message
Comment