Stylelint: Misleading error for declaration-block-no-ignored-properties with display: inline-block and float

[adidahiya]

Describe the issue:

This is a great rule, but I think the rationale behind disallowing display: inline-block + float is slightly incorrect. Adding float makes the inline-block part ignored, not the other way around. The phrasing in rule's current README suggests the inverse behavior -- that display: inline-block causes float to be ignored -- which is incorrect because adding float _does_ affect the appearance of the element.

Which rule, if any, is this issue related to?

declaration-block-no-ignored-properties

What CSS is needed to reproduce this issue?

.foo {
  display: inline-block;
  float: left;
}

What did you expect to happen?

Warning: float causes display: inline-block to be ignored.

What actually happened?

Warning: display: inline-block causes float to be ignored.

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[davidtheclark]

Am I right, @adidahiya, that the rule works as intended, and the problem is just that the error message miscommunicates the reasoning?

[adidahiya]

@davidtheclark yep, that's correct. The rule works but the error message is misleading and produces confusion in how to resolve the lint warning.

[davidtheclark]

Great. Probably a fairly easy bug fix if you or anyone else wants to jump on it.

[alexander-akait]

@adidahiya right, docs are valid or not?

[davidtheclark]

@evilebottnawi I think the docs are fine, just the message is off.

[davidtheclark]

Nevermind! I saw this sentence: "display: inline-block causes float to be ignored." That should be switched around, "float causes display: inline-block to be ignored."

[davidtheclark]

Same goes for "Although display: inline-block causes float to be ignored, block works with float."

[alexander-akait]

@davidtheclark What all of these right? :smile:

[davidtheclark]

It's float that causes display: inline-block to be ignored. I'll work on a fix for this :)

[davidtheclark]

Am I right in thinking that float: left|right causes all display values to be ignored? It forces a block box, I believe. (https://www.w3.org/TR/CSS21/visuren.html#floats)

If that's right, then we need to adjust the rule and it will be a breaking change. :(

[davidtheclark]

Or else we could patch this particular issue up temporarily, in a hacky way, and then do the real fix in v7.

[adidahiya]

To confirm my understanding of the breaking change:

Currently the lint failure is on the float property: Unexpected property "float" that is ignored because of "display: inline-block".

The fix is to move the failure location to the display property.

If users have been disabling the rule inline like this, then they will get lint errors if we implement the fix:

.foo {
  /* stylelint-disable */
  float: left;
  /* stylelint-enable */
  display: inline-block;
}

In that case, yes, +1 for a hacky fix right now and a real fix in the next major version.

[alexander-akait]

@davidtheclark i think we should resolve this issue in 7.0.0

[alexander-akait]

@davidtheclark not critical bug nows

[davidtheclark]

Agreed. Will mark it with the milestone.

[davidtheclark]

Looked into this a little, and found that there is some complexity to it.

float: left and float: right will cause all display values except display: none to be ignored. Looks to me like the system in the rule right now does not allow for this situation: the property display is not ignored, because none can be important; but most of display's possible values _are_ ignored.

This isn't _super_ bad, but I was already kind of worried about this rule, and it got me thinking — part of me wonders if we might not want to extract such a complex and potentially error-prone rule into an external plugin, and we were too quick to accept it into core. I feel like we should have done that with the properties ordering rule —— we still could, in v7. The reasoning is not that it's best for stylelint, so much that it's best for me and other maintainers. I'm certainly at the limit of the amount of time that I'm able to put into stylelint, and that limit might decrease soon (I'm starting a new job). So that's why I'm thinking seriously about extracting maintenance-heavy rules (as I imagine this one might be) into external plugins instead of continuing to pile them into core.

What do you think @stylelint/core? There's more discussion to be had here.

[davidtheclark]

After having a glass of water I've changed my mind a little.

The problem with the properties-order rule is that it may _always have shortcomings in many user's eyes_. It can't be "finished", the way something like declaration-no-important can be.

This rule is different. It can be "finished", in theory. So in that sense it's a better rule to keep in core. I guess because of the situation above some refactoring might be needed for v7, but that's fine.

However, this rule does have its own problem — the problem of https://github.com/stylelint/stylelint/issues/1236. Whenever we find holes in the rule, plugging them is a breaking change to stylelint. Maybe for that reason this should be a plugin with 0.x versions until it stabilizes, and _then_ we pull it back into core?

[jeddy3]

