get by label concat values #681

delca85 · 2020-07-03T13:09:55Z

What:

input elements with not empty value are retrieved by getAllLabelsByText
getByLabelText gets labels even when it receives a label composed of a concatenation

Why:

Issue #545 finds out that input elements are not recognized as labels and labelling by concatenation is not got by getByLabelText.

How:

queryAllLabelsByText now makes a query not only on elements of type
label but on input elements too. The empty input are excluded.
after evaluating labels node through matcher, the labels that doesn't match are combined through an helper function. If a combination matches with the received text than the labels is returned by queryAllLabelsByText.

Checklist:

make queryAllLabelsByText look for input elements too
create a queryAllLabels function that retrieves all labels without any regards about text to be matched
find labels matching
create combination function that concatenate all the labels and returns the node that through some combination matches with the received text
queryAllLabelsByText returns now the labels matching by itself and the ones matching through concantenation

Documentation added to the
docs site
Tests
Typescript definitions updated
Ready to be merged

I have already opened #607 to fix this behavior but maybe not in the right way. I hope this idea is better than the one I have submitted before.

…#545)

codesandbox-ci · 2020-07-03T13:10:48Z

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 49c6148:

Sandbox	Source
kentcdodds/react-testing-library-examples	Configuration

codecov · 2020-07-03T13:14:49Z

Codecov Report

Merging #681 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #681   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines          603       630   +27     
  Branches       151       163   +12     
=========================================
+ Hits           603       630   +27

Impacted Files	Coverage Δ
src/queries/label-text.js	`100.00% <100.00%> (ø)`
src/queries/role.js	`100.00% <0.00%> (ø)`
src/role-helpers.js	`100.00% <0.00%> (ø)`

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 9ae9244...49c6148. Read the comment docs.

kentcdodds

This one is also tricky. I have some feedback and questions/ideas.

kentcdodds · 2020-07-03T13:26:11Z

src/queries/label-text.js

