Skip to content

getByLabelText doesn't concat multipart aria-labelledby #545

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

Closed
sheesania opened this issue May 5, 2020 · 18 comments
Closed

getByLabelText doesn't concat multipart aria-labelledby #545

sheesania opened this issue May 5, 2020 · 18 comments
Labels
bug Something isn't working help wanted Extra attention is needed released

Comments

@sheesania
Copy link

  • React Testing Library version: 9.5.0
  • node version: 12.4.0
  • npm (or yarn) version: 6.13.6

What you did:

Along the lines of this MDN article, I have an input labelled by a combination of three different elements: a label, a text field, and another label. So the input has an aria-labelledby attribute with a value like label1 text-field label2. In my test, I'm trying to grab the input based on that full, concatenated label, with something like getByLabelText('text of label1 value of text-field text of label2').

What happened:

I can successfully grab the input with getByLabelText('text of label1') or getByLabelText('text of label2'), but giving it the full, concatenated label getByLabelText('text of label1 value of text-field text of label2') results in nothing being found. Even if I removed the text field ID from aria-labelledby, I still can't get the input using getByLabelText('text of label1 text of label2').

Reproduction:

Here's a codesandbox: https://codesandbox.io/s/rtl-aria-labelledby-multipart-vpjpx?from-embed=&file=/src/__tests__/index.js

Problem description:

Based on my understanding of the MDN article, the "label" for an input with a multiple IDs in aria-labelledby should be a concatenation of the values of the elements with those IDs. So I expected getByLabelText to retrieve the input based on this concatenated label (especially because it can handle aria-labelledby attributes with just one ID).

Suggested solution:

From looking at label-text.js, it seems like there are a couple things going on...

  • getByLabelText only looks at <label> elements, so that <input> that's part of my aria-labelledby doesn't get picked up at all. (However, even when I removed the input's ID from aria-labelledby, I still had the concatenation problem, so this is kind of a separate issue.)
  • getByLabelText does a sweep for individual <label> elements matching the given text (before trying to figure out what input elements they're labeling). This means it doesn't catch combinations of <label> elements (or any other kind of element) that match the given text when concatenated.

So on the surface, fixing my problem would require significantly switching around how getByLabelText works so that it 1) looks at elements beyond <label>s and 2) considers combinations of element values.

However, the documentation explicitly says getByLabelText first looks for labels and then associated elements, and it also says something semi-cryptic about how you should use getByRole if you want to be "robust against switching to aria-label or aria-labelledby". So I'm not sure if my "problem" is actually just the intended behavior.

