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

[postcss-nesting] issues with :is pseudo selector #195

Closed
2 of 3 tasks
lubomirblazekcz opened this issue Jan 22, 2022 · 14 comments
Closed
2 of 3 tasks

[postcss-nesting] issues with :is pseudo selector #195

lubomirblazekcz opened this issue Jan 22, 2022 · 14 comments

Comments

@lubomirblazekcz
Copy link

lubomirblazekcz commented Jan 22, 2022

Bug description

The :is selector is working quite strangly for me, since the update I would rather use noIsPseudoSelector: true, which I think should have been default, it is quite a breaking change and there are lot of unexpected results. See below, this is when using noIsPseudoSelector: false

Source CSS

.ui-input {
  & :is(input, textarea, .input) {
    @nest :is([data-icon*="left"])& {
      padding-left: 1rem;
    }
}

or

.ui-input {
  & :is(input, textarea, .input) {
    @nest :not([data-state*="placeholder"])& {
      &[placeholder] {
        padding-top: 1rem;
      }
    }
}

Expected CSS

.ui-input:is([data-icon*="left"]) :is(input,textarea,.input)

or

.ui-input:not([data-state*="placeholder"]) [placeholder]:is(input, textarea, .input)

Actual CSS

:is([data-icon*="left"]):is(.ui-input :is(input,textarea,.input))

or

[placeholder]:not([data-state*="placeholder"]):is(.ui-input :is(input, textarea, .input))

Does it happen with npx @csstools/csstools-cli <plugin-name> minimal-example.css?

No response

Extra config

No response

What plugin are you experiencing this issue on?

PostCSS Nesting

Plugin version

10.1.2

What OS are you experiencing this on?

macOS, Windows

Node Version

16.13.1

Validations

  • Follow our Code of Conduct
  • Check that there isn't already an issue that request the same feature to avoid creating a duplicate.

Would you like to open a PR for this bug?

  • I'm willing to open a PR
@romainmenke
Copy link
Member

Hi @lubomirblazekcz!

Thank you for bringing this up!

Wrapping the parent selector with :is is actually the only correct way to implement nesting according to the spec.

The nesting spec can be a bit counter intuitive because it is not a simple find/replace where the parent selector is inserted for each &.

In general it is best to avoid nesting too deeply or using complex selectors (those using combinators like space). Using nesting selectors like your example will also not work correctly in older browsers.

The correct source CSS would be :

.ui-input {
  & :is(input, textarea, .input) {
    @nest :is([data-icon*="left"]) & {
      padding-left: 1rem;
    }
}

Which becomes :

:is([data-icon*="left"]) :is(.ui-input :is(input,textarea,.input))

The space before & is important as :is(.ui-input input) matches input and not .ui-input.


These changes were introduced in postcss-preset-env and postcss-nesting as breaking changes. Having a correct implementation that follows the spec as closely as possible is important for when browsers support nesting natively.

@lubomirblazekcz
Copy link
Author

@romainmenke thanks for clearing this up, I've been used to use & to match the top parent selector, which works with noIsPseudoSelector: true. But I can see now that isn't actually part of the spec :/

I am actually surprised that :is works like this, with the space us you showed me now. Guess I have some rewriting to do 😅

@lubomirblazekcz
Copy link
Author

Hmm after further analyzing, I would expect the output be following

:is([data-icon*="left"]) .ui-input :is(input, textarea, .input)

This is how I would expect it to work according the spec. Which wouldn't be obviously good, because there would be no way to match top-parent selector after further nesting. I might be wrong, but this is something that is not very clear in spec. Or maybe I've missed that :is magic somewhere in the spec.

@romainmenke
Copy link
Member

romainmenke commented Jan 22, 2022

Will try to write down how the spec is defined (afaik, and I might be wrong) and how the plugin works.

The relevant spec section is here : https://www.w3.org/TR/css-nesting-1/#nest-selector

for simplicity I am omitting the is(input, textarea, .input) as it doesn't affect output

.ui-input {
  & input {
    @nest :is([data-icon*="left"]) & {
      padding-left: 1rem;
    }
  }
}
  1. take the first nested selector :
& input
  1. wrap the parent with :is() and replace & with result
:is(.ui-input) input
  1. the plugin has a heuristic to skip :is() when the parent is a <simple-selector>

so actual output is .ui-input input

At this point the selector looks like this :

.ui-input input {
  @nest :is([data-icon*="left"]) & {
    padding-left: 1rem;
  }
}
  1. take the second nested selector :
:is([data-icon*="left"]) &
  1. wrap the parent with :is() and replace & with result
:is([data-icon*="left"]) :is(.ui-input input)

This matches an element with tag name input having an ancestor matching [data-icon*="left"] and an ancestor matching .ui-input. These ancestors do not need to be the same element but can be.


The same logic without the space :

.ui-input {
  & input {
    @nest :is([data-icon*="left"])& {
      padding-left: 1rem;
    }
  }
}
  1. take the first nested selector :
& input
  1. wrap the parent with :is() and replace & with result
:is(.ui-input) input
  1. the plugin has a heuristic to skip :is() when the parent is a <simple-selector>

so actual output is .ui-input input

At this point the selector looks like this :

.ui-input input {
  @nest :is([data-icon*="left"])& {
    padding-left: 1rem;
  }
}
  1. take the second nested selector :
:is([data-icon*="left"])&
  1. wrap the parent with :is() and replace & with result
:is([data-icon*="left"]):is(.ui-input input)

Which can be rewritten as :

  1. :is(.ui-input input):is([data-icon*="left"]) (two :is() functions in a compound can be switched)
  2. :is(.ui-input input)[data-icon*="left"] (:is() with only a <simple-selector> is can be reduced)
  3. .ui-input input[data-icon*="left"]

This last step (3) can't be easily abstracted into plugin logic but a complex selector in :is() followed by a simple selector is also equivalent to the same selector without :is()

This matches an element with tag name input and attribute data-icon*="left" having an ancestor that matches .ui-input.


Can you pinpoint which step in this logic is unexpected? :)

If at any point I say something that doesn't match the actual plugin output for a given source, please call this out. This is complicated stuff and I might be wrong about things

@lubomirblazekcz
Copy link
Author

The 5. is unexpected to me, why should be the parent wrapped with :is() in that case? The only wraping mentioned in the spec is folowing, which make sense to me. But some part in the spec are complicated, and I am also not sure if I understand it all corectlly.

a, b {
  & c { color: blue; }
}

:is(a, b) c { color: blue; }

Anyway, what was my main concern was that it was possible to select the grandparent selector prior the :is update (with @nest and & without space), but that's not the case anymore with noIsPseudoSelector: false.

I was not aware that it was not part of the spec, so this might be something I should open an issue at w3c/csswg-drafts, because selecting grandparent selector might be quite important in some cases.

@lubomirblazekcz
Copy link
Author

Currently with postcss gradparent can be selected folowing way, but as I said - I am not sure if that is how it was intended in the spec and it's also not ideal to repeat the grandparent selector in the @nest

.a {
  color: blue;
  & .b {
    @nest .a:not(.c) & {
      color: red
    }
  }
}

.a:not(.c) :is(.a .b) {
    color: red;
}

@romainmenke
Copy link
Member

romainmenke commented Jan 22, 2022

As far as I know grandparent selection was never part of the spec and if the plugin worked like this before it was likely a bug :/

I would also advice against this style but that is my own opinion :)

I think the intention behind the spec was to enable logical grouping of styles in a block like syntax.

.element {
  color: red;
  width: 50px;

  &:hover {
    color: blue;
  }

  @media (min-width: 520px) {
    & {
      width: 100px;
    }
  }
}

In reality much more complex things are done with it and sass like nesting is in no way spec compliant but it did set the expectations of the feature.

This plugin attempts to follow the spec but this is only possible by wrapping each parent with :is() for each level of nesting.

As :is() support is still not great in slightly older browsers this is far from ideal and we try to omit :is() where possible to increase support.

@lubomirblazekcz
Copy link
Author

If older browsers you mean Safari, then yes 😅

To be honest I am also againts deep and complex nesting, but there are many cases when you get nested into second level and using grandparent selector in the second level is much cleaner syntax then using it at first level.

And I never thought it was a bug, because less works exactly the same way :D

@romainmenke
Copy link
Member

Chrome also less than a year ;)
Almost the same time of implementation as Safari.

