fix(getByLabelText): Support aria-labelledby attr containing multiple ids #59

rubencosta · 2018-06-16T08:51:36Z

What: Change getByLabelText to support inputs where aria-labelledby attr contains multiple label ids

Why: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-labelledby_attribute

How: By changing the css attribute selector from [attr=value] to [attr~=value]

Checklist:

Documentation N/A
Tests
Ready to be merged
Added myself to contributors table

sompylasar · 2018-06-16T17:30:17Z

src/__tests__/element-queries.js

    </div>
  `)
  expect(getByLabelText('1st').id).toBe('first-id')
  expect(getByLabelText('2nd').id).toBe('second-id')
  expect(getByLabelText('3rd').id).toBe('third-id')
  expect(getByLabelText('4th').id).toBe('fourth.id')
+  expect(getByLabelText('5th one').id).toBe('fifth-id')
+  expect(getByLabelText('5th two').id).toBe('fifth-id')


If there's 1-to-many relation between label and element, getByLabelText should either return an array of matching elements, or return the element with "most relevant" label (e.g. matching label's id should be closer to the end of an element's labelledby list, though there still can be more than one of those), or something else.

Try with Mozilla's example in the test:

<div id="billing">Billing</div> <div> <div id="name">Name</div> <input type="text" aria-labelledby="billing name"/> </div> <div> <div id="address">Address</div> <input type="text" aria-labelledby="billing address"/> </div>

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-labelledby_attribute

Which element do you want to find with getByLabelText('Billing')? I'd expect two.

What about getByLabelText('Billing Name')? I'd expect to find one, but it won't.

If you want to really support this multi-label use case, it's more complex than changing the selector.

Hi @sompylasar and thanks for the super fast feedback :)

From what I understand if there is support aria-labelledby then having 1-to-n elements to labels should also be supported and I think that it already is with getAllByLabelText since it will return an array of matching elements.
For the getByLabelText I understand your concern about which element to return when there are multiple valid matches but IMO trying to come up with a decision for which is the "most relevant" element is not necessary since AFAIK this is not part of the spec and the default behavior of querySelector is also to return the first match. That would actually be my expectation.

Which element do you want to find with getByLabelText('Billing')? I'd expect two.

Why would you expect two? The default behavior of getBy*() is to return the first match correct? If something else is necessary you can pass a function. So let's say that I actually want the billing address:

getByLabelText( (content, element) => content === 'Billing' && element.getAttribute('aria-labelledby').includes('address'), )

What about getByLabelText('Billing Name')? I'd expect to find one, but it won't.

I don't understand this one since getLabelText takes the text of one label and you are passing the text of two different labels. Maybe would be nice to be able to do this?

To me it does not seem so complex but I am a new user of this library and might not understand it's goals. Anyway I am happy to add the tests for the examples in https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-labelledby_attribute and see that all are supported but I don't know if this makes sense. For my use case I basically would like getByLabelText to find elements that use aria-labelledby with A space-separated list of element IDs which is a valid value.

Update

After testing out my proposed solution for finding a specific element when there are multiple associated with a label does not work with the current implementation because the element that gets passed to the matcher funcion is actually the label element and not the labelled element. IMO this is not intuitive comparing with the getByText(fn) that gets called with the element that you want to select.

Another problem that I found while using the MDN examples is that getByLabelText actually only looks for label elements but according to MDN page:

This attribute can be used with any typical HTML form element; it is not limited to elements that have an ARIA role assigned.

IMO a possible solution would be:

Changing the behavior of queryAllByLabelText so that it always returns an array of elements associated with the label that can be correctly filtered by the user if the user provides a matcher function.

For this, It should call the matcher function with the textContent (of the labelElement), the labelElement and the labelledElement so that it would be possible to match a specific label element and also to match a specific labelled element.

Unrestricting the selector so that it stops being label specific (at least for the aria-labelledby case).

@sompylasar do you think this could work? Are there other concerns that I missed?

Why would you expect two? The default behavior of getBy*() is to return the first match correct?

Yes, agree, getBy for first-in-DOM, getAllBy for all.

Thanks for the long reply and the Update! I'm glad that you found some API inconsistencies.

What about getByLabelText('Billing Name')? I'd expect to find one, but it won't.

I don't understand this one since getLabelText takes the text of one label and you are passing the text of two different labels. Maybe would be nice to be able to do this?

To me it does not seem so complex but I am a new user of this library and might not understand it's goals.

The goal of this library, at least of this part with the queries (let @kentcdodds correct me if I'm wrong) is to provide DOM query functions to look for elements based on what the user can read (with eyes or a screenreader).

If the element says it's labelledby two labels, and the Mozilla example shows that, it means that one label is not enough to understand the purpose of the labelledby element (the input): it's not enough to look for "Name", it may be ambiguous, as there can be another section before "Billing" like "Personal" with the same "Name" label.

Changing the behavior of queryAllByLabelText so that it always returns an array of elements associated with the label that can be correctly filtered by the user if the user provides a matcher function.

This sounds like how this function should work, I wonder why it doesn't and how it works now.

For this, It should call the matcher function with the textContent (of the labelElement), the labelElement and the labelledElement so that it would be possible to match a specific label element and also to match a specific labelled element.

I agree with providing more context to the matcher functions for the caller to make the decisions.

Unrestricting the selector so that it stops being label specific (at least for the aria-labelledby case).

Yes, this should only apply to aria-labelledby case, it should still work the same for plain label.

RE: which direction the library should go, I degelate to @kentcdodds

From what I understand if there is support aria-labelledby then having 1-to-n elements to labels should also be supported and I think that it already is with getAllByLabelText since it will return an array of matching elements.

This is correct. I don't think changes are necessary. If getAllByLabelText is not working as expected, let's file that as a separate bug.

sompylasar · 2018-06-16T17:31:24Z

src/__tests__/element-queries.js

@@ -2,8 +2,8 @@ import 'jest-dom/extend-expect'
 import {render} from './helpers/test-utils'

 beforeEach(() => {
-  window.Cypress = null;
-});
+  window.Cypress = null


Off-topic but why null, not undefined? @kentcdodds

Someone else did that I think. I probably would have set it to undefined, but I didn't care enough to ask them to change it.

sompylasar · 2018-06-16T17:33:13Z

src/queries.js

    } else {
-      throw getElementError(`Unable to find a label with the text of: ${text}`, container)
+      throw getElementError(
+        `Unable to find a label with the text of: ${text}`,


In other messages it's "with the text:" but here it's "with the text of:" — inconsistent.

@sompylasar should I act on this?

Not sure, I just noticed.

Don't bother. We can fix that in another PR if we want.

kentcdodds

I think this PR is good as-is. Please correct me if I'm missing something.

kentcdodds · 2018-06-18T22:07:35Z

src/__tests__/element-queries.js

@@ -2,8 +2,8 @@ import 'jest-dom/extend-expect'
 import {render} from './helpers/test-utils'

 beforeEach(() => {
-  window.Cypress = null;
-});
+  window.Cypress = null


Someone else did that I think. I probably would have set it to undefined, but I didn't care enough to ask them to change it.

kentcdodds · 2018-06-18T22:11:22Z

src/__tests__/element-queries.js

    </div>
  `)
  expect(getByLabelText('1st').id).toBe('first-id')
  expect(getByLabelText('2nd').id).toBe('second-id')
  expect(getByLabelText('3rd').id).toBe('third-id')
  expect(getByLabelText('4th').id).toBe('fourth.id')
