Add more robust selector handling for getByLabelText #373

airjp73 · 2019-10-06T14:00:47Z

What:

Adds more robust handling of the selector option that can be passed to the getByLabelText family of queries. Fixes #372.

Why:

More info in related issue (#372)

How:

Just some tweaking to label-text.js. It wasn't trying to find all the possible matches, and instead would short-circuit when it found something. For example, if the label had a for attribute, it wouldn't also check for elements that were aria-labelledby that label. Then I also added another step at the end where it filters down the elements it found based on the selector option.

Checklist:

Documentation added to the docs site (this PR)
I've prepared a PR for types targetting https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/testing-library__dom N/A
Tests
Ready to be merged

kentcdodds

This is looking good.

I want to make this a non-breaking change, but it really is a breaking change 🙁

kentcdodds · 2019-10-06T14:02:26Z

src/queries/label-text.js

+  const elementsMatchingSelector = new Set(container.querySelectorAll(selector))
+  return allMatches.filter(matchingElement =>
+    elementsMatchingSelector.has(matchingElement),
+  )


I think we'd be better off with matches here: https://developer.mozilla.org/en-US/docs/Web/API/Element/matches

I had a feeling this wasn't the best way. Probably could have done a little more digging haha.

kentcdodds · 2019-10-06T14:04:20Z

src/__tests__/element-queries.js

+  expect(result).toHaveLength(1)
+  expect(result[0].id).toBe('input1')
+})
+


Could you add another test for the situation I found myself in where we had two elements that had the same label text?

const {getByLabelText} = render(` <label> Test Label <input /> </label> <label> Test Label <textarea></textarea> </label> `) // select the input

Thanks!

Relates to [this PR](testing-library/dom-testing-library#373).

Passes with no changes to production code

airjp73 · 2019-10-06T14:32:52Z

This is looking good.

I want to make this a non-breaking change, but it really is a breaking change 🙁

Yeah, it totally is. Even one of the cases I pointed out in the issue

const {getByLabelText} = render(
  <label>
    <span>Username</span>
    <input />
  </label>
);

// This worked before even though the documentation said it didn't
// and now you need to provided a selector
const input = getByLabelText('Username');

matchingElement.matches sounds a little weird

kentcdodds · 2019-10-06T15:01:00Z

Wait. So to be clear, what you're saying is that with these changes I can't get the input in your example with getByText(/username/i) (without the selector)?

airjp73 · 2019-10-06T15:09:35Z

More specifically, the query will return the input and the span as matches. So if you only want the input it needs to be filtered down with the selector.

airjp73 · 2019-10-06T15:12:24Z

It's easy enough to change if that's not what you want. What is the correct behavior is in that situation? The textContent of the label could be made up of any number of elements, so I'm not sure how we would filter it by default.

Example:

const {getAllByLabelText} = render(`
  <label>
    <h1>Test</h1>
    <h2>Label</h2>
    <input id="input1" />
  </label>
`)

// Returns 3 elements [h1, h2, input]
const result = getAllByLabelText(/Test Label/i)

alexkrolick · 2019-10-06T17:36:53Z

That case with the h1/h2 should only return the input element. getByLabelText returns the labelled elements, not the label content.

airjp73 · 2019-10-06T17:49:24Z

That definitely makes sense. How should we qualify which children are the label and which are labeled? The current behavior with label children (before this change), is to do label.querySelector(selector) where selector defaults to *. So all children of a matched label are picked up.

Does one of these make sense?

Default selector to grab inputs and textareas
Special case label children to only pick up inputs and textareas
Figure out which elements, specifically, contain the text we matched on and exclude them

airjp73 · 2019-10-07T12:01:17Z

I went ahead with something like my second option. If that's not right I can totally change it 👍

So it will now pick up

Any element referenced by a matched label's for attribute
Any element that is aria-labelledby a matched label
Any input or textarea that is a child of a matched label

Then those results are filtered down based on the provided selector option.

kentcdodds · 2019-10-07T13:00:57Z

src/queries/label-text.js

      }
      if (label.childNodes.length) {
        // <label>text: <input /></label>
-        return label.querySelector(selector)
+        label
+          .querySelectorAll('input, textarea')


This is insufficient. We need to select every form control element type. Unfortunately there's not a common class that all form control elements inherit from, but I think we'd be safe if we found every element that supports the .labels property.

I'm not sure of the best way to do this, but here's a list of all the element types: https://developer.mozilla.org/en-US/docs/Web/API (Everything named something like HTML*Element).

I'll look into that and see if I can find all of them. 👍

kentcdodds

Thank you for your work on this!

airjp73 · 2019-10-07T21:29:13Z

Happy to help!

airjp73 · 2019-10-07T21:49:46Z

I think I got all of them. One thing that's a little unsatisfying is it necessitates updating the list of supported elements manually if another element is added.

kentcdodds

This looks good to me! Because this is a breaking change, I'd like to consider whether there's anything else we'd like to include in this major version bump. I don't expect it to be a difficult upgrade for people, but it would probably be a good thing to include as many breaking changes at once if there's anything else we'd like to break 🤔

airjp73 · 2019-10-07T22:09:57Z

Somewhat related to this change, I wonder if it's worth doing a quick look at the other queries to make sure the behavior of the selector option is consistent across them?

kentcdodds · 2019-10-07T22:11:14Z

I believe all the other queries are directed at the element that's actually returned, rather than taking a hop like this one does, but if you'd like to double check that'd be great :)

kentcdodds · 2019-10-07T22:12:06Z

Another thing that we should maybe consider is adding a labelSelector option which works the way the original selector worked. We wouldn't even have to document it (because I think it's unlikely people would use it), but that way people would have a migration path if they used the old selector option.