In the 7.3 release we will add a plugin to transform :is.
This comes with an option to emit warnings when certain selectors will behave differently in older browsers.

Using this config then might be interesting :

postcssPresetEnv({
  features: {
    'is-pseudo-class': {
      onComplexSelector: 'warning',
    },
    'nesting-rules': {
      noIsPseudoSelector: false,
    }
  }
})

This combo will ensure that your source code will never have unexpected results in older browsers. (if you resolve the warnings)

@lubomirblazekcz
Copy link
Author

Chromium browsers have the advantage of better update cycle, thanks for the info though

@romainmenke
Copy link
Member

@lubomirblazekcz Closing this as the plugin works as intended in this case.
Thank you again for bringing this up, always insightful to get feedback!

If the spec does change or if you encounter further issues please let us know, we will happily look into these!

@shye0000
Copy link

shye0000 commented Feb 11, 2022

Cool, looking forward to the v7.3 release, Im having the same issue too, and using noIsPseudoSelector: true as a tempo solution for old browsers that we claimed to "support" :p

@romainmenke
Copy link
Member

@shye0000 Can you clarify your comment? :)

7.3 was released and includes a plugin to partially resolve :is.

Important notes : https://github.com/csstools/postcss-plugins/tree/main/plugins/postcss-is-pseudo-class#%EF%B8%8F-known-shortcomings

@shye0000
Copy link

@romainmenke ah my bad, didn't notice all that, Ill try that, thx again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants