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

Improve accessibility labels for filters tab and button #4396

Merged
merged 8 commits into from
May 30, 2024
Merged

Improve accessibility labels for filters tab and button #4396

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3265d2a
Add aria-label to VFilterTab
AetherUnbound May 27, 2024
972ba83
Update filter's aria-label within translations
AetherUnbound May 28, 2024
bbedbed
Simplify VFilterTab by using $tc filter
AetherUnbound May 28, 2024
6ac04e0
Fix tests
AetherUnbound May 28, 2024
41770f3
Use a more stable locator for testing
AetherUnbound May 29, 2024
32a834c
Use the plural translation for all cases
AetherUnbound May 29, 2024
bd4caeb
Update frontend/test/locales/ar.json
obulat May 30, 2024
647ec29
Add a translator's note to the base translation file
AetherUnbound May 30, 2024
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
7 changes: 6 additions & 1 deletion frontend/src/components/VHeader/VHeaderMobile/VFilterTab.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
<template>
<VTab id="filters" size="medium" class="gap-x-2">
<VTab
id="filters"
size="medium"
class="gap-x-2"
:aria-label="$tc('header.filterButton.withCount', appliedFilterCount)"
>
<VFilterIconOrCounter :applied-filter-count="appliedFilterCount" />
<h2 class="label-regular">{{ $t("filters.title") }}</h2>
</VTab>
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/locales/scripts/en.json5
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
loading: "Loading...",
filterButton: {
simple: "Filters",
withCount: "{count} Filter|{count} Filters",
withCount: "Filter ({count})|Filters ({count})",
},
seeResults: "See results",
backButton: "Go back",
Expand Down
9 changes: 4 additions & 5 deletions frontend/test/playwright/utils/navigation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,11 @@ export const searchTypeNames = {
*/
export const openContentSettingsTab = async (
page: Page,
tab: "searchTypes" | "filters" = "searchTypes",
dir: LanguageDirection = "ltr"
tab: "searchTypes" | "filters" = "searchTypes"
) => {
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()
Copy link
Collaborator

@sarayourfriend sarayourfriend May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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)"?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@sarayourfriend sarayourfriend May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There are a few things here:

  1. 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.
  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.
  3. 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 t function 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?

Copy link
Collaborator

@sarayourfriend sarayourfriend May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}

/**
Expand Down Expand Up @@ -105,7 +104,7 @@ export const setContentSwitcherState = async (
await buttonLocator.isEnabled()
await buttonLocator.click()
}
return openContentSettingsTab(page, contentSwitcherKind, dir)
return openContentSettingsTab(page, contentSwitcherKind)
} else if (isPressed) {
await closeContentSettingsModal(page, dir)
}
Expand Down