airjp73 · 2019-10-09T22:13:48Z

I'm not fully sure what the labelSelector would look like.

The main case where people will run into issues is if they have a label that does multiple of these things:

Has a for prop
Has an id prop
Has children

Before, if the label had a for prop, it would only query for that one element and never query for anything with aria-labelledby. And if the label had an id, it would query for elements that are aria-labelledby the label and never query the children of the label. Now all those conditions can be checked for the same label.

So if we added a labelSelector, would we also change that -- only query for one of those cases for any given label? Or would it just be a selector that only applies to label children and not those other cases?

Edit: If we do want it to completely go back to the old behavior, maybe we could do a boolean option instead.

airjp73 · 2019-10-09T22:17:05Z

I believe all the other queries are directed at the element that's actually returned, rather than taking a hop like this one does, but if you'd like to double check that'd be great :)

I checked the docs and it looks like ByText and ByLabelText are the only queries that have the selector option anyway so I don't think this is anything we need to worry about.

kentcdodds · 2019-10-10T02:21:41Z

Awesome. Thanks for checking. I think we're about ready to make this happen. But since it's a breaking change, I want to take advantage of that churn to make other breaking changes we've been wanting to make (#376). I'd love it if you want to weigh in :)

codesandbox-ci · 2019-12-13T15:34:39Z

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 d5ca2f8:

Sandbox	Source
compassionate-bush-nqy67	Configuration
react-testing-library demo	Issue #372

codecov · 2019-12-13T15:37:54Z

Codecov Report

Merging #373 into next will not change coverage.
The diff coverage is 100%.

@@         Coverage Diff         @@
##           next   #373   +/-   ##
===================================
  Coverage   100%   100%           
===================================
  Files        22     22           
  Lines       404    409    +5     
  Branches     95     95           
===================================
+ Hits        404    409    +5

Impacted Files	Coverage Δ
src/queries/label-text.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 6084f53...d5ca2f8. Read the comment docs.

weyert · 2019-12-23T07:41:38Z

I am having the same issue when working with Downshift I would love to switch back to getLabelByText. My current solution is to use getByRole('textbox') but that doesn't make me excited.

