Fix null labels on hidden inputs#804
Fix null labels on hidden inputs#804kentcdodds merged 2 commits intotesting-library:masterfrom thomasmarshall:pr/fix-hidden-input-labels
Conversation
The `labels` property on `input` elements of type `hidden` is `null` rather than `NodeList` [1]. This meant the `getRealLabels` function would return `null` causing `queryAllByLabelText` to throw an error when it tried to call `length` on `null` [2]. This commit fixes the issue by ensuring the element is labelable before calling `labels` on it, and adds a test case for this specific scenario. [1]: https://html.spec.whatwg.org/multipage/forms.html#dom-lfe-labels [2]: https://github.com/testing-library/dom-testing-library/blob/62f4e5e09a4b81ef66679560b540523edccdef98/src/queries/label-text.js#L52
|
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. |
Codecov Report
@@ Coverage Diff @@
## master #804 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 26 26
Lines 719 719
Branches 184 184
=========================================
Hits 719 719
Continue to review full report at Codecov.
|
kentcdodds
left a comment
kentcdodds
left a comment
There was a problem hiding this comment.
Thanks for the contribution! One question please.
| test('hidden inputs are not labelable', () => { | ||
| const element = document.createElement('input') | ||
| element.type = 'hidden' | ||
| expect(getRealLabels(element)).toEqual([]) |
There was a problem hiding this comment.
What was the result before your changes? 🤔
There was a problem hiding this comment.
Before the change it returns null:
There was a problem hiding this comment.
I didn't see the PR and I have opened a PR on the same behaviour because I saw an issue about it.
marcosvega91
left a comment
marcosvega91
left a comment
There was a problem hiding this comment.
Thanks! :) I have added a small request
src/label-helpers.js
Outdated
| if (element.labels !== undefined) return element.labels | ||
|
|
There was a problem hiding this comment.
Is better to do in this way instead of swap the two functions?
| if (element.labels !== undefined) return element.labels | |
| if (element.labels !== undefined) return element.labels ?? [] | |
There was a problem hiding this comment.
I've added a squash commit with your suggestion – thanks @marcosvega91.
This commit fixes the issue by retuning an empty array if the `labels` property is `null`, and adds a test case for this specific scenario.
kentcdodds
left a comment
kentcdodds
left a comment
There was a problem hiding this comment.
Thank you very much! I apologize for not reading your original post more carefully. Had I done that, I wouldn't have needed you to take the time to make the screenshot 🤦♂️
I blame covid.
Thanks!
|
@all-contributors please add @thomasmarshall for code and tests |
|
I've put up a pull request to add @thomasmarshall! 🎉 |
|
No worries, thanks! |
|
🎉 This PR is included in version 7.26.5 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
4 participants
What:
This PR ensures
*LabelTexthelpers do not throw an error when there are hidden inputs.Why:
The
labelsproperty oninputelements of typehiddenisnullrather thanNodeList. This meant thegetRealLabelsfunction would returnnullcausingqueryAllByLabelTextto throw an error when it tried to calllengthonnull.I noticed this issue when using
cy.findByLabelText(from @testing-library/cypress) – which fails with the following exception on pages with hidden inputs:How:
This fixes the issue by ensuring the element is labelable before calling
labelson it. There was already anisLabelableguard clause in place, but it was only checked after returningelement.labels(which isnullfor hidden inputs). Alternative solutions might be to add a different guard clause to return an empty array ifelement.labelsisnull, or to check the return value fromgetRealLabelsbefore callinglengthon it.Checklist:
docs site N/A