Skip to content

fix: Various implicit roles in ByRole #446

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.

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

Merged
merged 5 commits into from
Mar 4, 2020

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Feb 4, 2020

With #408 we'll be able to correctly identify some of these implicit roles (e.g. <form> or <section>)

What:

Fix various issues with implicit roles

Why:

Examples for elements with the wrong implicit role when using aria-query@3:

  • <form> without an accessible name having a role (though even with it now has is not queried by the form role. this requires feat(byRole): Add name filter #408)
  • <select> being identified as both combobox and listbox even though it is only a combobox
  • <input list="" /> not being identified as a combobox

How:

Bump aria-query to ^4.0.1

Checklist:

  • ~[ ] Documentation added to the
    docs site
  • [ ] I've prepared a PR for types targeting
    DefinitelyTyped~
  • Tests
  • Ready to be merged

@eps1lon eps1lon added the bug Something isn't working label Feb 4, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 4, 2020

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 3d38d1b:

Sandbox Source
practical-hooks-5i0vh Configuration

@codecov
Copy link

codecov bot commented Feb 4, 2020

Codecov Report

Merging #446 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #446   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          22     22           
  Lines         397    402    +5     
  Branches       94     95    +1     
=====================================
+ Hits          397    402    +5
Impacted Files Coverage Δ
src/role-helpers.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a98dc9f...3d38d1b. Read the comment docs.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thank you! Just one comment.

@kentcdodds
Copy link
Member

this requires #408

Does this mean that we should make sure we merge #408 before this PR?

@eps1lon
Copy link
Member Author

eps1lon commented Mar 3, 2020

Would be better to not have a this released without #408. Then we can add tests with named elements.

@kentcdodds
Copy link
Member

Working on getting this mergable right now.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Looking good. Just want to make sure aria-query didn't mess us up.

Comment on lines 4 to 11
"region:

Name "":
<section
data-testid="a-section"
/>

--------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

This change appears to be a result of the aria-query update. Maybe that's a bug?

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Super. Thank you!

@kentcdodds kentcdodds merged commit 4b7996c into testing-library:master Mar 4, 2020
@kentcdodds
Copy link
Member

🎉 This PR is included in version 6.14.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@eps1lon eps1lon deleted the bump/aria-query branch March 4, 2020 21:40
@nixxquality
Copy link

This was a breaking change in my case.

If anyone else is having issues with ****ByRole("form") not working anymore, add an ARIA label to your form.

   return (
-    <form onSubmit={ onSubmit }>
+    <form onSubmit={ onSubmit } aria-label="Configuration">
       <div id="floatsubmit">

@smeijer
Copy link
Member

smeijer commented Jul 8, 2020

Is that form bug something for which an issue should be created?

@kentcdodds
Copy link
Member

I don't think an aria label should be required on the form. This seems like a bug.

@nixxquality
Copy link

It was an upstream change in aria-query, and it follows the w3c spec.
The only problem I had with it was that it was a breaking change in aria-change (4.0) but it was a minor change in this project, sending me on a bit of a journey to find out why my code suddenly wasn't working (because it wasn't correct in the first place, mind you).

@kentcdodds
Copy link
Member

Oh, that's very interesting. So a label is required on a form according to the spec?

@nickserv
Copy link
Member

nickserv commented Jul 8, 2020

Should we bump the major version? The semver spec recommends doing that retroactively after it’s discovered that a breaking change has been already published accidentally.

@smeijer
Copy link
Member

smeijer commented Jul 9, 2020

Oh, that's very interesting. So a label is required on a form according to the spec?

It does look that way:

role=form if the form element has an accessible name. Otherwise, no corresponding role.
--

It still feels very weird to me. Other elements (like heading, article) get an implicit role, but form does not? Both Chrome and Firefox show me a role=form for forms without accessible names. Are they messing up here?

On a positive note, the label seems to work nicely.

And maybe that's the entire idea behind it. A heading and article will make sense due to their content. But selecting a form without a label, might not. Think of invoice address and shipping address. Without proper labels, it's impossible to tell which "input: address" has the focus.

<form aria-labelledby="signup-form">
  <h1 id="signup-form">
    Signup Form
  </h1>
  
  <input placeholder="email" />
</form>
screen.getByRole('form', { name: /signup form/i })

playground

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants