Skip to content
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.

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

fix: [1706] Handle non string values gracefully for removeAttribute o… #1707

Conversation

OlaviSau
Copy link
Contributor

…n elements

@OlaviSau OlaviSau requested a review from capricorn86 as a code owner January 29, 2025 09:37
@OlaviSau OlaviSau force-pushed the fix-1706-remove-attribute-with-non-string branch from b49fdea to b87aba3 Compare January 29, 2025 09:47
@OlaviSau OlaviSau changed the title fix: [1702] Handle non string values gracefully for removeAttribute o… fix: [1706] Handle non string values gracefully for removeAttribute o… Jan 29, 2025
Copy link
Owner

@capricorn86 capricorn86 left a comment

Choose a reason for hiding this comment

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

Nice contribution!

@@ -793,6 +793,9 @@ export default class Element
* @param name Name.
*/
public removeAttribute(name: string): void {
if (typeof name !== 'string') {
Copy link
Owner

@capricorn86 capricorn86 Jan 29, 2025

Choose a reason for hiding this comment

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

The attribute is actually not ignored when name is not a string, it is stringified.

You can test it with this:

var div = document.createElement('div');

div.setAttribute(null, 'test');

// Outputs '<div null="test"></div>'
console.log(div.outerHTML);

div.removeAttribute(null);

// Outputs '<div></div>'
console.log(div.outerHTML);

I think it is best if we add the stringify using String() to NamedNodeMap to methods such as getNamedItem().

@OlaviSau OlaviSau force-pushed the fix-1706-remove-attribute-with-non-string branch from b87aba3 to c5bd563 Compare January 29, 2025 19:42
@OlaviSau OlaviSau force-pushed the fix-1706-remove-attribute-with-non-string branch from 7936d65 to 38934ea Compare January 30, 2025 09:13
@OlaviSau
Copy link
Contributor Author

OlaviSau commented Jan 30, 2025

One note - there is no need to implement a check on NamedNodeMap setNamedItem because Attr constructor should not available in the first place, thus an invalid name should not be able to be created unless the user explicitly goes out of their way to do so.

@OlaviSau OlaviSau force-pushed the fix-1706-remove-attribute-with-non-string branch from 4e7e662 to 5e41a11 Compare January 30, 2025 11:06
@OlaviSau
Copy link
Contributor Author

This is quite a significant breaking change even though it's more correct this way. Perhaps we should add a strict mode flag and not enable it by default and have a transition period?

@OlaviSau OlaviSau force-pushed the fix-1706-remove-attribute-with-non-string branch 2 times, most recently from 1f99c55 to 3422c7e Compare January 30, 2025 11:10
@OlaviSau OlaviSau closed this Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants