fix: extra protection around tagName accessor #13
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
karlsnyder0
approved these changes
May 15, 2025
src/getTag.js
Outdated
Comment on lines
13
to
16
| // This can happen when a <form> element contains an <input> | ||
| // with an id of `tagName`. In this case, the form element's | ||
| // tagName property is a reference to the input element, not | ||
| // a string. |
src/getTag.js
Outdated
| @@ -6,6 +6,17 @@ | |||
| */ | |||
| export function getTag( el, filter ) | |||
| { | |||
| if (typeof el.tagName !== 'string') { | |||
There was a problem hiding this comment.
Maybe fall back to try nodeName in this case, and if that's also non-string then all bets are off and return null
There was a problem hiding this comment.
Good idea, validating it in the browser and it seems like nodeName is not effected by the tagName override (unless an input has an id of nodeName, in which case it is lol).
So the only place we'd fall down is if we had a form like:
<form>
<input id="tagName" />
<input id="nodeName" />
</form>
nodeName is used by the isElement checks so having that overridden would be much more painful. I don't think we've seen that yet 🤞
mike-plummer
approved these changes
May 15, 2025
|
🎉 This PR is included in version 2.1.1 🎉 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alert: https://cypressio.slack.com/archives/C0614M4DZV5/p1746718971129419
We have a test case in the cypress-services repo that specifically overrides a form element's
tagNameproperty using a nested<input id="tagName">element. When the form is processed with unique-selector, unique-selector will throw as a result of this overridden property that references an element (rather than the expected string). This is causing a few alerts to come through (and knocking our Cypress reports into a "Processed with errors" state).So, added a little check to ignore tagName when it's not the expected type.
I looked at the other accessors in use here, and most everything else goes through a
getAttributecall. I [don't think we can really be exhaustive here without going overboard, buttagNameoverrides are something the App has run into as well, so we might as well work around it (and keep our own tests happy). If we notice other goofy things happening to elements, we can evaluate further protections.