The reasoning is not that it's best for stylelint, so much that it's best for me and other maintainers. I'm certainly at the limit of the amount of time that I'm able to put into stylelint, and that limit might decrease soon (I'm starting a new job).

This is a very valid consideration. I'm in a similar boat. I'm going to be returning to full-time work soon. It's very likely I'll need to limit my stylelint hours. As an aside, it feels a little like a mini-era is coming to an end. We've put so much into stylelint this last year, and now it feels like we're entering a new stage. I think that this is the time to think about how the project can be _sustainably_ maintained going forward.

The problem with the properties-order rule is that it may _always have shortcomings in many user's eyes_. It can't be "finished", the way something like declaration-no-important can be. ...

This makes _a lot_ of sense to me. I think this additional distinction between core rules or a plugins will help us keep things manageable.

I feel like we should have done that with the properties ordering rule —— we still could, in v7.

I think the time is right to do this.

We can still have a _stripped down_ declaration-block-properties-order rule in core, but it only does what the name suggests i.e. checks that properties are in a _specific_ order. A plugin (perhaps called something more open-ended like block-structure) could house things like (flexible and strict) groups, spacing between groups, spacing between declarations, order of nested statements (at-rules & rules etc), and so on.

This rule [declaration-block-no-ignored-properties] is different. It can be "finished", in theory. So in that sense it's a better rule to keep in core.

Agreed.

Maybe for that reason this should be a plugin with 0.x versions until it stabilizes, and then we pull it back into core?

Agreed. I think this should be the standard procedure for any rule who end-state isn't _absolutely clear_. I think declaration-no-invalid proposal is the perfect candidate for this. I think the only other proposal that isn't absolutely clear is declaration-block-no-misaligned-values. As "misaligned" is ambiguous e.g. I can we someone requesting this:

/* alignment based on "property groups" */
a {
  top:    10px;
  bottom: 5px;

  font-family: arial;
  color:       red;
}

Do we still feel that declaration-block-no-ignored-properties's end-state is unclear? If so, then I agree we should pull it out of core in v7. If we are _confident_ it's finished, then I'm happy to see it stay.

I've taken a look through the rules and pretty much everything else looks "finished" _and with a clear end-state_. The only questionable rule is indentation. So far we've added new features (e.g. checking inside of parens) behind options. There's an upcoming option to indent each block's closing brace. Do we pull this into a plugin?

---

I'm off to Nicaragua on Monday for a little under 3 weeks. I'm going to be offline during the week, but I hope to check-in on the issues on the Saturdays (but without my laptop, so I won't be coding). As always, feel free to merge and release as you see fit.

[davidtheclark]

I'm off to Nicaragua on Monday for a little under 3 weeks.

Cool! I will also be less available for 3 weeks starting Monday. I'm going backpacking for one week and then spending 2 weeks traveling to start my new job. Let's try to make sure we have things in a stable state before Monday, then!

We can still have a stripped down declaration-block-properties-order rule in core, but it only does what the name suggests i.e. checks that properties are in a specific order.

I'm in favor of this — simplifying declaration-block-properties-order in core and extracting the rest into a plugin. We'll have to find a maintainer for that plugin.

Do we still feel that declaration-block-no-ignored-properties's end-state is unclear

I don't think the end-state is unclear, I just think it's hard to reach, because there's a danger that there will always be some value-combination that we've overlooked. Depending on how confident @evilebottnawi is in the current state of the rule we can consider putting it into a plugin _temporarily_. The advantage of putting that rule into a plugin is that it can then move through its own breaking changes as needed _until it is stable_, instead of destablizing core. We can keep that in mind in the future if a complex, demanding rule like this, which might be unstable for a period, comes up.

The only questionable rule is indentation

I think indentation is necessary enough that we need it in core. But I wonder if we might consider extracting the hierarchical selectors part into a plugin, since that is specialized.

[alexander-akait]

@jeddy3 i have idea how rewrite rule more correct and include all properties and values from specification. I think this rule like no-unsupported-browser-features. no-unsupported-browser-features based on caniuse and one time have no errors, after little time throw errors (and i think it is normal), we have severity and options for CI be green. declaration-block-no-ignored-properties i planned rewrite on css spec and sometimes it throw errors too.

[alexander-akait]

@davidtheclark i agree with indentation we should extracting the hierarchical selectors part into a plugin.

[jeddy3]

I'm going backpacking for one week and then spending 2 weeks traveling to start my new job.

Nice! Have a great time :)

But I wonder if we might consider extracting the hierarchical selectors part into a plugin, since that is specialized.

Sgtm.

Let's try to make sure we have things in a stable state before Monday, then!

Agreed.

[alexander-akait]

@davidtheclark @jeddy3 Who will review my code? :sob:

[davidtheclark]

Do we want to try to get v7 out before next Monday, or should we hold off on that because it's a little dangerous?

[davidtheclark]

I'm thinking maybe we should hold off on it.

[jeddy3]

I'm thinking maybe we should hold off on it.

I'm down with this. Shall we add deprecation warnings before Monday though? Perhaps giving someone the chance to start on, for example, the block-structure plugin in our absence? We can take v7 leisurely upon our return.

@davidtheclark @jeddy3 Who will review my code?

@evilebottnawi You can take some time off stylelint too, if you like? :) We can pick things back up in a couple of week's time.

[alexander-akait]

@davidtheclark @jeddy3 v7 can wait. It is really dangerous.

[ntwb]

Holidays and jobs, awesome 🎉

Maybe wait a few weeks before dropping v7, no need to hurry it out the door 😄

[nstepien]

Hi, I'd just like to add that something like float: none; and display: inline-block; should be allowed together.

[alexander-akait]

@MayhemYDG in 7.0.0 this fixed

[octoxan]

Hi, I'd just like to add that something like float: none; and display: inline-block; should be allowed together.

Completely agreed. That one drives me crazy. I like having 0 errors in my files and sometimes one or two are forced to stick around for this reason.

[jeddy3]

@octoxan If you'd like to see the issue resolved, please consider contributing this fix. In the meantime you can either turn the rule off in your config, or use the /* stylelint-disable */ comments.

[octoxan]

@jeddy3 I'll look into it this weekend for sure. Wish me luck lol

[jeddy3]

There's a "Working with rules" section in the developer guide that'll help you get started. Good luck :)

[jeddy3]

Done in #1610

[CheungJ]

Nice! Thanks @jeddy3

[octoxan]

Thank gods! I was so tired of seeing that float: none can't be used with display: inline-block. lol

[jeddy3]

Nice! Thanks @jeddy3

@evilebottnawi put the legwork into shoring up this rule.