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

Add more "missing value defaults" #36

Closed
karlhorky opened this issue Dec 6, 2020 · 17 comments
Closed

Add more "missing value defaults" #36

karlhorky opened this issue Dec 6, 2020 · 17 comments
Labels
🗄 area/interface This affects the public interface 💪 phase/solved Post is done 🧒 semver/minor This is backwards-compatible change 🦋 type/enhancement This is great to have

Comments

@karlhorky
Copy link

karlhorky commented Dec 6, 2020

Hi @wooorm!

Subject of the feature

As discussed, the rehype-minify-enumerated-attribute schema doesn't include some "missing value defaults":

  • img[loading]
  • iframe[loading]
  • form[target]
  • button[formtarget]
  • ol[type]
  • li[type]
  • ul[type] (although, this is deprecated)

These are just the ones that I have found in my short, spotty research on MDN. There are probably more.

Problem

I'm trying to gather a list of "missing value defaults" for a new ESLint rule:

jsx-eslint/eslint-plugin-react#2866

Expected behavior

The list should be complete.

Alternatives

Use another library, such as one from the list below:

@karlhorky karlhorky added 🙉 open/needs-info This needs some more info 🦋 type/enhancement This is great to have labels Dec 6, 2020
@wooorm
Copy link
Member

wooorm commented Dec 8, 2020

target can’t work, because it’s not an enumerated attribute. It can have other values: https://html.spec.whatwg.org/multipage/browsers.html#valid-browsing-context-name-or-keyword

@karlhorky
Copy link
Author

Oh, oops... is there another package that would handle this?

Or, if it will be extracted to a new package à la #37 , maybe this would be a good addition to the new data package?

@wooorm
Copy link
Member

wooorm commented Dec 8, 2020

ol[type] is not defined as enumerated, but seems to be, so I think I can add that.
ul[type] and li[type] are deprecated, so I can add them here, but for a linter you should probably warn about their use.

loading is a new enumerated attribute and I’ll add them

@wooorm
Copy link
Member

wooorm commented Dec 8, 2020

is there another package that would handle this?

It can have any string (so long as it doesn‘t start with a _), so I don’t think it can be handled? 🤷‍♂️

@wooorm wooorm closed this as completed in a3a64e9 Dec 8, 2020
@wooorm
Copy link
Member

wooorm commented Dec 8, 2020

Released! Thanks!

@wooorm wooorm added ⛵️ status/released 🗄 area/interface This affects the public interface 🦋 type/enhancement This is great to have 🧒 semver/minor This is backwards-compatible change and removed 🙉 open/needs-info This needs some more info 🦋 type/enhancement This is great to have labels Dec 8, 2020
@karlhorky
Copy link
Author

target can’t work, because it’s not an enumerated attribute

So looking at the MDN page, it seems like target has some "enumerated-like" values (which have special meanings):

  • _self (default if missing)
  • _blank
  • _parent
  • _top

I don’t think it can be handled?

So maybe these above could be handled (including the default value if it's missing)?

If you want to separate between "strictly enumerated" and other types, then the object shape for an attribute could have an extra field (eg. type: "enumerated"). This would allow for handling missing value defaults of non-enumerated attributes.

@karlhorky
Copy link
Author

And thanks for the addition + release!

@wooorm
Copy link
Member

wooorm commented Dec 8, 2020

So, for eslint-plugin-react, the thing is that you want to drop _self if it is specified, right?

@karlhorky
Copy link
Author

So, for eslint-plugin-react, the thing is that you want to drop _self if it is specified, right?

Right, as in the issue:

// Incorrect
<form target="_self"></form>

// Correct
<form></form>

@wooorm wooorm reopened this Dec 8, 2020
@wooorm
Copy link
Member

wooorm commented Dec 8, 2020

Alright, so implementing that here makes sense too. It’d mean that “invalid” values are allowed (instead of translated to a different invalid value).
And for the schema, maybe a key allowUnknownStates?

@karlhorky
Copy link
Author

It’d mean that “invalid” values are allowed

Do you mean because attributes like target can also have values not in the states array? If I'm understanding correctly, then yes.

And for the schema, maybe a key allowUnknownStates?

Right, given the rest of the nomenclature in the package, that would seem to make sense.

@wooorm wooorm closed this as completed in 5cb4419 Dec 17, 2020
@wooorm
Copy link
Member

wooorm commented Dec 17, 2020

alright, fixed!

@karlhorky
Copy link
Author

karlhorky commented Dec 21, 2020

Should the missing value for ol[type] be '1' and ul[type] be 'circle'? Seems that way on MDN at least...

@karlhorky
Copy link
Author

Oh and form[target] - should this be '_self'?

@karlhorky
Copy link
Author

karlhorky commented Dec 22, 2020

Going through MDN and the HTML spec, found some more inconsistencies / additions:

  1. [autocomplete]: should this have an array of tagNames? MDN says <input>, <textarea>, <select>, and <form>
  2. img[decoding]: missing value default of 'auto'
  3. [loading] should be ordered after [keytype] and [kind]
  4. [formmethod] should have a missing of null (from the spec: "The formmethod attribute ... has no missing value default."). Maybe also change the comment to update the [method], because it isn't actually 100% synced...
  5. [formenctype] should have a missing of null (from the spec: "The formenctype attribute ... has no missing value default."). Maybe also change the comment to update the [enctype], because it isn't actually 100% synced...
  6. [formtarget] probably should have a missing of null (couldn't find it in the spec, but seems consistent with the others). Maybe also change the comment to update the [target], because it isn't actually 100% synced...
  7. the missing value default for li[type] should maybe be null instead? (from MDN: "This type overrides the one used by its parent
      element, if any.")

    Would you prefer I opened a new issue for these and the other two issues above?


    Additionally, I wonder if it would make sense to add additional information to address the caveat you noted in your comment for attributes like inputMode (where they only apply to elements with either a) presence of an attribute or b) an attribute set to a certain value):

      inputMode: {
        // In fact only applies to `text`, `search`, and `password`.
        tagNames: 'input',

    Eg. either changing tagNames to selectors and allowing a value like ['input[type="text"]', 'input[type="search"]', 'input[type="password"]'], or adding an additional field like tagMatcher, which could allow a more powerful approach like a function ((el) => /text|search|password/.test(el.type)). I suppose the law of least power would suggest something closer to the first approach though.

@karlhorky
Copy link
Author

Ok, opened #38 for the comments above.

@karlhorky
Copy link
Author

And also #39 for the more complex matching conditions mentioned in my comment above.

@wooorm wooorm added the 💪 phase/solved Post is done label Aug 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 💪 phase/solved Post is done 🧒 semver/minor This is backwards-compatible change 🦋 type/enhancement This is great to have
Development

No branches or pull requests

2 participants