If it is the intended behavior, then it would be great to clarify that in the documentation (and I'd be happy to submit a pull request for that!).

(I should also clarify that I'm happily using getByRole('checkbox', {name: 'allow up to 0 words in between'}) for my specific use case right now per this Stack Overflow post. I just wanted to submit this issue in case this is a bug that should be fixed!)

@kentcdodds
Copy link
Member

Huh! That's very interesting. I'd never considered that use case before but it makes sense.

Our general recommendation is to use getByRole with name these days, but I think this should be supported by getByLabelText as well so I'd consider this a bug. I think it's definitely a fixable issue (though plenty of work as you noted). Would you be interested in working on that?

@kentcdodds kentcdodds added bug Something isn't working help wanted Extra attention is needed labels May 5, 2020
@sheesania
Copy link
Author

I'd be happy to give it a go at some point, but I'm not sure when I'll have time to allocate to it. So if someone else wants to tackle it sooner, that's just fine.

@delca85
Copy link
Member

delca85 commented Jun 3, 2020

Hi! @kentcdodds @sheesania If it's ok for you I could give it a shot!

@kentcdodds
Copy link
Member

Sounds fine to me 👍

@delca85
Copy link
Member

delca85 commented Jun 4, 2020

I need a clarification: would be the desired getByLabelText behavior to make it look for a match even in the value displayed by the input elements or maybe that edit would break up something related to getByLabelText usage?
Thanks!

@kentcdodds
Copy link
Member

Hi @delca85,

I'm not 100% certain I understand what you're asking, but maybe this will help: https://cpdth.csb.app/ (codesandbox)

Here's what the accessibility tab says about this element:

image

Based on this, I should be able to select that checkbox with the query:

screen.getByLabelText('allow up to 0 words in between')

I believe that will not work right now, and that's what we should fix. Does that help?

@delca85
Copy link
Member

delca85 commented Jun 4, 2020

Hi @kentcdodds ! Thanks a lot for your explaination. I got that point, but maybe my question was not right anyway.
According to @sheesania first message, and even according to me, one of the issues related to this behavior is that the <input> element are not checked by getByLabelText so I was asking if part of the solution would be make it check <input> elements too.
Maybe this is a silly question!

@kentcdodds
Copy link
Member

(Not sure whether you'll see this because you're not active on twitter, but I tweeted about this @sheesania :) https://twitter.com/kentcdodds/status/1268571672405086209)

@delca85
Copy link
Member

delca85 commented Jun 4, 2020

yes I can see it, I am not an active writer but I am a really active reader! :)

@kentcdodds
Copy link
Member

@delca85, I was referring to @sheesania, but that's great 📚

man with books in two hands and one on his foot

@delca85
Copy link
Member

delca85 commented Jun 5, 2020

Hey @kentcdodds !
I would like to share my ideas about this bug fixing.
I thought to add an argument to getByLabelText named concat. According to this argument, the labels matcher should check the candidate label against the text, with an extra mode that check if the text includes the candidate label. This mode should exist in addition to strict and fuzzy mode.
I have already develop a draft of this solution (it provides another check on the obtained labels) and it seems be working but I was wondering if it is acceptable because I am not so convinced that the exact and concat arguments could coexist and be coherent. I could throw an error if the both are set to true, but I would like to receive your opinion.
Thank in advance!

@kentcdodds
Copy link
Member

Can you help me understand why the concat option is necessary? I'm not sure I understand.

@delca85
Copy link
Member

delca85 commented Jun 6, 2020

If I got the point rightly, the concatenated labels are not retrieved by queryAllLabelsByText because of the matcher used.
It checks if the text given as argument to getByLabelText is the same displayed by the labels found in the container.
If this matcher's check would become an includes check instead of an equality check the concatenated labels would be retrieved too. I am not sure that this is the right change to bring.
The concat option would be used to change the matcher behaviour (or to use another matcher to be created), because I suppose that the standard matcher check should be the equality one.

I hope I explained myself.

@kentcdodds
Copy link
Member

I think I understand. If you could go ahead and push the code you have now (even if it's not finished yet) then it may help a bit.

I was thinking about it and this is a lot more difficult than I initially assumed so I'm interested in what you have 😀

delca85 pushed a commit to delca85/dom-testing-library that referenced this issue Jun 6, 2020
delca85 pushed a commit to delca85/dom-testing-library that referenced this issue Jun 6, 2020
delca85 pushed a commit to delca85/dom-testing-library that referenced this issue Jun 6, 2020
@delca85
Copy link
Member

delca85 commented Jun 6, 2020

I have pushed my code. I am really doubtful about it, like I wrote in the pr but I would be glad if you share your ideas with me.

@kentcdodds
Copy link
Member

Thank you @delca85! I'll give it a look as soon as I can.

@eps1lon
Copy link
Member

eps1lon commented Jun 8, 2020

(I should also clarify that I'm happily using getByRole('checkbox', {name: 'allow up to 0 words in between'}) for my specific use case right now per this Stack Overflow post.

I would recommend this as well. getByLabelText is really only a more performant escape hatch for simple labels. It's doesn't cover many valid labelling techniques and might even be misleading in worst case scenarios. It's sufficient for testing if performance is an issue. For complex scenarios getByRole with name should be preferred.

delca85 pushed a commit to delca85/dom-testing-library that referenced this issue Jul 3, 2020
delca85 pushed a commit to delca85/dom-testing-library that referenced this issue Jul 3, 2020
delca85 pushed a commit to delca85/dom-testing-library that referenced this issue Jul 3, 2020
@delca85 delca85 mentioned this issue Jul 3, 2020
9 tasks
delca85 pushed a commit to delca85/dom-testing-library that referenced this issue Jul 4, 2020
delca85 pushed a commit to delca85/dom-testing-library that referenced this issue Jul 4, 2020
delca85 pushed a commit to delca85/dom-testing-library that referenced this issue Jul 4, 2020
delca85 pushed a commit to delca85/dom-testing-library that referenced this issue Jul 4, 2020
delca85 pushed a commit to delca85/dom-testing-library that referenced this issue Jul 4, 2020
@testing-library testing-library deleted a comment Jul 14, 2020
delca85 pushed a commit to delca85/dom-testing-library that referenced this issue Jul 16, 2020
@kentcdodds
Copy link
Member

🎉 This issue has been resolved in version 7.21.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

No branches or pull requests

4 participants