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] no-invalid-html-attribute: allow 'shortcut icon' on link #3174

Merged
merged 2 commits into from Jan 17, 2022

Conversation

Primajin
Copy link
Contributor

@Primajin Primajin commented Jan 11, 2022

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

  1. First of all I'm checking if there are pairs registered for the given attribute name after all.
  2. Now I'm splitting the attribute value(s) into overlapping pairs. If you want to check on the funky regex: https://regex101.com/r/aJXAEz/1
  3. now I'm iterating pair by pair
  4. on each pair I check what are the possible siblings for my current item - currently there is only 1:1 but potentially there could be a pair where foo can go with bar OR baz but NEVER qux
  5. now I check if my pair is separated by a single space or multiple in between
  6. Check if the first value has a corresponding pairing
  7. Get the last value as this is the "real" second one
  8. Check if this is not one of the anticipated siblings
  9. set message and message id based on, if the second value is truthy
    1. if it is the attribute was not paired with the right one
    2. if its not it stands alone which makes it invalid.
  10. report the issue
  11. check if there is only one option what the sibling could be, if there is only one anyways let's autofix it for the user! 🎉

Demo

This introduces two new messages:

<link rel="shortcut"></link>

Reports: “shortcut” must be directly followed by “icon”.



<link rel="shortcut manifest"></link>

Reports: “shortcut” can not be directly followed by “manifest” without “icon”.

@Primajin Primajin changed the title allow 'shortcut' on link [fix] no-invalid-html-attribute - allow 'shortcut' on link Jan 11, 2022
@Primajin Primajin changed the title [fix] no-invalid-html-attribute - allow 'shortcut' on link [Fix] no-invalid-html-attribute: allow 'shortcut' on link Jan 11, 2022
@Primajin Primajin marked this pull request as draft January 15, 2022 18:43
Copy link
Member

@ljharb ljharb left a 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.

lib/rules/no-invalid-html-attribute.js Outdated Show resolved Hide resolved
lib/rules/no-invalid-html-attribute.js Outdated Show resolved Hide resolved
@Primajin
Copy link
Contributor Author

Let's add some tests that show that shortcut by itself is invalid.

Yes that should be already in:

    {
      code: '<link rel="shortcut"></link>',
      output: '<link rel="shortcut icon"></link>',
      errors: [
        {
          messageId: 'notAlone',
          data: {
            reportingValue: 'shortcut',
            missingValue: 'icon',
          },
        },
      ],
    },

Just shortcut can not be alone and will be auto-fixed to shortcut icon.

Now next I need to look into some spacing that is still added:
image

Hope to be able to solve this soon 🤞🏻
When everything is ready I will mark the PR as ... "not draft" 😅 and will write up some description what I did and what my intentions were.

@Primajin Primajin marked this pull request as ready for review January 16, 2022 12:39
@Primajin
Copy link
Contributor Author

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 noopener but it is expected that there is an (invalid) leading space, so then I changed the expectations on my newly added code to also expect a trailing space that I just couldn't manage to get rid of in one go (will be picked up by spaceDelimited rule in the next cycle).

I added my intentions kind of line by line so that you can understand the code better for making suggestions.
Happy / Ready for round two 😄 👍🏻

@Primajin
Copy link
Contributor Author

Primajin commented Jan 16, 2022

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.
This didn't influence any of the existing tests, so this should be safe 👍🏻

@Primajin Primajin requested a review from ljharb January 16, 2022 13:09
@ljharb
Copy link
Member

ljharb commented Jan 16, 2022

We should not autofix shortcut to shortcut icon; that's not a safe autofix because it will change runtime behavior. It should be a warning that requires the user to manually fix it.

tests/lib/rules/no-invalid-html-attribute.js Outdated Show resolved Hide resolved
tests/lib/rules/no-invalid-html-attribute.js Outdated Show resolved Hide resolved
tests/lib/rules/no-invalid-html-attribute.js Outdated Show resolved Hide resolved
tests/lib/rules/no-invalid-html-attribute.js Outdated Show resolved Hide resolved
@Primajin
Copy link
Contributor Author

Thanks for the feedback! Will work on it! 👍🏻

@Primajin Primajin requested a review from ljharb January 16, 2022 17:02
Copy link
Member

@ljharb ljharb left a 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>',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
output: '<link rel="shortcut "></link>',
output: '<link rel="shortcut"></link>',

Copy link
Contributor Author

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>',

Copy link
Contributor Author

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.

Copy link
Contributor Author

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)

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 basically saying that the autofix output should never have leading or trailing spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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-commenter
Copy link

codecov-commenter commented Jan 17, 2022

Codecov Report

Merging #3174 (0d89462) into master (151bb2b) will not change coverage.
The diff coverage is n/a.

❗ Current head 0d89462 differs from pull request most recent head 4de38ef. Consider uploading reports for the commit 4de38ef to get more accurate results
Impacted file tree graph

@@           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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 151bb2b...4de38ef. Read the comment docs.

@ljharb ljharb changed the title [Fix] no-invalid-html-attribute: allow 'shortcut' on link [Fix] no-invalid-html-attribute: allow 'shortcut icon' on link Jan 17, 2022
@ljharb ljharb changed the title [Fix] no-invalid-html-attribute: allow 'shortcut icon' on link [Fix] no-invalid-html-attribute: allow 'shortcut icon' on link Jan 17, 2022
@Primajin
Copy link
Contributor Author

Yay 🎉 Thanks for your support - in the end it was a lot of fun, even when it was a bit scary first 🚀

@ljharb
Copy link
Member

ljharb commented Jan 18, 2022

Heads up that I did need to land 5f49f51, due to array destructuring and spread syntax not working on node 4 and 5.

@ljharb
Copy link
Member

ljharb commented Jan 18, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

react/no-invalid-html-attribute seems to report false positives
3 participants