+  expect(getByLabelText('5th one').id).toBe('fifth-id')
+  expect(getByLabelText('5th two').id).toBe('fifth-id')


From what I understand if there is support aria-labelledby then having 1-to-n elements to labels should also be supported and I think that it already is with getAllByLabelText since it will return an array of matching elements.

This is correct. I don't think changes are necessary. If getAllByLabelText is not working as expected, let's file that as a separate bug.

kentcdodds · 2018-06-18T22:12:17Z

src/queries.js

    } else {
-      throw getElementError(`Unable to find a label with the text of: ${text}`, container)
+      throw getElementError(
+        `Unable to find a label with the text of: ${text}`,


Don't bother. We can fix that in another PR if we want.

kentcdodds · 2018-06-27T20:38:19Z

Sorry I've forgotten about this PR. If you could fixup the merge conflicts that'd be great.

The best way is probably to resolve conflicts in .all-contributorsrc then run git commit --amend --no-edit and it'll fix the README for you in the githook.

… ids

rubencosta · 2018-06-28T05:10:49Z

@kentcdodds no prob :) I've fixed the merge conflicts.

kentcdodds

Coolio 👍 thanks!

kentcdodds · 2018-06-28T05:21:32Z

🎉 This PR is included in version 2.6.3 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

sompylasar reviewed Jun 16, 2018

View reviewed changes

kentcdodds approved these changes Jun 18, 2018

View reviewed changes

fix(getByLabelText): Support aria-labelledby attr containing multiple…

648415a

… ids

rubencosta force-pushed the pr/multiple-aria-labelledby branch from 965bd04 to 648415a Compare June 28, 2018 04:36

kentcdodds approved these changes Jun 28, 2018

View reviewed changes

kentcdodds merged commit ba44c14 into testing-library:master Jun 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(getByLabelText): Support aria-labelledby attr containing multiple ids #59

fix(getByLabelText): Support aria-labelledby attr containing multiple ids #59

rubencosta commented Jun 16, 2018

sompylasar Jun 16, 2018

rubencosta Jun 17, 2018 •

edited

sompylasar Jun 17, 2018

kentcdodds Jun 18, 2018

sompylasar Jun 16, 2018

kentcdodds Jun 18, 2018

sompylasar Jun 16, 2018

rubencosta Jun 17, 2018

sompylasar Jun 17, 2018

kentcdodds Jun 18, 2018

kentcdodds left a comment

kentcdodds Jun 18, 2018

kentcdodds Jun 18, 2018

kentcdodds Jun 18, 2018

kentcdodds commented Jun 27, 2018

rubencosta commented Jun 28, 2018

kentcdodds left a comment

kentcdodds commented Jun 28, 2018

fix(getByLabelText): Support aria-labelledby attr containing multiple ids #59

fix(getByLabelText): Support aria-labelledby attr containing multiple ids #59

Conversation

rubencosta commented Jun 16, 2018

Choose a reason for hiding this comment

rubencosta Jun 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Jun 27, 2018

rubencosta commented Jun 28, 2018

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Jun 28, 2018

rubencosta Jun 17, 2018 •

edited