My question is how can I help to get this PR merged?

kentcdodds · 2020-01-21T18:46:21Z

I'm finally back from a bit of a break. I'll be taking a hard look at breaking shipping some breaking changes soon.

kentcdodds · 2020-03-03T16:19:37Z

Sorry this has taken so long. Really hoping to get to this soon. It's just been a beast to make enough time for this major version bump. And I try to avoid doing those 😅

kentcdodds

Alright, ready to merge this into next!

kentcdodds · 2020-03-04T17:37:38Z

🎉 This PR is included in version 6.16.0 🎉

The release is available on:

npm package (@next dist-tag)
GitHub release

Your semantic-release bot 📦🚀

kentcdodds · 2020-03-04T17:45:51Z

I made a little mistake with the auto-release here, but the latest is still pointing to the right version so hopefully we don't have issues with people getting 6.16.0 by mistake.

In any case, you will soon be able to test this via npm install @testing-library/dom@beta

Closes #372 BREAKING CHANGE: If you used the `selector` option in `ByLabelText` queries, then you will probably need to update that code to be able to find the label you're looking for.

airjp73 · 2020-03-23T00:15:05Z

Awesome! Glad I was able to help. :)

dreyks · 2020-04-06T14:53:04Z

hmm... I've got a bit lost on where this change was introduced, i guess it was 6.16.0 but that release is tagged as pre-release and this change is highlighted once again in v7.0.0-beta.1, where it was properly marked as BREAKING

unfortunately, that mention didn't make it to the final 7.0.0 release notes, so I suggest we add it there cc @kentcdodds

also this is breaking in one more way not mentioned in this PR: now getByLabelText finds all inputs that are inside a given label, which leads to TestingLibraryElementError: Found multiple elements with the text in cases where there actually are several inputs inside a single label (that might be a questionable HTML in the first place, but still it was working and testable with pre-6.16.0)

so this change can be breaking even for those who don't use selector

dreyks · 2020-04-06T15:00:16Z

on the other hand WHATWG Spec states

If the for attribute is not specified, but the label element has a labelable element descendant, then the first such descendant in tree order is the label element's labeled control.`
[emphasis added]

So apparently having several inputs within one label is not that against the spec and the first input should be the one returned

kentcdodds · 2020-04-06T15:20:07Z

Whoops! Sorry about that. We had a few bumps in the release and missed a few things in the notes 😬 I'll get that added asap.

airjp73 · 2020-04-06T16:27:35Z

@kentcdodds, do you want me to put in a PR to address @dreyks suggestion that it return the first input instead of all of them in the case that a label contains multiple inputs?

kentcdodds · 2020-04-06T16:31:10Z

That seems reasonable to me, yes. Thank you! I'll consider this a bug fix rather than a breaking change.