+  const combs = [[]]
+  const matching = []
+  for (const label of labels) {
+    const copy = [...combs] // See note below.


I'm not sure what note this is referring to. Also, if I'm following this logic correctly, the for loop on the next line will only be run only once (for this iteration of the nested loop) and the value of the prefix will be [] every time. Any reason we need a loop here?

I am not sure I am totally getting your point, are you telling that this loop runs only once inside the outer loop?

Correct. Because copy is [[]] and so it's an array with only one element in it. It'll never be....

Wait. I guess that's only the case on the first iteration of the outer loop. Future iterations could have more... Hmmm... I need to take more time to understand what's going on here...

Yes, I think it's only the first iteration of the outer loop that makes the inner one run only once.

kentcdodds · 2020-07-03T13:27:43Z

src/queries/label-text.js

+  return matching
+}
+function queryAllLabels(container) {
+  return Array.from(container.querySelectorAll('label,input'))


It occurs to me that an element can be labeled by anything, not just label and input. So while this may be solving for the input case, it doesn't solve for everything else 🤔

That said, I'm not sure how to handle "everything else." It seems to me that rather than searching for all possible elements that can label anything, we should be searching for everything that's labeled first, determine the contents of their label, and then return the elements whose label matches the query.

For what it's worth, I think that searching for labelled items and then for their labels is more feasible than querying for all elements that could be labels. Like you said above!

If you'd like to tack a whack at that, I think it would solve a lot of problems.

For sure I would like to do it!

kentcdodds · 2020-07-03T13:31:11Z

src/queries/label-text.js

+  const labelsNotMatchingTextAndNotEmpty = textToMatchByLabels.filter(
+    ({node, textToMatch}) =>
+      Boolean(textToMatch) &&
+      nodesByLabelMatchingText.findIndex(nodeByLabelByText =>
+        nodeByLabelByText.isEqualNode(node),
+      ) === -1,
+  )


Suggested change

const labelsNotMatchingTextAndNotEmpty = textToMatchByLabels.filter(

({node, textToMatch}) =>

Boolean(textToMatch) &&

nodesByLabelMatchingText.findIndex(nodeByLabelByText =>

nodeByLabelByText.isEqualNode(node),

) === -1,

)

const labelsNotMatchingTextAndNotEmpty = textToMatchByLabels.filter(

({node, textToMatch}) =>

textToMatch && nodesByLabelMatchingText.some(n => n.isEqualNode(node)),

)

I think the right change should be:

const labelsNotMatchingTextAndNotEmpty = textToMatchByLabels.filter( ({node, textToMatch}) => textToMatch && !nodesByLabelMatchingText.some(n => n.isEqualNode(node)), )

because I would like to retrieve all the labels in textToMatchByLabels but not empty and not included in nodesByLabelMatchingText

…check their label value (testing-library#545)

delca85 · 2020-07-04T21:30:54Z

Hey @kentcdodds !
I have pushed some changes that follow the suggestion you gave me yesterday.

Now queryAllByLabelText at the beginning looks for labelled element and then checks their labels value.
I still makes a research of all the label elements to get all the elements labelled but without aria-labelledby and aria-label attribute.

Right now I am still using the "old" queryAllLabelsByText in order to be able to send a specific error message in case of the searched label is found but none form control is associated to it. I think this is not totally right because that queryAllLabelsByText is looking just for label and input element not everything while everything could be a label (as you told).

I suppose my job is not finished yet but this is what I achieved.

kentcdodds

Hey @delca85, thanks for this. This isn't quite doing what I thought (getting all labelable elements first, then finding their labels). I'm going to need more time to review/provide feedback on this. Just commenting here to let you know I haven't forgotten.

kentcdodds · 2020-07-05T14:01:44Z

src/__tests__/element-queries.js

+  <label>
+    All alone but with children
+    <textarea>Hello</textarea>
+    <select><option value="0">zero</option></select>
+  </label>`)


This should find textarea. And it does so when using label.control

Maybe I am not understanding, you are saying that the right test to be done here (at line 374) was that the labeled element is textarea instead of that no input element labeled was found?

getByLabelText(/alone/) would have found the textarea element, I added the selector to make it return null.

Please tell me if I am missing something.

kentcdodds · 2020-07-14T21:37:59Z

I apologize it's taken me so long to give feedback here. I've been doing more family stuff than open source stuff recently.

I was thinking about a much bigger reworking of the logic for this function.

Right now, here's what we try to do:

Find all matching labels and then find all the elements those labels are used to label
Find all other matching elements and see if they're also used to label anything

The #545 should be found via the #2 option there, but it's difficult because the search query matches across multiple labels, so we won't find it without some additional complex logic.

What I was thinking is we could do this instead:

Find all labeled elements (everything with a .label property or an aria-labeledby attribute)
Filter those elements to the ones whose labels match the search text

I think this would allow us to more easily solve #545 and would result in fewer bugs. I expect that this would be a non-breaking change.

Does that make sense?

delca85 · 2020-07-15T13:36:17Z

I apologize it's taken me so long to give feedback here. I've been doing more family stuff than open source stuff recently.

Don't be sorry at all, family stuff is always first class stuff!

Right now, here's what we try to do:

Find all matching labels and then find all the elements those labels are used to label

Find all other matching elements and see if they're also used to label anything

The #545 should be found via the #2 option there, but it's difficult because the search query matches across multiple labels, so we won't find it without some additional complex logic.

Totally agree with you, specially regarding the additional complex logic required in order to solve #545 .

What I was thinking is we could do this instead:

Find all labeled elements (everything with a .label property or an aria-labeledby attribute)

Filter those elements to the ones whose labels match the search text

I think this would allow us to more easily solve #545 and would result in fewer bugs. I expect that this would be a non-breaking change.

Does that make sense?

It makes a lot of sense and it is what I have tried to do in the code I pushed here :

at this line I make a query to get all the elements with aria-labelledby and then I look for their value to check if it matches with the desired one;
at this line I retrieve the element with aria-label attribute and check if their value match with the desired one
finally I get all the label element because I thought that some labels still be missed by the first two sets retrieved by the steps 1 and 2.

In comparison to what you wrote, I check the value of the elements I found with a label immediately after I found them, because I thought it is more performant (I save a loop doing this instead of checking the value of all the labeled elements after I found all of them).

Like I told you here I am still using queryAllLabelsByText and this is the reason (and an issue related):

Right now I am still using the "old" queryAllLabelsByText in order to be able to send a specific error message in case of the searched label is found but none form control is associated to it. I think this is not totally right because that queryAllLabelsByText is looking just for label and input element not everything while everything could be a label (as you told).

I hope I explained what I did in a clear way, if I am still miss something please tell me.
Anyway, thanks for your time.

kentcdodds

Ah, that makes sense. Sorry for my confusion! I had an idea. What do you think?

kentcdodds · 2020-07-15T14:55:42Z

src/queries/label-text.js

-          container.querySelector(`[id="${label.getAttribute('for')}"]`),
+
+  const matchingLabelledElements = Array.from(container.querySelectorAll('*'))
+    .filter(element => element.hasAttribute('aria-labelledby'))


I think we might be able to clean up some stuff (and reduce the number of loops) by changing this filter to include elements with the .label property:

Suggested change

.filter(element => element.hasAttribute('aria-labelledby'))

.filter(element => element.label || element.hasAttribute('aria-labelledby'))

This would require the reducer function handle the .label elements a little differently, but I think it would allow us to remove some other code as well (specifically all the code for matchingElementsByLabels).

I am going to try what you suggested as soon as possible.

Hey @kentcdodds !
For sure I am missing something but so far I have not been able to make the change you suggested me.
Trying to include the elements with the .label property in the filter you highlighted it is not working because the element labeled by a label element put around them have the .label property undefined.
I have tried to show it in this sandbox.

Again, I am aware there is something I am not doing in the right way but I am not able to get it.

Oh, sorry, that's a typo. It should be .labels. This is a NodeList of all the labels. If there are multiple, then it looks like the Chrome Accessibility DevTools combines them:

https://jsbin.com/pudalunaxu/1/edit?html,output

<div> <label for="my-input">One</label> <label for="my-input">Two</label> <input id="my-input" /> </div>

So I think that's probably what we should do as well. Combine the text for all labels in the NodeList, then compare that against the given TextMatch.

Does that make sense?

Yes, it makes a lot of sense.
I have just pushed some code using .labels.

I am not so sure I have done all the work because I am not obtaining all the labels in the NodeList through a permutation but just join their values (and join values in the NodeList removing an item a time). I don't know if you meant this with combine.

…instead of looking for label elements (testing-library#545)

kentcdodds

This is looking pretty great! Just one question.

kentcdodds · 2020-07-16T22:51:44Z

src/queries/label-text.js

+        ? labelledElement.getAttribute('aria-labelledby').split(' ')
+        : []
+      const labelsValue = labelsId.length
+        ? labelsId.map(labelId => {


What happens if an element had both a .labels property and the aria-labelledby? I don't know if that's valid or not. Is it?

aria-labelledby has precedence: https://www.w3.org/TR/accname-1.1/#step2B

2B is for aria-labelledby, 2D for

In that case, I think this PR is good to go! Anyone have other thoughts?

kentcdodds

This is super work @delca85! Thank you for your patience with me and all my feedback. You did a superb job 👏

kentcdodds · 2020-07-20T19:33:47Z

@all-contributors please add @delca85 for tests and code

allcontributors · 2020-07-20T19:33:55Z

@kentcdodds

I've put up a pull request to add @delca85! 🎉

kentcdodds · 2020-07-20T19:34:53Z

Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the other/MAINTAINING.md and CONTRIBUTING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

kentcdodds · 2020-07-20T19:42:41Z

🎉 This PR is included in version 7.21.2 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

delca85 · 2020-07-20T19:47:41Z

Thank you so much @kentcdodds !
I have learnt a lot working on this PR and it's totally a pleasure being part of this team.
Thanks!

smeijer · 2020-07-21T06:51:17Z

Thank you @delca85. This is PR is really awesome!

delca85 · 2020-07-21T12:54:06Z

Thank you @delca85. This is PR is really awesome!

Thanks @smeijer , I really hope you are right!

Bianca Del Carretto added 3 commits July 3, 2020 14:48

add input node as label (testing-library#545)

4033c6e

create an helper function to retrieve all the labels (testing-library…

b609b32

…#545)

get concat label through labels combination (testing-library#545)

732a8ab

kentcdodds reviewed Jul 3, 2020

View reviewed changes

Bianca Del Carretto added 2 commits July 3, 2020 22:11

remove useless comment and refactor function (testing-library#545)

eb8517d

make queryAllByLabelText looks before for labelled elements and then …

f3a3c65

…check their label value (testing-library#545)

Bianca Del Carretto added 3 commits July 5, 2020 00:00

fix wrong splice usage (testing-library#545)

df29ead

remove useless code (testing-library#545)

de06780

improve test coverage (testing-library#545)

9d2be9f

kentcdodds reviewed Jul 8, 2020

View reviewed changes

kentcdodds reviewed Jul 15, 2020

View reviewed changes

retrieve elements with labels together the ones with aria-labelledby …

49c6148

…instead of looking for label elements (testing-library#545)

kentcdodds reviewed Jul 16, 2020

View reviewed changes

kentcdodds approved these changes Jul 20, 2020

View reviewed changes

kentcdodds merged commit 53e0d18 into testing-library:master Jul 20, 2020

allcontributors bot mentioned this pull request Jul 20, 2020

docs: add delca85 as a contributor #706

Merged

kentcdodds added the released label Jul 20, 2020

delca85 mentioned this pull request Jul 21, 2020

fix aria-labelledby with not found id #711

Merged

4 tasks

pahan35 mentioned this pull request Jul 22, 2020

TextField sets aria-labelledby in the wrong element rmwc/rmwc#658

Closed

	.filter(element => element.hasAttribute('aria-labelledby'))
	.filter(element => element.label \|\| element.hasAttribute('aria-labelledby'))

get by label concat values #681

get by label concat values #681

Uh oh!

Conversation

delca85 commented Jul 3, 2020

Uh oh!

codesandbox-ci bot commented Jul 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kentcdodds left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delca85 commented Jul 4, 2020

Uh oh!

kentcdodds left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kentcdodds commented Jul 14, 2020

Uh oh!

delca85 commented Jul 15, 2020

Uh oh!

kentcdodds left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kentcdodds left a comment

Choose a reason for hiding this comment

Uh oh!

kentcdodds Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kentcdodds left a comment

Choose a reason for hiding this comment

Uh oh!

kentcdodds commented Jul 20, 2020

Uh oh!

allcontributors bot commented Jul 20, 2020

Uh oh!

codesandbox-ci bot commented Jul 3, 2020 •

edited

Loading

codecov bot commented Jul 3, 2020 •

edited

Loading

kentcdodds Jul 16, 2020 •

edited

Loading