rehypejs / rehype-minify

plugins to minify HTML
https://unifiedjs.com
MIT License
89 stars 16 forks source link

Add more "missing value defaults" #36

Closed karlhorky closed 3 years ago

karlhorky commented 3 years ago

Hi @wooorm!

Subject of the feature

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

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:

https://github.com/yannickcr/eslint-plugin-react/issues/2866

Expected behavior

The list should be complete.

Alternatives

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

wooorm commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Released! Thanks!

karlhorky commented 3 years ago

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):

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 commented 3 years ago

And thanks for the addition + release!

wooorm commented 3 years ago

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

karlhorky commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

alright, fixed!

karlhorky commented 3 years ago

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

karlhorky commented 3 years ago

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

karlhorky commented 3 years ago

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 commented 3 years ago

Ok, opened #38 for the comments above.

karlhorky commented 3 years ago

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