search

testing-library / dom-testing-library

🐙 Simple and complete DOM testing utilities that encourage good testing practices.
https://testing-library.com/dom
MIT License
3.25k stars 463 forks source link

feat: upgrade aria-query to 5.3.0 #1241

Closed jlp-craigmorten closed 10 months ago

jlp-craigmorten commented 1 year ago

What: Upgrade aria-query to version 5.3.0.

Why: Receive latest specification updates for role / element mappings to align with WAI ARIA 1.2.

Fixes #1100 Fixes #1150 Fixes #1201 Fixes #1234 Fixes #1239

How: Upgrading version in package.json. Updating tests to mirror changes.

Checklist:

There are a couple changes of note as a result of this upgrade, namely:

There are several other role / element mapping changes as a result of this change to align with specifications.

Given the number of role additions, removals, and modifications (including changes to role and/or changes to the constraints for a particular role) this should be considered a breaking change.

codesandbox-ci[bot] commented 1 year ago

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 69d966a84b5a67c3123e9d2335b084f595fa02f9:

Sandbox Source
react-testing-library-examples Configuration
fail to find role code Issue #1100
react-testing-library demo Issue #1100
react-testing-library demo (forked) Issue #1201
react-testing-library demo (forked) Issue #1234
react-testing-library demo Issue #1234
codecov[bot] commented 1 year ago

Codecov Report

Merging #1241 (69d966a) into alpha (452097b) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             alpha     #1241   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines         1038      1041    +3     
  Branches       349       349           
=========================================
+ Hits          1038      1041    +3
Flag Coverage Δ
node-18 100.00% <100.00%> (ø)
node-20 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/role-helpers.js 100.00% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

MatanBobi commented 1 year ago

Hi @jlp-craigmorten! Thanks for taking the time to create this and the corresponding fix in aria-query, this is extremely appreciated. I'll try to look into the changes later on today, in the meantime, I pushed a change to fix the CI issues we had.

cmorten commented 1 year ago

Hi @jlp-craigmorten! Thanks for taking the time to create this and the corresponding fix in aria-query, this is extremely appreciated. I'll try to look into the changes later on today, in the meantime, I pushed a change to fix the CI issues we had.

Appreciate it - I’ve just raised https://github.com/testing-library/dom-testing-library/issues/1242 r.e. the CI issue

nickmccurdy commented 1 year ago

My understanding of SemVer is that we only need to publish breaking changes in a major release if we upgrade to a version of a spec that's no longer back compatible with a version that we've documented supporting.

Here are the specification versions we link to, organized by our APIs:

API Specification
ByRole option description Accessible Name and Description Computation 1.1
ByRole API Accessible Rich Internet Applications 1.2
ByRole option description Accessible Rich Internet Applications 1.1
isInaccessible Accessible Rich Internet Applications 1.2
MatanBobi commented 1 year ago

My understanding of SemVer is that we only need to publish breaking changes in a major release if we upgrade to a version of a spec that's no longer back compatible with a version that we've documented supporting.

Here are the specification versions we link to, organized by our APIs:

API Specification
ByRole option description Accessible Name and Description Computation 1.1
ByRole API Accessible Rich Internet Applications 1.2
ByRole option description Accessible Rich Internet Applications 1.1
isInaccessible Accessible Rich Internet Applications 1.2

Thanks @nickmccurdy! Where were these documented? I'm not sure they are correct. The thing is, we have some role definition that have changed in 1.2, so people might look for role="img" for example and not get the elements they used to get.

jlp-craigmorten commented 11 months ago

This is now blocked behind #1242

(as is any other changes to this repo)

MatanBobi commented 11 months ago

I agree with @timdeschryver about this being a breaking change that should cause a major version release. If that's the path we're going with, we can merge these two together (this one and #1242) and create a major release. That way we'll be sure we're not breaking something with the op_mobile change and with this one. What do you all think?

timdeschryver commented 11 months ago

@MatanBobi that would mean to drop support for Node v14, right? I like that, because also end of life.

MatanBobi commented 11 months ago

@MatanBobi that would mean to drop support for Node v14, right?

I like that, because also end of life.

Yes, definitely. In this major we'll also drop support for node 14 with these changes.

@eps1lon do you have any concerns?

MatanBobi commented 10 months ago

Hi @jlp-craigmorten :) Since we're creating a new major version (due to the fact we're dropping support for node 14 and 16), we'll probably get this into alpha version first. We're trying to gather a list of the changes for the change log, do you happen to know if there's a full list somewhere?

Thanks again for the effort you've put into this one!

nickmccurdy commented 10 months ago

My understanding of SemVer is that we only need to publish breaking changes in a major release if we upgrade to a version of a spec that's no longer back compatible with a version that we've documented supporting.

Here are the specification versions we link to, organized by our APIs:

API Specification
ByRole option description Accessible Name and Description Computation 1.1
ByRole API Accessible Rich Internet Applications 1.2
ByRole option description Accessible Rich Internet Applications 1.1
isInaccessible Accessible Rich Internet Applications 1.2

Thanks @nickmccurdy! Where were these documented? I'm not sure they are correct. The thing is, we have some role definition that have changed in 1.2, so people might look for role="img" for example and not get the elements they used to get.

I was referring to the docs, I've added links above to give more context.

That being said, it seems like we've already decided on a major version, which I support.

nickmccurdy commented 10 months ago

If you merge in from alpha CI should be fixed by #1255.

MatanBobi commented 10 months ago

I was referring to the docs, I've added links above to give more context.

Thanks @nickmccurdy, we'll probably need to update the places where we direct to 1.1 as part of the major release :)

jlp-craigmorten commented 10 months ago

Hi @jlp-craigmorten :) Since we're creating a new major version (due to the fact we're dropping support for node 14 and 16), we'll probably get this into alpha version first. We're trying to gather a list of the changes for the change log, do you happen to know if there's a full list somewhere?

Thanks again for the effort you've put into this one!

Just done a scan through the aria-query changeset from 5.1.3 to 5.3.0 and believe these are the pertinent changes:

Feel free to condense / summarise as feel is best!

There is one other file change in this PR in addition to the changeset from upgrade aria-query which is a fix, but afaik is somewhat an implementation detail to enable a couple of the above points, namely the likes of "fix: img element with an empty alt attribute now has an implicit presentation role", so probably not needing it's own point.

For the footer and header element roles, the "scoped to body element" constraint isn't honoured by dom-testing-library, so may be confusing to include in the description... not sure off top of my head whether this lib would say the implicit role is generic or banner (for header elements) / contentinfo (for footer elements)

Update: so for the header element it is banner (and there was a test already demonstrating this), and for the footer it is contentinfo (and have just pushed a commit to eliminate this ambiguity). Might be worth not including the generic role points for these elements as that isn't something this lib supports.

nickmccurdy commented 10 months ago

I don't seem to be getting any roles for summary. Shouldn't we at least have button?

cmorten commented 10 months ago

I don't seem to be getting any roles for summary. Shouldn't we at least have button?

See #1252

aria-query follows the HTML ARIA 1.2 spec which states there is no corresponding role for the <summary> element.

There is a note to say that many UAs / ATs have opted to use the button role, but this isn’t formalised - it appears to be an active agenda item on HTML and AAM working groups to resolve.

To inherit a role in DTL would require a change to aria-query so issue / PR would be needed there.

MatanBobi commented 10 months ago

Since we're following the spec with this one, I think it's safe to at least merge to alpha so I'm merging this one :)