Relates to [this PR](testing-library/dom-testing-library#373).

airjp73 added 2 commits October 6, 2019 09:47

test: Add test cases for more robust label filtering

019a1c3

feat: Add more robust label filtering

2a832cd

kentcdodds reviewed Oct 6, 2019

View reviewed changes

airjp73 added a commit to airjp73/testing-library-docs that referenced this pull request Oct 6, 2019

Update documentation for ByLabelText queries

915a1cb

Relates to [this PR](testing-library/dom-testing-library#373).

airjp73 added 2 commits October 6, 2019 10:25

test: Add test case for multiple labels with the same text

7b41148

Passes with no changes to production code

Use matches instead of performing another query

581c451

airjp73 mentioned this pull request Oct 6, 2019

Update documentation for ByLabelText queries testing-library/testing-library-docs#282

Merged

Change variable name

696f309

matchingElement.matches sounds a little weird

Tweak logic for getting label children

d7a9c2a

kentcdodds reviewed Oct 7, 2019

View reviewed changes

airjp73 added 2 commits October 7, 2019 17:43

Add more form controls

ad90d4e

Just inline the string

a7ad2fb

Merge branch 'master' into 372-label-fix

dc48a12

kentcdodds approved these changes Oct 7, 2019

View reviewed changes

kentcdodds mentioned this pull request Oct 10, 2019

BREAKING: make waitForDomChange accept a callback #376

Closed

kentcdodds added the BREAKING CHANGE This change will require a major version bump label Oct 10, 2019

Merge branch 'master' into 372-label-fix

8eb8c9f

kentcdodds changed the base branch from master to next March 4, 2020 16:58

Merge branch 'next' into 372-label-fix

d5ca2f8

kentcdodds approved these changes Mar 4, 2020

View reviewed changes

kentcdodds merged commit 9fd422a into testing-library:next Mar 4, 2020

kentcdodds added the released on @next label Mar 4, 2020

wKovacs64 mentioned this pull request Mar 13, 2020

[v7] *ByLabelText finding additional elements when descendants of label #482

Closed

airjp73 mentioned this pull request Apr 6, 2020

fix: Only return first child element in label query #520

Merged

4 tasks

zikaari mentioned this pull request Jul 8, 2020

Make @testing-library/dom a peer dependency instead testing-library/angular-testing-library#122

Closed

superT999 added a commit to superT999/testing-library-docs that referenced this pull request Feb 13, 2023

Update documentation for ByLabelText queries

bf79b86

Relates to [this PR](testing-library/dom-testing-library#373).

Add more robust selector handling for getByLabelText #373

Add more robust selector handling for getByLabelText #373

Conversation

airjp73 commented Oct 6, 2019 • edited Loading

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds Oct 6, 2019

Choose a reason for hiding this comment

airjp73 Oct 6, 2019

Choose a reason for hiding this comment

kentcdodds Oct 6, 2019

Choose a reason for hiding this comment

airjp73 commented Oct 6, 2019

kentcdodds commented Oct 6, 2019

airjp73 commented Oct 6, 2019

airjp73 commented Oct 6, 2019 • edited Loading

alexkrolick commented Oct 6, 2019

airjp73 commented Oct 6, 2019 • edited Loading

airjp73 commented Oct 7, 2019

kentcdodds Oct 7, 2019

Choose a reason for hiding this comment

airjp73 Oct 7, 2019

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

airjp73 commented Oct 7, 2019

airjp73 commented Oct 7, 2019

kentcdodds left a comment

Choose a reason for hiding this comment

airjp73 commented Oct 7, 2019

kentcdodds commented Oct 7, 2019

kentcdodds commented Oct 7, 2019

airjp73 commented Oct 9, 2019 • edited Loading

airjp73 commented Oct 9, 2019

kentcdodds commented Oct 10, 2019

codesandbox-ci bot commented Dec 13, 2019 • edited Loading

codecov bot commented Dec 13, 2019 • edited Loading

Codecov Report

weyert commented Dec 23, 2019 • edited Loading

kentcdodds commented Jan 21, 2020

kentcdodds commented Mar 3, 2020

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Mar 4, 2020

kentcdodds commented Mar 4, 2020

airjp73 commented Mar 23, 2020

dreyks commented Apr 6, 2020 • edited Loading

dreyks commented Apr 6, 2020

kentcdodds commented Apr 6, 2020

airjp73 commented Apr 6, 2020

kentcdodds commented Apr 6, 2020

airjp73 commented Oct 6, 2019 •

edited

Loading

airjp73 commented Oct 6, 2019 •

edited

Loading

airjp73 commented Oct 6, 2019 •

edited

Loading

airjp73 commented Oct 9, 2019 •

edited

Loading

codesandbox-ci bot commented Dec 13, 2019 •

edited

Loading

codecov bot commented Dec 13, 2019 •

edited

Loading

weyert commented Dec 23, 2019 •

edited

Loading

dreyks commented Apr 6, 2020 •

edited

Loading