Querying aria role superclass #675
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit bd1c6d9:
|
Codecov Report
@@ Coverage Diff @@
## master #675 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 24 24
Lines 596 610 +14
Branches 148 151 +3
=========================================
+ Hits 596 610 +14
Continue to review full report at Codecov.
|
| @@ -63,12 +63,17 @@ test('`selected: true` matches `aria-selected="true"` on supported roles', () => | |||
| ).toEqual(['selected-native-columnheader', 'selected-columnheader']) | |||
|
|
|||
| expect(getAllByRole('gridcell', {selected: true}).map(({id}) => id)).toEqual([ | |||
| 'selected-rowheader', | |||
There was a problem hiding this comment.
Appreciate the work but I'm not convinced this will actually be an improvement and not cause more confusion. I'd get behind including subclasses for abstract roles (not really though) but for concrete roles it's a bit confusing that this role would also be considered a gridcell. Because it actually isn't.
In it's current form this will also be a breaking change. This is evident from the changes in existing tests.
There was a problem hiding this comment.
I understand your point.
The *byRole queries could get only the abstract super classes if it sounds more reasonable. Otherwise an additional parameter could be added to decide if the hierarchies should be navigated or not.
There was a problem hiding this comment.
Wouldn't this change make testing library behave more like a real browser would? I don't consider bug fixes to be breaking changes. Either way I think it's worth it as long as this models browser behavior effectively.
There was a problem hiding this comment.
Wouldn't this change make testing library behave more like a real browser would?
I'm not aware of any behavior browsers are adding for aria roles. What do you think would getByRole('gridcell') accomplish if it would return <th> or getByRole('input') for <input type="search" />?
There was a problem hiding this comment.
As far as I understand, this part of the WAI ARIA spec requires that subclasses implement properties of superclasses. https://www.w3.org/WAI/PF/aria/roles#inheritedattributes
In that case it seems to me like these changes are important for matching browser behavior. Additionally, not supporting this would violate the Liskov Substitution Principle (replacing a type with a subclass type needs to still retain the interfaces of the original superclass type).
However one potentially issue I'm seeing is that a user may want to call getByRole when there's already another element of that role with a subclass, so the new behavior could now have multiple ambiguous results. This could be a confusing change in behavior of the tests, but as far as I understand is not actually a semver breaking change because it is a bug fix. Is there a better way to handle this situation? Perhaps we could warn users about this behavior in a minor release (FWIW this is consistent with how the React team has handled similar behavioral changes). Alternatively we could treat this as more of a getByAttribute and add support for attributes other than role while making it more clear that subclasses are intentionally not supported because the functionality is meant to represent literal attributes, not inherited rules.
There was a problem hiding this comment.
I think the other concerns are mostly related to backwards compatibility. Abstract roles have no impact on that. If only abstract roles are included, then it seems like it should be easy to include while providing 90+% of what users actually want out of this.
I'm going to need to see a real use case.
A real use case for testing? The above examples are actual tests I tried to write for my application, so that is a real use case.
The application is dynamically generated from a schema, so validating that the inputs appear in the expected order is an important detail (while also saving me writing several different expect()s, one for each input).
There was a problem hiding this comment.
The above examples are actual tests I tried to write for my application, so that is a real use case.
e.g. Here are exactly the same kind of tests on a table when viewing the data (rather than editing in a form):
https://github.com/aio-libs/aiohttp-admin/blob/ee9498171d0f608574c5c80fb7dc80c0dac51af9/admin-js/tests/simple.test.js#L19
There was a problem hiding this comment.
I'm open to looking at a new PR which handles abstract roles.
Can you show me how your test would change with this new feature?
There was a problem hiding this comment.
Can you show me how your test would change with this new feature?
The first test examples above, which operate on inputs of a form, would work. Today they don't (the last linked test is for the view mode, where data is in a table). My in-progress tests locally currently look like:
expect(within(edit).getByLabelText("Id *")).toHaveValue(1);
expect(within(edit).getByLabelText("Num *")).toHaveValue(5);
expect(within(edit).getByLabelText("Optional Num")).toHaveValue(null);
expect(within(edit).getByLabelText("Value *")).toHaveValue("first");
Which fails to test ordering while being very verbose. This would become:
expect(within(edit).getAllByRole("input").map((e) => e.value)).toEqual([1, 5, null, "first"]);
Probably with a second line to assert labels as mentioned above.
There was a problem hiding this comment.
That makes sense.
I don't actively maintain this project, but I think a PR would be the next step.
delca85
changed the title
|
I've updated the docs in testing-library/testing-library-docs#520 as an alternative to this, and it's been merged. Do we want to close this? |
|
Yeah, as much as it pains me to close @delca85's PR, I think it's better this way (so sorry Bianca!! we'll get a PR from you merged in here eventually I swear). |
What:
*ByRolequeries are now able to retrieve roles according to super class roles hierarchies.Why:
According to #448 is a needed enhancement.
How:
queryAllByRoleChecklist:
queryAllByRoledocs site