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.

Sign up for GitHub

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

Jump to bottom

Fix declaration-block-no-redundant-longhand-properties false negatives for *-block and *-inline logical properties #7200

Closed
smfaramarzirad opened this issue Sep 24, 2023 · 10 comments · Fixed by #7208
Closed

Fix declaration-block-no-redundant-longhand-properties false negatives for *-block and *-inline logical properties #7200

smfaramarzirad opened this issue Sep 24, 2023 · 10 comments · Fixed by #7208
Labels
status: wip is being worked on by someone type: bug a problem with a feature or rule

Comments

@smfaramarzirad
Copy link

smfaramarzirad commented Sep 24, 2023 •
edited

What steps are needed to reproduce the bug?

margin-block-start: 2px;
margin-block-end: 2px;
margin-inline-start: 2px;
margin-inline-end: 2px;

What did you expect to happen?

margin-block: 2px;
margin-inline: 2px;

What actually happened?

margin-block-start: 2px;
margin-block-end: 2px;
margin-inline-start: 2px;
margin-inline-end: 2px;
@ybiquitous
Copy link
Member

@smfaramarzirad Thanks for the report. The current declaration-block-no-redundant-longhand-properties rule doesn't support the margin-block and margin-inline shorthand properties (see also the demo), but adding the support for these properties sounds good to me.

I'm transferring this issue to the Stylelint core repository since this repository is for the shared config.

@ybiquitous ybiquitous transferred this issue from stylelint/stylelint-config-standard Sep 25, 2023
@ybiquitous ybiquitous changed the title declaration-block-no-redundant-longhand-properties does not work in logical properties Add margin-block and margin-inline support for declaration-block-no-redundant-longhand-properties Sep 25, 2023
@ybiquitous ybiquitous added status: ready to implement is ready to be worked on by someone type: bug a problem with a feature or rule labels Sep 25, 2023
@ybiquitous ybiquitous changed the title Add margin-block and margin-inline support for declaration-block-no-redundant-longhand-properties Fix declaration-block-no-redundant-longhand-properties false negatives for margin-block and margin-inline properties Sep 25, 2023
@ybiquitous
Copy link
Member

CanIuse stats:

I think they have enough usage to be supported.

FYI. inset also has the usage rate (90.69%).

I've labeled the issue as ready to implement. Please consider contributing if you have time.

There are steps on how to fix a bug in a rule in the Developer guide.

@mattxwang Feel free to comment if you have any help.

@Mouvedia
Copy link
Member

Mouvedia commented Sep 25, 2023 •
edited

Those are not covered as well:

  • overflow
  • background-position
  • border-inline-width
  • border-block-width
  • offset-position
  • overscroll-behavior

see #6984 (comment)

@ybiquitous
Copy link
Member

@Mouvedia Thanks for the info. All of the properties except offset-position have enough usage stats.

For offset-position, it may be better to avoid implementing it since the property is still experimental. See the CanIuse stats.

It's no problem to include also the additional properties in one PR. 👍🏼

@smfaramarzirad
Copy link
Author

CanIuse stats:

