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

Add aria-expanded and aria-control attribbutes to gift card checkbox #2458

Closed
wants to merge 1 commit into from
+6 −1
Closed

Add aria-expanded and aria-control attribbutes to gift card checkbox #2458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions assets/recipient-form.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ if (!customElements.get('recipient-form')) {
if (!this.checkboxInput.checked) {
this.clearInputFields();
this.clearErrorMessage();
this.checkboxInput.setAttribute('aria-expanded', false)
} else {
this.checkboxInput.setAttribute('aria-expanded', true)
}
}

Expand Down
4 changes: 3 additions & 1 deletion snippets/gift-card-recipient-form.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
name="{{ gift_card_recipient_control_flag }}"
disabled
unchecked
aria-expanded="false"
Copy link
Contributor

@metamoni metamoni Mar 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think this isn't working as you'd expect because it's missing a role. aria-expanded works with the following associated roles. A button, for example, has an implicit role, so you wouldn't need to add one. But for an element without an implicit role, such as a div or input, the screen reader will require more context to understand what the widget does. The combobox role would probably be a good fit for this.

I'm not super familiar with this area, but I'd be happy to take a look at it myself if you add some detailed testing steps (some simple instructions on how to get to this screen/checkbox would be helpful). Feel free to reach out if you'd like to pair too.

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 how you'd go about testing the feature/fix: video

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, @ludoboludo. @antiBaconMachine and I have discussed this, as well as potential solutions. Here's a summary:

  • we both agreed that a checkbox might not be the best element to use here
  • I suggested changing the element to a button and using one of the following patterns: an accordion or a disclosure (which we use in the drawer menu)
  • I suggested checking with UX to find a good pattern and adjust the design if needed

Copy link
Contributor

Choose a reason for hiding this comment

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

Sweet, thanks for the update 🙂

aria-controls="Recipient-fields-{{ section.id }}"
>
<label class="recipient-checkbox" for="Recipient-Checkbox-{{ section.id }}">
<svg
Expand Down Expand Up @@ -76,7 +78,7 @@
{%- endif -%}
</ul>
</div>
<div class="recipient-fields">
<div class="recipient-fields" id="Recipient-fields-{{ section.id }}">
<hr>
<div class="recipient-fields__field">
<div class="field">
Expand Down