Add `"ignoreLonghands": []` to `declaration-block-no-redundant-longhand-properties`

[Mouvedia]

use cases

• text-decoration
• background

rationale

If a browser doesn't support the new arity for a given shorthand, it will drop it altogether. For example if we were to introduce an enhancement for the current fixer of text-decoration by adding support to text-decoration-thickness, for a lot of users of the rule it would introduce a hard to detect regression.
They would have to rely on "ignoreShorthands": ["text-decoration"] which is arguably a downgrade compared to the status quo. Hence a new option is required to be more granular and cross-browser.

text-decoration

rationale for new fix

https://github.com/stylelint/stylelint/pull/6580#discussion_r1095994177 #7060

without option

a {
  text-decoration-line: underline;
  text-decoration-style: solid;
  text-decoration-color: green,
  text-decoration-thickness: 1px;
}

becomes

a {
  text-decoration: underline solid green 1px;
}

with "ignoreLonghands": ["text-decoration-thickness"]

a {
  text-decoration-line: underline;
  text-decoration-style: solid;
  text-decoration-color: green,
  text-decoration-thickness: 1px;
}

becomes

a {
	text-decoration: underline solid green;
	text-decoration-thickness: 1px;
}

references

https://bugs.webkit.org/show_bug.cgi?id=190774 https://drafts.csswg.org/css-text-decor-4/#text-decoration-property

implementation details

Firefox 69 supports this feature (preffed off) under the name text-decoration-width, behind the pref layout.css.text-decoration-width.enabled. text-decoration-skip is not part of the shorthand but will have to be eventually added in a separate PR.

browser support

https://caniuse.com/?search=text-decoration-thickness

background

rationale

https://caniuse.com/background-img-opts

example

{
  "fix": true,
  "rules": {
    "declaration-block-no-redundant-longhand-properties": [
      true,
      { "ignoreLonghands": ["background-size", "background-origin", "background-clip"] }
    ],
  }
}

a {
  background-image: none;
  background-position: 0% 0%;
  background-size: auto auto;
  background-repeat: repeat;
  background-origin: padding-box;
  background-clip: border-box;
  background-attachment: scroll;
  background-color: transparent;
}

becomes

a {
  background: repeat scroll 0% 0% none transparent;
  background-size: auto auto;
  background-origin: padding-box;
  background-clip: border-box;
}

[Mouvedia]

Iv reverted to status: needs discussion because on a browser that doesn't support text-decoration-thickness

a {
  text-decoration-thickness: 1px;
}

will simply be ignored, but

a {
  text-decoration: underline solid green 1px;
}

will be dropped altogether because it will consider it invalid. i.e. style, line and color will be ignored as well

tl;dr: if it's not configurable (pick between 3 or 4) it's not worth adding IMHO i.e. it would require ignoreProperty: ["/regex/", /regex/, "string"] e.g. ignoreProperty: ["text-decoration-thickness"]

[ybiquitous]

Hum, I think it's still worth to support text-decoration-thickness in the following case:

a {
	text-decoration-line: underline;
	text-decoration-style: solid;
	text-decoration-color: green;
	text-decoration-thickness: 1px;

	/* ↓↓↓ autofixed */

	text-decoration: underline solid green 1px;
}

At the same time, we'd like to keep the current behavior if text-decoration-thickness is absent:

a {
	text-decoration-line: underline;
	text-decoration-style: solid;
	text-decoration-color: green;

	/* ↓↓↓ autofixed */

	text-decoration: underline solid green;
}

Can we achieve this by the rule's implementation? @mattxwang

[Mouvedia]

That's possible, but as I have explained in the previous comment, it is not worth adding unless we have a new option that let's us ignore text-decoration-thickness for this rule. e.g. if ignored

a {
	text-decoration-line: underline;
	text-decoration-style: solid;
	text-decoration-color: green;
	text-decoration-thickness: 1px;
}

would autofix to

a {
	text-decoration: underline solid green;
	text-decoration-thickness: 1px; // only this line is dropped if the browser doesn't support it
}

i.e. the existing ignoreShorthand: : ["text-decoration"] is not granular enough and would be considered a downgrade compared to the status quo.

[ybiquitous]

Currently, caniuse shows 93% coverage at the global:

It may make sense to wait until this coverage increases more. We can revisit this issue when the coverage is enough.

[Mouvedia]

It wouldn't change my opinion: it requires a new option. I wish the option I proposed would have other use cases though.

[ybiquitous]

@Mouvedia You mean a new option like ignoreProperties, right? To be honest, I'm concerned that a new option may be confused with ignoreShorthands.

[Mouvedia]

@ybiquitous ignoreLonghands wouldn't be confusing. The problem is not the name, it's that it wouldn't be warranted if we only had one use case. But I found another one: background used to be the shorthand of 5 properties, but now it's 8. i.e. ignoreLonghands: ["background-size", "background-origin", "background-clip"]

Should I create a new issue for that option?

[ybiquitous]

Make sense. Adding a new ignoreLonghands option sounds good to me. 👍🏼

Should I create a new issue for that option?

No need. Let's continue the discussion here so that we can easily understand the context.

So could you update the issue title and comment on a blueprint for the new option?

[mattxwang]

Ah! I think this issue slipped by me earlier. This is certainly doable, though it'll require quite a bit of refactoring to get it to work nicely with the existing approach. After v16, I am happy to work on this a bit (since, for better or worse, I've been heavily involved with this rule and its autofix).

[github-actions[bot]]

This issue is older than one month. Please ask before opening a pull request, as it may no longer be relevant.

[Mouvedia]

After v16, I am happy to work on this a bit (since, for better or worse, I've been heavily involved with this rule and its autofix).

@mattxwang are you still planning to work on it? @ybiquitous I might work on it if @mattxwang doesn't assign himself; can we switch to ready to implement?

[mattxwang]

@Mouvedia go for it, I've been swamped with work lately and haven't had the chance to work on Stylelint :(

[ybiquitous]

@Mouvedia It still makes sense to implement the new option ignoreLonghands. Let's switch to "ready to implement".

@jeddy3 Please comment if you have any concerns.

[Mouvedia]

Based on @jeddy3's 👍 I've labeled the issue as ready to implement. Please consider contributing if you have time.

There are steps on how to add a new option in the Developer guide.

[github-actions[bot]]

This issue is older than one month. Please ask before opening a pull request, as it may no longer be relevant.

[Mouvedia]

a {
	text-decoration-line: underline;
	text-decoration-style: solid;
	text-decoration-color: green;

	/* ↓↓↓ autofixed */

	text-decoration: underline solid green;
}

This is certainly doable, though it'll require quite a bit of refactoring to get it to work nicely with the existing approach. After v16, I am happy to work on this a bit (since, for better or worse, I've been heavily involved with this rule and its autofix).

@mattxwang are you still interested in implementing that enhancement? i.e. autofix with optional longhands missing If so can you create an issue?

[mattxwang]

@mattxwang are you still interested in implementing that enhancement? i.e. autofix with optional longhands missing If so can you create an issue?

I don't think I'd be able to dedicate time to this until the summer (~June) after I'm done teaching. If you'd like to take it on, go for it!

(would you still want me to draft up an issue)?

[Mouvedia]

(would you still want me to draft up an issue)?

When you will have time.