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] no-invalid-html-attribute
: allow 'shortcut icon' on link
#3174
Conversation
no-invalid-html-attribute
- allow 'shortcut' on link
no-invalid-html-attribute
- allow 'shortcut' on linkno-invalid-html-attribute
: allow 'shortcut' on link
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.
Let's add some tests that show that shortcut
by itself is invalid.
OK I see that I can't actually do two things in one go, so even code: '<a rel={"batgo noopener"}></a>',
output: '<a rel={" noopener"}></a>', doesn't turn directly into only I added my intentions kind of line by line so that you can understand the code better for making suggestions. |
Ohh and there was an issue on the white-space part - before we always removed everything that is not exactly one space character - but that means that two spaces between two words will just be removed, instead we should replace all white-space that is not in the beginning or the end to exactly one space character. For security this should also include non-breaking-spaces (e.g. when copied from somewhere else) - so I added another test case that covers that. |
We should not autofix |
Thanks for the feedback! Will work on it! 👍🏻 |
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.
Looks great! Just one nit.
}, | ||
{ | ||
code: '<link rel="shortcut foo"></link>', | ||
output: '<link rel="shortcut "></link>', |
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.
output: '<link rel="shortcut "></link>', | |
output: '<link rel="shortcut"></link>', |
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.
Hmm but it actually does that - it's the opposite of
code: '<a rel={"batgo noopener"}></a>',
output: '<a rel={" noopener"}></a>',
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.
1) no-invalid-html-attribute
invalid
<link rel="shortcut foo"></link>:
AssertionError [ERR_ASSERTION]: Output is incorrect.
+ expected - actual
-<link rel="shortcut "></link>
+<link rel="shortcut"></link>
The test fails when I remove it
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
Ahh no, we shouldn't remove it - it is the testcase for the notPaired
branch (in contrast to notAlone
)
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.
I'm basically saying that the autofix output should never have leading or trailing spaces.
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.
Yeah that also confused me, but apparently that's "how things used to be" in master
as well:
Apparently it only runs one cycle when fixing and we would need to find a way to kind of while
loop so long until it can't fix any more issues - but that sounds like a major refactor to me 🤔
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.
fair point. altho it seems like adding .trim()
to the attribute contents wouldn't be that difficult?
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.
Good idea! But where would I run this, I think I haven't fully grasped when/how fix runs. Would this be part of fix? Or would we just always in the end after all checks run another trim as it's never good/valid to keep it?
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.
Adding a single fixer on program exit that trimmed any attribute strings that had been previously autofixed (we’d have to track that manually) would be good. I think that should wait for a followup PR, tho, because it was a pre-existing problem.
I’ll land this tomorrow, as-is.
Codecov Report
@@ Coverage Diff @@
## master #3174 +/- ##
=======================================
Coverage 97.54% 97.54%
=======================================
Files 120 120
Lines 8265 8265
Branches 2975 2975
=======================================
Hits 8062 8062
Misses 203 203 Continue to review full report at Codecov.
|
no-invalid-html-attribute
: allow 'shortcut' on linkno-invalid-html-attribute
: allow 'shortcut icon' on link
no-invalid-html-attribute
: allow 'shortcut icon' on linkno-invalid-html-attribute
: allow 'shortcut icon' on link
Yay 🎉 Thanks for your support - in the end it was a lot of fun, even when it was a bit scary first 🚀 |
Heads up that I did need to land 5f49f51, due to array destructuring and spread syntax not working on node 4 and 5. |
I also filed #3180 to track updating the implementation - I think now that we have "pairs", and good test cases, it'd be worth rewriting it from scratch. |
Closes #3172
Description
For historical reasons, the icon keyword may be preceded by the keyword "shortcut".
https://html.spec.whatwg.org/multipage/links.html#rel-icon (last sentence before the next paragraph about
license
)Intentions
foo
can go withbar
ORbaz
but NEVERqux
Demo
This introduces two new messages:
Reports:
“shortcut” must be directly followed by “icon”.
Reports:
“shortcut” can not be directly followed by “manifest” without “icon”.