* [`margin-block`](https://caniuse.com/mdn-css_properties_margin-block) - 90.69%

* [`margin-inline`](https://caniuse.com/mdn-css_properties_margin-inline) - 90.69%

I think they have enough usage to be supported.

FYI. inset also has the usage rate (90.69%).

I've labeled the issue as ready to implement. Please consider contributing if you have time.

There are steps on how to fix a bug in a rule in the Developer guide.

@mattxwang Feel free to comment if you have any help.

In my country (Iran), more than 95% of browsers support margin-block.

The issue is not only margin-inline and margin-block. There are other logical features such as:

border-block, border-inline,
border-inline-color, border-inline-style, border-inline-width, padding-block, padding-inline, inset-block, inset-inline, scroll-margin-block, scroll-margin-inline, scroll-padding-block, scroll-padding-inline.

@mattxwang
Copy link
Member

Thanks for tagging me in! Course prep has been unfortunately busier than I thought, but if nobody else is interested in this PR, I can certainly add it to my own backlog. I can be conservative for now in the properties we support.

I've also been meaning to resolve some of the longstanding issues with the autofix - I suspect there are still a few incorrect behaviours (e.g. with grid) that nobody has reported since having all of the longhand properties is somewhat unlikely. I'll try to also balance that with the messageArgs work!

@ybiquitous ybiquitous changed the title Fix declaration-block-no-redundant-longhand-properties false negatives for margin-block and margin-inline properties Fix declaration-block-no-redundant-longhand-properties false negatives for *-block and *-inline logical properties Sep 27, 2023
mattxwang added a commit that referenced this issue Oct 2, 2023
@mattxwang
Fix declaration-block-no-redundant-longhand-properties false negati…
9835633 
…ves for `margin-block` and `margin-inline`

This is the original ask in #7200.
@mattxwang mattxwang mentioned this issue Oct 2, 2023
Merged
12 tasks
@ybiquitous ybiquitous added status: wip is being worked on by someone and removed status: ready to implement is ready to be worked on by someone labels Oct 3, 2023
mattxwang added a commit that referenced this issue Oct 4, 2023
@mattxwang
Fix declaration-block-no-redundant-longhand-properties false negati…
4bfee38 
…ves for `*-block` and `*-inline` logical properties (#7208)

Closes #7200.

> Is there anything in the PR that needs further explanation?

Bundling many properties into this PR as suggested in the original issue. I'm only including the ones that are trailed with `-block` and `-inline` (as scoped in the original issue title); I will address further false negatives in another PR.

I've chosen to base this off of `main` since:

- we can probably publish a patch off of this
- I don't think this'll introduce many (or any?) merge conflicts with `v16`

Affected properties:

- [x] `border-block`
- [x] `border-inline`
- [x] `inset-block`
- [x] `inset-inline`
- [x] `scroll-margin-block`
- [x] `scroll-margin-inline`
- [x] `scroll-padding-block`
- [x] `scroll-padding-inline`
- [x] `margin-block`
- [x] `margin-inline`
- [x] `padding-block`
- [x] `padding-inline`
@mattxwang
Copy link
Member

@Mouvedia I'm going through the list you had earlier, and wanted to ask:

  • for background-position: is this a shorthand? I see how we could translate background-position-x and -y into the "shorthand" version, but it doesn't seem to indicate that this is a true shorthand property in the spec (draft)
  • for border-image-width`: what would this be a shorthand for? I don't see any candidate longhand properties / anything in the spec, but perhaps I'm missing something

(MDN also has a handly list that I'm using, though it is incomplete)

@mattxwang mattxwang mentioned this issue Oct 6, 2023
Merged
8 tasks
@Mouvedia
Copy link
Member

Mouvedia commented Oct 6, 2023 •
edited

true shorthand property

I don't know what you mean by true though.
If 2, or more, declarations can be converted to less—currently only one—declarations, I consider it warrants to be added.

for border-image-width: what would this be a shorthand for? I don't see any candidate longhand properties / anything in the spec, but perhaps I'm missing something

I am not sure, I can't remember; I may have made a mistake. Ill remove it from the list.

@mattxwang
Copy link
Member

mattxwang commented Oct 6, 2023 •
edited

true shorthand property

I don't know what you mean by true though. If 2, or more, declarations can be converted to less—currently only one—declarations, I consider it warrants to be added.

Two things on my mind:

  • the word "shorthand" doesn't appear anywhere in the spec for background-position; usually, in other shorthand a line like "This property is a shorthand property that" exists
  • the draft spec seems to still be a work in progress:

This section is still being worked out. The tricky thing is making all the start/end keywords work sanely.

From my understanding of the spec, it's also not straightforward to convert two of the longhands into the background-position shorthand, since many different types of values are allowed in both longhands (which require some custom resolving code).

@mattxwang mattxwang mentioned this issue Oct 7, 2023
Merged
@Mouvedia
Copy link
Member

Mouvedia commented Oct 7, 2023

it's also not straightforward

Not always indeed; it doesn't have to. We are not in a hurry to support all shorthands.
Let's resolve the easy ones first and then decide whether the harder ones are still in scope for the rule.
I would say the only reason Id be against it, is if it had a substantial effect on performance.

mattxwang added a commit that referenced this issue Oct 7, 2023
@mattxwang
Fix declaration-block-no-redundant-longhand-properties false negati…
3374bb4 
…ves for `overflow`, `overscroll-behavior`, `scroll-margin`, `scroll-padding`, and new Box Alignment shorthands (#7213)

Ref: #7200 (comment) & other missing shorthands.

Wasn't sure about `background-position` or `border-image-width` in the linked issue, so I left a comment.

Current properties:

- [x] `gap`: [94% on caniuse](https://caniuse.com/mdn-css_properties_gap)
- [x] `overflow`
- [x] `overscroll-behavior`
- [x] `place-content`: [95% on caniuse](https://caniuse.com/mdn-css_properties_place-content)
- [x] `place-items`: [95% on caniuse](https://caniuse.com/mdn-css_properties_place-items)
- [x] `place-self`: [95% on caniuse](https://caniuse.com/mdn-css_properties_place-self)
- [x] `scroll-margin` (we already supported the `block`/`inline` properties)
- [x] `scroll-padding` (we already supported the `block`/`inline` properties)

Properties in the [shorthand list](https://developer.mozilla.org/en-US/docs/Web/CSS/Shorthand_properties#see_also) that I'm choosing not to do (but could bundle in?):

- `container`: [88% on caniuse](https://caniuse.com/mdn-css_properties_container). I think this is supported by all major browsers, but is slightly smaller than the other shorthands we've supported
- `contain-intrinsic-size`: [74% on caniuse](https://caniuse.com/mdn-css_properties_contain-intrinsic-size), which seems low?
- `font-synthesis`: [92% on caniuse](https://caniuse.com/mdn-css_properties_font-synthesis), but the way this shorthand behaves is a bit different than the standard shorthand (it's almost like a flag?) so I'll do this in a different PR
- `font-variant`: [96% on caniuse](https://caniuse.com/mdn-css_properties_font-variant) same as `font-synthesis` re: shorthand behavior
- `scroll-timeline`: [62% on caniuse](https://caniuse.com/mdn-css_properties_scroll-timeline), definitely too low!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
status: wip is being worked on by someone type: bug a problem with a feature or rule
Milestone
No milestone
4 participants
@Mouvedia @ybiquitous @mattxwang @smfaramarzirad