podman-desktop icon indicating copy to clipboard operation
podman-desktop copied to clipboard

chore(test): implement aria-label for dropdown button and update UI tests

Open amisskii opened this issue 1 year ago • 2 comments

Signed-off-by: Anton Misskii [email protected]

What does this PR do?

implements aria-label for dropdown button and updates UI tests

What issues does this PR fix or reference?

https://github.com/containers/podman-desktop/issues/9532

How to test this PR?

pnpm test:ui

  • [ ] Tests are covering the bug fix or the new feature

amisskii avatar Oct 22 '24 11:10 amisskii

After reading the comments, it seems that we're not focusing on the right pattern like where to put a name for a button.

IMHO the expectation is to have this component following the combobox semantic while it is not

I should not search for a button but for a combobox using aria-label

so the role 'combobox' should be added (and all the others like aria-expanded, etc)

then for the option, it's the same for the options, I don't see any option role

@deboer-tim why this component has no combobox/option aria-labels ? AFAIK it should be a drop-in replacement from tests framework from Select/Options to Dropdown (tests should not be changed)

benoitf avatar Oct 23 '24 09:10 benoitf

After reading the comments, it seems that we're not focusing on the right pattern like where to put a name for a button.

IMHO the expectation is to have this component following the combobox semantic while it is not

I should not search for a button but for a combobox using aria-label

so the role 'combobox' should be added (and all the others like aria-expanded, etc)

Yes, there needs to be a role="combobox" on the same element as aria-label. This will allow code like screen.getByRole('combobox', { name: 'x' }) to work, but not methods userEvent.selectOptions(), which requires at least a couple other aria-labels.

then for the option, it's the same for the options, I don't see any option role

@deboer-tim why this component has no combobox/option aria-labels ? AFAIK it should be a drop-in replacement from tests framework from Select/Options to Dropdown (tests should not be changed)

I had a commit that added all the labels as per an example on MDN, but vitest still failed. I went down the rabbit hole of starting to look at vitest source to see if it was actually hardcoded to select, and thought I should look at how UI libraries do it instead. I tried I couple that I had looked at prior to building this - and they literally had no aria support.

The first 10 instances I switched didn't use aria-labels at all and both unit and e2e tests passed, so I assumed it was a problem solvable later...

Short term, yes, let's do this and I'll be trying again for full aria. Long term, we should probably make sure that all locators are part of PR check?

deboer-tim avatar Oct 23 '24 12:10 deboer-tim