-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Use live region to announce recipient form #2672
Conversation
c5ea6cf
to
d1df340
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good 👍 I'd just suggest a potential different approach but in the end it seem to work either way 🙂
<p | ||
id="Recipient-fields-live-region-{{ section.id }}" | ||
class="visually-hidden" | ||
aria-live="polite" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id="Recipient-fields-live-region-{{ section.id }}" | ||
class="visually-hidden" | ||
aria-live="polite" | ||
aria-hidden="true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I think we could potentially approach it differently. What you have now is a toggling between hidden and not hidden.
We could instead, have this p
always there, visually hidden the way you have it and use the JS, not to toggle aria-hidden
, but to inject the text.
Because it's a role="status"
, anytime something is added in that container, it will announce it if I'm not mistaking.
For the strings, you can add them in theme.liquid
like we do for:
window.accessibilityStrings = {
imageAvailable: `{{ 'products.product.media.image_available' | t: index: '[index]' }}`,
shareSuccess: `{{ 'general.share.success_message' | t }}`,
pauseSlideshow: `{{ 'sections.slideshow.pause_slideshow' | t }}`,
playSlideshow: `{{ 'sections.slideshow.play_slideshow' | t }}`,
};
This way it follows the pattern we've used in other part of the theme (eg: SKU on product page)
assets/recipient-form.js
Outdated
} else { | ||
this.clearInputFields(); | ||
this.disableInputFields(); | ||
this.clearErrorMessage(); | ||
this.recipientFieldsLiveRegion.innerText = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since usually it seem to be a pattern to have some feedback when toggling ON and OFF (aria hidden for example), I'd consider having a window.accessibilityStrings.recipientFormCollapsed
or something.
Other than that it's looking good 👍 Thanks for the changes :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, i'll request the translations.
Thanks again for doing the work Fred 👍
d13bddd
to
bae8cc5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked this over and tested, all works as expected. 👍
* Add animation to the announcement bar. * Add animation for the current and next slides. * Make animation works for auto-rotating. * Ensure the slider rotates in reverse when it reaches the end/begining. * Add condition to apply delay for announcement-bar only * Refactor and update some logic. * Refactoring. * Remove unneccessary css code. * Refactor css. Change the order. * Refactor JS. * Refactor css. * Isolate all logic in slideshowComponent. * Simplify applyAnimation method. * Reuse currentPage * Apply animation on selected slides. * Adjust duration of animation and shifting. * Refactoring * Another refactoring * Remove timeout and add delay in css * Refactoring * Add no-js * Prevent snapping during animation * Fix auto-rotation positioning * Refactor applyAnimation * Refactor and remove unused elements * [Announcement bar] Add social icons (#2497) * Rename announcement-bar to utility-bar section. Add basic logic for social-icons in the utility-bar. * Remove social icons from the utility bar for tablet Remove social icons snippet * Prevent auto-rotation after users interact with arrow buttons. Change the slider width for screens wider than 1200px. Rename back a file name from utility-bar to announcement-bar. * Prevent auto-rotation for mobile. Reduce social icons sizes for in the utility bar. Change grid system. * Prevent showing bottom-border when the utility bar is empty. * Add layout for screens under 1200px. * Adjust a separater line for max-width: 1199px * Revert announcement-bar name. Remove additional layout for desktop under 1120px. Remove JS logic. * Minor changes. * Refactor of css classes. Refactor of liquid logic. Use social-icons as a snippet. * Refactor in social-icons snippet. Change the naming in css accordingly BEM. * Update 14 translation files * Update 6 translation files * Change label for show_social. * Update 15 translation files * Update 5 translation files --------- Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> * Use live region to announce recipient form (#2672) * Use live region to announce recipient form * Inject text instead of toogling aria-hidden * Announce form collapsed when checkbox unticked * Update 20 translation files * Update 4 translation files * Update 6 translation files --------- Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> * Safari fix and addressing feadback * Correct typo * Remove unnecessary code * Prevent applying animation when you scroll * Remove console.log * Refactoring --------- Co-authored-by: Ludo <ludo.segura@shopify.com> Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> Co-authored-by: Fred Ma <frederic.ma@shopify.com>
Is it possible to add this functionality to a highly customized, older theme? I put the recipient form on a separate digital gift card product template and attached custom product properties to it, but that didn't seem to affect sending the gift card to the intended recipient. Would I have to make a custom app to accomplish this using the storefront API? It seems native to the Shopify platform now since the "Gift Card Confirmation" notification now includes this liquid tag in the notification template: {% if gift_card.recipient %} - but what exactly do I need in place for it to occur? I have both of these included in my theme now: |
* Use live region to announce recipient form * Inject text instead of toogling aria-hidden * Announce form collapsed when checkbox unticked * Update 20 translation files * Update 4 translation files * Update 6 translation files --------- Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
* Add animation to the announcement bar. * Add animation for the current and next slides. * Make animation works for auto-rotating. * Ensure the slider rotates in reverse when it reaches the end/begining. * Add condition to apply delay for announcement-bar only * Refactor and update some logic. * Refactoring. * Remove unneccessary css code. * Refactor css. Change the order. * Refactor JS. * Refactor css. * Isolate all logic in slideshowComponent. * Simplify applyAnimation method. * Reuse currentPage * Apply animation on selected slides. * Adjust duration of animation and shifting. * Refactoring * Another refactoring * Remove timeout and add delay in css * Refactoring * Add no-js * Prevent snapping during animation * Fix auto-rotation positioning * Refactor applyAnimation * Refactor and remove unused elements * [Announcement bar] Add social icons (Shopify#2497) * Rename announcement-bar to utility-bar section. Add basic logic for social-icons in the utility-bar. * Remove social icons from the utility bar for tablet Remove social icons snippet * Prevent auto-rotation after users interact with arrow buttons. Change the slider width for screens wider than 1200px. Rename back a file name from utility-bar to announcement-bar. * Prevent auto-rotation for mobile. Reduce social icons sizes for in the utility bar. Change grid system. * Prevent showing bottom-border when the utility bar is empty. * Add layout for screens under 1200px. * Adjust a separater line for max-width: 1199px * Revert announcement-bar name. Remove additional layout for desktop under 1120px. Remove JS logic. * Minor changes. * Refactor of css classes. Refactor of liquid logic. Use social-icons as a snippet. * Refactor in social-icons snippet. Change the naming in css accordingly BEM. * Update 14 translation files * Update 6 translation files * Change label for show_social. * Update 15 translation files * Update 5 translation files --------- Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> * Use live region to announce recipient form (Shopify#2672) * Use live region to announce recipient form * Inject text instead of toogling aria-hidden * Announce form collapsed when checkbox unticked * Update 20 translation files * Update 4 translation files * Update 6 translation files --------- Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> * Safari fix and addressing feadback * Correct typo * Remove unnecessary code * Prevent applying animation when you scroll * Remove console.log * Refactoring --------- Co-authored-by: Ludo <ludo.segura@shopify.com> Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> Co-authored-by: Fred Ma <frederic.ma@shopify.com>
PR Summary:
This PR implements support for accessibility by announcing the recipient form when the checkbox is ticked/unticked on the gift card product page
Why are these changes introduced?
Fixes https://github.com/Shopify/core-issues/issues/55523
What approach did you take?
The original plan was to use
aria-expanded
andaria-controls
but those are not supported for acheckbox
role. To make it work, we have to use a supported role e.g.button
but to align with UX design, we are keeping the checkbox as planned.So the alternative is now using a live region to announce the form when the checkbox is ticked/unticked
Other considerations
Decision log
aria-expanded
aria-expanded
is not supported forcheckbox
roleVisual impact on existing themes
No impact on themes unless a11y feature is turned on.
See video
Testing steps/scenarios
Demo links
Checklist