Improve accessibility labels for filters tab and button #4396
Conversation
openverse-bot
added
🟨 priority: medium
github-actions
bot
added
🧱 stack: frontend
AetherUnbound
removed
the
🚦 status: awaiting triage
Nope, it's handled automatically in this GitHub workflow when merging to main: https://github.com/WordPress/openverse/actions/workflows/generate_pot.yml After merging, no need for any further intervention. |
There was a problem hiding this comment.
I looked at the visual regression test failures, but didn't understand what the problem is yet. As for the e2e tests, this patch should fix some of them:
Subject: [PATCH] Add catalog media properties docs
Signed-off-by: Olga Bulat <[email protected]>
---
Index: frontend/test/playwright/utils/navigation.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/frontend/test/playwright/utils/navigation.ts b/frontend/test/playwright/utils/navigation.ts
--- a/frontend/test/playwright/utils/navigation.ts (revision d5123efe3d891dad612c8581d1fd36f166b0d9a5)
+++ b/frontend/test/playwright/utils/navigation.ts (date 1716920676271)
@@ -50,11 +50,10 @@
export const openContentSettingsTab = async (
page: Page,
tab: "searchTypes" | "filters" = "searchTypes",
- dir: LanguageDirection = "ltr"
) => {
- const tabKey = tab === "searchTypes" ? "searchType.heading" : "filters.title"
+ const tabId = tab === "searchTypes" ? "content-settings" : "filters"
- await page.getByRole("tab", { name: t(tabKey, dir) }).click()
+ await page.locator(`#tab-${tabId}`).click()
}
/**
@@ -105,7 +104,7 @@
await buttonLocator.isEnabled()
await buttonLocator.click()
}
- return openContentSettingsTab(page, contentSwitcherKind, dir)
+ return openContentSettingsTab(page, contentSwitcherKind)
} else if (isPressed) {
await closeContentSettingsModal(page, dir)
}
AetherUnbound
force-pushed
the
fix/filter-a11y-label
branch
from
d5123ef to
6ac04e0
Compare
| await page.getByRole("tab", { name: t(tabKey, dir) }).click() | ||
| await page.locator(`#tab-${tabId}`).click() |
There was a problem hiding this comment.
Generally it's best practice to avoid element id-based locators in tests. They don't mirror human behaviour at all, regardless of what or whether assistive technologies are in use. What's changed about the implementation that prevents selecting on the role and name any more? Is it because of the dynamic filter count? If so, we should use a regex to select based on a substring... you'll just need to copy in the RTL regex as distinct from the LTR (/filters/ for LTR, /التصفية/ for RTL, these regexes appear to work for me locally in Node) and select which to use based on the dir argument.
There was a problem hiding this comment.
Our i18n setup in Playwright tests is quite basic, and does not have support for $tc.
We do not want to use a hard-coded regex for the selector because the string might change without the changes to the key.
If we continue using the current test t function for this locator, it would return Filter ({count})|Filters ({count})" for English name of the tab. We could use some string manipulation and remove the part after the first (, but then the usefulness of this kind of selector is very questionable. Besides, for RTL, the order of words might be different, and we could be removing the part of the string that is actually useful (if the parentheses are in the beginning of the phrase).
My justification for this change was that this is a helper function used for opening the content settings modal. We could make sure that we use the role- and name- based selectors in the tests that specifically test the filters button and the way it opens the modal, and keep the id-based selector for the helper function used in other tests where this is not the behavior under test.
That said, I wonder if we really need the pluralization in the filters tab label ("Filter (1)"/"Filters (n)" distinction). Always using "Filters" would simplify the code, but would it look correct for an English speaker? "Filters (1)" and "Filters (5)"?
There was a problem hiding this comment.
If we decide not to use pluralization for Filters, then this PR is ready. Otherwise, we will also need to update the frontend/test/locales/ar.json file with test values for Arabic (Google suggests "({count}) مرشح|مرشحات ({count})"), which will also probably cause the need to update RTL snapshots
There was a problem hiding this comment.
There are a few things here:
- My understanding is that pluralization might not be necessary because it's a stable name of the tab, not a reference to the number of filters applied. It's " Filters ({count} )", in a sense? I don't know if that comes across in all languages, or even to the translator, and regardless of what we pick, we should put a translator note to explain the context for this string.
- Whether we pluralise or not is irrelevant to the question of a regex. The regex intentionally doesn't match the entire string and only the part that would be stable (and pulled from our stable testing environment documents, which are predictable). We don't need to update the testing translations to literally reflect the actual translation. The testing translations need to be plausible, and ideally somewhat reflect the length of the string in the tested language. So long as this tab is the filters tab, the search string would never change.
- Navigation utilities, perhaps most of all, should follow the best practices for navigating the app "using the tools a human would have" because they're implicitly testing that getting around the application works for a human (as close as we can get). End-to-end tests are not unit tests, they are integration tests. I don't think they should take artificial short-cuts.
There was a problem hiding this comment.
2. Whether we pluralise or not is irrelevant to the question of a regex. The regex intentionally doesn't match the entire string and only the part that would be stable (and pulled from our stable testing environment documents, which are predictable). We don't need to update the testing translations to literally reflect the actual translation. The testing translations need to be plausible, and ideally somewhat reflect the length of the string in the tested language. So long as this tab is the filters tab, the search string would never change.
This would require a hard-coded regex instead of using the i18n keys.
I feel like there are 2 best practices that are in conflict here:
- use role and name based selectors to mirror the user behavior
- use the i18n keys and
tfunction instead of hard-coding strings.
If we use the i18n key, we cannot easily create a regex that would correctly match the tab's accessible name because it would return Filter ({count})|Filters ({count})", and we need to match "Filter (1)" or "Filters (\d)". Even if we split the return value by a space, it would work for English("Filter" would match any variant), but I'm not sure it would work for Arabic (because the singular form might be very different from the plural one).
If we use the hard-coded regex, then we are going back on "not using hard-coded strings in tests".
Is there something I'm not seeing?
There was a problem hiding this comment.
No, I don't think you're missing anything. You're right, on the face of it using a regex is in conflict with avoiding hard coded text. But when avoiding hard coded text (which is a preference to make changing tests easier) conflicts with a best practice focused on ensuring the application is accessible, I have to think the accessibility preference is more important. Neither of these are absolute best practices or preferences, but certainly the one intended to ensure the accessibility of the application should be considered more important than one meant to make the development experience of test writing slightly simpler.
There was a problem hiding this comment.
I realized that we don't use a pluralized (i.e., 1 filter/2 filters) for the visible label, it's always "Filters". So, we could probably use the single "Filters ({count})" for the aria-label as well.
In light of the discussion with @sarayourfriend, I created a patch that reverts the locator changes, and updates the filter locator to fix the tests:
Subject: [PATCH] Test fixes
---
Index: frontend/test/playwright/utils/navigation.ts
<+>UTF-8
===================================================================
diff --git a/frontend/test/playwright/utils/navigation.ts b/frontend/test/playwright/utils/navigation.ts
--- a/frontend/test/playwright/utils/navigation.ts
+++ b/frontend/test/playwright/utils/navigation.ts
@@ -49,11 +49,13 @@
*/
export const openContentSettingsTab = async (
page: Page,
- tab: "searchTypes" | "filters" = "searchTypes"
+ tab: "searchTypes" | "filters" = "searchTypes",
+ dir: LanguageDirection = "ltr"
) => {
- const tabId = tab === "searchTypes" ? "content-settings" : "filters"
+ // Use a hard-coded Regex to match the dynamic Filters tab name
+ const tabName = tab === "searchTypes" ? t("searchType.heading", dir) : dir === "ltr" ? /filter/i : /مرش/
- await page.locator(`#tab-${tabId}`).click()
+ await page.getByRole("tab", { name: tabName }).click()
}
/**
@@ -104,7 +106,7 @@
await buttonLocator.isEnabled()
await buttonLocator.click()
}
- return openContentSettingsTab(page, contentSwitcherKind)
+ return openContentSettingsTab(page, contentSwitcherKind, dir)
} else if (isPressed) {
await closeContentSettingsModal(page, dir)
}
Index: frontend/test/locales/ar.json
<+>UTF-8
===================================================================
diff --git a/frontend/test/locales/ar.json b/frontend/test/locales/ar.json
--- a/frontend/test/locales/ar.json
+++ b/frontend/test/locales/ar.json
@@ -52,7 +52,7 @@
"loading": "جار التحميل...",
"filterButton": {
"simple": "مرشحات",
- "withCount": "{count} عامل التصفية | {count} عوامل التصفية"
+ "withCount": "({count}) مرشحات|({count}) مرشح"
},
"seeResults": "انظر النتائج",
"backButton": "عُد",
Co-authored-by: Olga Bulat <[email protected]>
|
Thank you @obulat for the patch! I've applied it and simplified the English translation. Let me know if anything else is needed for this! |
Co-authored-by: sarayourfriend <[email protected]>
Fixes
Fixes #957 by @obulat
Description
This PR adds a common
aria-labelto both theVFilterTabandVFilterButton(based on feedback provided in #957) which more appropriately conveys in text format what sighted users are expected to understand about the action component. The new label takes the form (in English) ofFilters ({number}).I've opted to reduce the set of changes to just this, since the testing translation changes were pretty unrelated and this makes the changeset much smaller.
Testing Instructions
just frontend/run devFilters (3)Filters (3)Checklist
Update index.md).main) or a parent feature branch.just catalog/generate-docsfor catalogPRs) or the media properties generator (
just catalog/generate-docs media-propsfor the catalog or
just api/generate-docsfor the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin