Skip to content

fix(ByLabelText): support IE and other legacy browsers #726

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Aug 5, 2020
Merged

fix(ByLabelText): support IE and other legacy browsers #726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
664d980
fix(ByLabelText): support IE
maxnewlands Aug 2, 2020
847996e
fix: ensure mocks are restored
maxnewlands Aug 3, 2020
311891b
test: remove unnecessary test cases
maxnewlands Aug 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/__tests__/element-queries.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1182,3 +1182,19 @@ test('return a proper error message when no label is found and there is an aria-
</div>"
`)
})

// https://github.com/testing-library/dom-testing-library/issues/723
it('gets form controls by label text on IE and other legacy browsers', () => {
// Simulate lack of support for HTMLInputElement.prototype.labels
jest
.spyOn(HTMLInputElement.prototype, 'labels', 'get')
.mockReturnValue(undefined)

const {getByLabelText} = renderIntoDocument(`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how necessary it is to copy/paste this from the other test. I think a single label that situation that covers the code path we're testing should suffice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we'd use the same test that is run with a test matrix covering different environments i.e. run with

  • .labels and .control (modern browser
  • no .labels but .control (IE 11)
  • neither .lables nor .control (edge 17)

I guess we can set something up like this in another PR where we leverage different jest environments that mock out problematic DOM methods we're aware of.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neither .lables nor .control (edge 17)

Shoot, I overlooked this. Appreciate you calling it out, @eps1lon !

I'll push my changes that reduce this to one test case for now. I also like the idea of running a comprehensive set of tests in different environments, and if it's preferable to take that approach instead of what's in this PR, please feel free to close this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, let's get this in then we can add that in a future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. Then if there are no other comments, this just needs an approval. 🙏

<label>
Label text
<input id="input-id" />
</label>
`)
expect(getByLabelText('Label text').id).toBe('input-id')
})
25 changes: 21 additions & 4 deletions src/queries/label-text.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,9 @@ function queryAllByLabelText(
const matcher = exact ? matches : fuzzyMatches
const matchNormalizer = makeNormalizer({collapseWhitespace, trim, normalizer})
const matchingLabelledElements = Array.from(container.querySelectorAll('*'))
.filter(
element => element.labels || element.hasAttribute('aria-labelledby'),
)
.filter(element => {
return getLabels(element) || element.hasAttribute('aria-labelledby')
})
.reduce((labelledElements, labelledElement) => {
const labelsId = labelledElement.getAttribute('aria-labelledby')
? labelledElement.getAttribute('aria-labelledby').split(' ')
Expand All @@ -86,7 +86,7 @@ function queryAllByLabelText(
)
return labellingElement ? getLabelContent(labellingElement) : ''
})
: Array.from(labelledElement.labels).map(label => {
: Array.from(getLabels(labelledElement)).map(label => {
const textToMatch = getLabelContent(label)
const formControlSelector =
'button, input, meter, output, progress, select, textarea'
Expand Down Expand Up @@ -235,3 +235,20 @@ export {
findAllByLabelText,
findByLabelText,
}

// Based on https://github.com/eps1lon/dom-accessibility-api/pull/352
function getLabels(element) {
if (element.labels !== undefined) return element.labels

if (!isLabelable(element)) return null

const labels = element.ownerDocument.querySelectorAll('label')
return Array.from(labels).filter(label => label.control === element)
}

function isLabelable(element) {
return (
element.tagName.match(/BUTTON|METER|OUTPUT|PROGRESS|SELECT|TEXTAREA/) ||
(element.tagName === 'INPUT' && element.getAttribute('type') !== 'hidden')
)
}
2 changes: 1 addition & 1 deletion tests/setup-env.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,5 @@ beforeAll(() => {
})

afterAll(() => {
console.warn.mockRestore()
jest.restoreAllMocks()
})