Accessibility add h2 to drawer filter #1564
Conversation
ghost
added
the
snippets/facets.liquid
Outdated
| @@ -381,6 +381,7 @@ | |||
| {%- endif -%} | |||
| {% comment %} Drawer and mobile filter {% endcomment %} | |||
| <menu-drawer class="mobile-facets__wrapper{% if filter_type == 'horizontal' or filter_type == 'vertical' %} medium-hide large-up-hide{% endif %}" data-breakpoint="mobile"> | |||
| <h2 class="visually-hidden" id="verticalTitle">{{ 'products.facets.filter_by_label' | t }}</h2> | |||
There was a problem hiding this comment.
Hmm isn't this going to create a duplicate ID since it will have one on mobile for the menu drawer and one for the desktop version 🤔
There was a problem hiding this comment.
This is true when Horizontal or vertical filters is selected.
I checked and it is not duplicated for drawer, as far as I can tell, because the same drawer is used for mobile and desktop.
snippets/facets.liquid
Outdated
| @@ -381,6 +381,7 @@ | |||
| {%- endif -%} | |||
| {% comment %} Drawer and mobile filter {% endcomment %} | |||
| <menu-drawer class="mobile-facets__wrapper{% if filter_type == 'horizontal' or filter_type == 'vertical' %} medium-hide large-up-hide{% endif %}" data-breakpoint="mobile"> | |||
| <h2 class="visually-hidden" id="verticalTitle">{{ 'products.facets.filter_by_label' | t }}</h2> | |||
There was a problem hiding this comment.
Right now, the heading is always set to Filter, but this title is not always going to be Filter.
(see lines 411 to 423 as an example).
On Desktop:
- On desktop, it is true, the
asidewill always point to Filter only, it's enabled, because sort is located outside of the drawer.
On mobile, we have a few considerations to make
- When sort is enabled only, it will need to read
Sort - When Filter is enabled only, it will read
Filter(what it currently is) - When Filter and Sort are enabled, it will need to read
Filter and Sort.
Question:
- Instead of adding a new H2, could we consider repurposing a heading(s) that are already inside of the drawer? We'd have to confirm with Scott that a11y wise, the position of them would be okay. I think it should, as they are being used to label the aside.
There was a problem hiding this comment.
Follow up question/observation.
When Horizontal or Vertical filters are enabled. We can end up with duplicate IDs in the DOM.
From testing, it looks like even though the desktop content is hidden, the aside will still reference the desktop verticalTitle id when on mobile.
There was a problem hiding this comment.
Instead of adding a new H2, could we consider repurposing a heading(s) that are already inside of the drawer?
Tested and aligned; let's reference the h2 in the drawer. And yes, let's avoid id duplication.
There was a problem hiding this comment.
Updated to reference the id in the drawer but well still have the id issue 🤔 I dont think we can add 2 aria-labelledby and even though they are not visually there they exist in the dom twice since you have the id html in the drawer and then vertical or horizontal desktop
There was a problem hiding this comment.
@martinamarien Interesting approach. If we're going this route we might as well add these strings to the hidden element at the end of the DOM and reference accordingly.
<aside aria-labelledby="{% if "desktop" == true %}a11y-facet-desktop{% else %}a11y-facet-mobile{% endif %}">
<!-- … -->
<ul hidden>
<li id="a11y-refresh-page-message">Choosing a selection results in a full page refresh.</li>
<li id="a11y-facet-desktop">Desktop specific heading</li>
<li id="a11y-facet-mobile">Mobile specific heading</li>
</ul>There was a problem hiding this comment.
Thanks for providing this input Scott.
{% if "desktop" == true %}a11y-facet-desktop{% else %}a11y-facet-mobile{% endif %}
Unless I am misunderstanding, I don't think we can do this. I'm not aware of a way to know if I am on desktop or mobile in liquid. So we would need to make the id fixed, but the heading "dynamic".
There was a problem hiding this comment.
Could we use the drawer/horizontal/vertical setting as a conditional?
If we can't distinguish for specific strings, not worries. "Filter" will suffice.
There was a problem hiding this comment.
Could we use the drawer/horizontal/vertical setting as a conditional?
No. because the Drawer/Horizontal/Vertical setting only affects the Desktop. So using it to determine the string wouldn't be valid.
On mobile, they all use the drawer, so depending on the settings, it could be Sort, Filter or Sort and Filter.
Since the <aside> is used on mobile and desktop I was trying to provide the right string.
If we are okay with using "filter" for Filter and Filter + Sort. Then I'd propose:
<aside aria-label="{% if "enable_filtering" %}Filter{% else %}Sort{% endif %}">
There was a problem hiding this comment.
Looks good. Just add some white space in between the words an should be good to go.
| @@ -407,7 +407,7 @@ | |||
| <div class="mobile-facets__inner gradient"> | |||
| <div class="mobile-facets__header"> | |||
| <div class="mobile-facets__header-inner"> | |||
| <h2 class="mobile-facets__heading medium-hide large-up-hide"> | |||
| <h2 class="mobile-facets__heading medium-hide large-up-hide" id="verticalTitle" tabindex="-1"> | |||
There was a problem hiding this comment.
Can you explain why we need the tabindex on the heading? I don't think I understand why it's needed. Thanks!
There was a problem hiding this comment.
Not required for the mobile/drawer context but is required when we implement #109.
|
@sofiamatulis, any update on this PR? |
PR Summary:
Add hidden h2 for the drawer filter
Why are these changes introduced?
Fixes #1561.
Demo links
Checklist