fix(getByLabelText): Support aria-labelledby attr containing multiple ids #59
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,8 @@ import 'jest-dom/extend-expect' | |
| import {render} from './helpers/test-utils' | ||
|
|
||
| beforeEach(() => { | ||
| window.Cypress = null | ||
| }) | ||
|
|
||
| test('query can return null', () => { | ||
| const { | ||
|
|
@@ -82,12 +82,20 @@ test('get can get form controls by label text', () => { | |
| <label for="fourth.id">4th</label> | ||
| <input id="fourth.id" /> | ||
| </div> | ||
| <div> | ||
| <div> | ||
| <label id="fifth-label-one">5th one</label> | ||
| <label id="fifth-label-two">5th two</label> | ||
| <input aria-labelledby="fifth-label-one fifth-label-two" id="fifth-id" /> | ||
| </div> | ||
| </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') | ||
|
There was a problem hiding this comment. 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: Which element do you want to find with What about If you want to really support this multi-label use case, it's more complex than changing the selector. There was a problem hiding this comment. Hi @sompylasar and thanks for the super fast feedback :) From what I understand if there is support
Why would you expect two? The default behavior of getByLabelText(
(content, element) =>
content === 'Billing' && element.getAttribute('aria-labelledby').includes('address'),
)
I don't understand this one since 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 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 Another problem that I found while using the MDN examples is that
IMO a possible solution would be:
@sompylasar do you think this could work? Are there other concerns that I missed? There was a problem hiding this comment.
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.
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.
This sounds like how this function should work, I wonder why it doesn't and how it works now.
I agree with providing more context to the matcher functions for the caller to make the decisions.
Yes, this should only apply to RE: which direction the library should go, I degelate to @kentcdodds There was a problem hiding this comment.
This is correct. I don't think changes are necessary. If |
||
| }) | ||
|
|
||
| test('get can get form controls by placeholder', () => { | ||
|
|
@@ -174,15 +182,15 @@ test('getAll* matchers return an array', () => { | |
| <div> | ||
| <img | ||
| data-testid="poster" | ||
| alt="finding nemo poster" | ||
| src="/finding-nemo.png" /> | ||
| <img | ||
| data-testid="poster" | ||
| alt="finding dory poster" | ||
| src="/finding-dory.png" /> | ||
| <img | ||
| data-testid="poster" | ||
| alt="jumanji poster" | ||
| src="/jumanji.png" /> | ||
| <p>Where to next?</p> | ||
| <label for="username">User Name</label> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,9 @@ import {getNodeText} from './get-node-text' | |
| import {prettyDOM} from './pretty-dom' | ||
|
|
||
| function debugDOM(htmlElement) { | ||
| const limit = process.env.DEBUG_PRINT_LIMIT || 7000 | ||
| const inNode = typeof module !== 'undefined' && module.exports | ||
| const inCypress = typeof window !== 'undefined' && window.Cypress | ||
| /* istanbul ignore else */ | ||
| if (inCypress) { | ||
| return '' | ||
|
|
@@ -66,7 +66,7 @@ function queryAllByLabelText( | |
| if (label.getAttribute('id')) { | ||
| // <label id="someId">text</label><input aria-labelledby="someId" /> | ||
| return container.querySelector( | ||
| `[aria-labelledby~="${label.getAttribute('id')}"]`, | ||
| ) | ||
| } | ||
| if (label.childNodes.length) { | ||
|
|
@@ -155,7 +155,10 @@ function queryByAltText(...args) { | |
| function getAllByTestId(container, id, ...rest) { | ||
| const els = queryAllByTestId(container, id, ...rest) | ||
| if (!els.length) { | ||
| throw getElementError( | ||
| `Unable to find an element by: [data-testid="${id}"]`, | ||
| container, | ||
| ) | ||
| } | ||
| return els | ||
| } | ||
|
|
@@ -167,7 +170,10 @@ function getByTestId(...args) { | |
| function getAllByTitle(container, title, ...rest) { | ||
| const els = queryAllByTitle(container, title, ...rest) | ||
| if (!els.length) { | ||
| throw getElementError( | ||
| `Unable to find an element with the title: ${title}.`, | ||
| container, | ||
| ) | ||
| } | ||
| return els | ||
| } | ||
|
|
@@ -179,7 +185,10 @@ function getByTitle(...args) { | |
| function getAllByValue(container, value, ...rest) { | ||
| const els = queryAllByValue(container, value, ...rest) | ||
| if (!els.length) { | ||
| throw getElementError( | ||
| `Unable to find an element with the value: ${value}.`, | ||
| container, | ||
| ) | ||
| } | ||
| return els | ||
| } | ||
|
|
@@ -191,7 +200,10 @@ function getByValue(...args) { | |
| function getAllByPlaceholderText(container, text, ...rest) { | ||
| const els = queryAllByPlaceholderText(container, text, ...rest) | ||
| if (!els.length) { | ||
| throw getElementError( | ||
| `Unable to find an element with the placeholder text of: ${text}`, | ||
| container, | ||
| ) | ||
| } | ||
| return els | ||
| } | ||
|
|
@@ -205,9 +217,15 @@ function getAllByLabelText(container, text, ...rest) { | |
| if (!els.length) { | ||
| const labels = queryAllLabelsByText(container, text, ...rest) | ||
| if (labels.length) { | ||
| throw getElementError( | ||
| `Found a label with the text of: ${text}, however no form control was found associated to that label. Make sure you're using the "for" attribute or "aria-labelledby" attribute correctly.`, | ||
| container, | ||
| ) | ||
| } else { | ||
| throw getElementError( | ||
| `Unable to find a label with the text of: ${text}`, | ||
|
There was a problem hiding this comment. In other messages it's "with the text:" but here it's "with the text of:" — inconsistent. There was a problem hiding this comment. @sompylasar should I act on this? There was a problem hiding this comment. Not sure, I just noticed. There was a problem hiding this comment. Don't bother. We can fix that in another PR if we want. |
||
| container, | ||
| ) | ||
| } | ||
| } | ||
| return els | ||
|
|
@@ -220,7 +238,10 @@ function getByLabelText(...args) { | |
| function getAllByText(container, text, ...rest) { | ||
| const els = queryAllByText(container, text, ...rest) | ||
| if (!els.length) { | ||
| throw getElementError( | ||
| `Unable to find an element with the text: ${text}. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.`, | ||
| container, | ||
| ) | ||
| } | ||
| return els | ||
| } | ||
|
|
@@ -232,7 +253,10 @@ function getByText(...args) { | |
| function getAllByAltText(container, alt, ...rest) { | ||
| const els = queryAllByAltText(container, alt, ...rest) | ||
| if (!els.length) { | ||
| throw getElementError( | ||
| `Unable to find an element with the alt text: ${alt}`, | ||
| container, | ||
| ) | ||
| } | ||
| return els | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Off-topic but why null, not undefined? @kentcdodds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.