less / less.js

Less. The dynamic stylesheet language.
http://lesscss.org
Apache License 2.0
17.03k stars 3.41k forks source link

Reflecting current set of CSS selectors : context.selectors not being carried over in eval contexts #2619

Open rjgotten opened 9 years ago

rjgotten commented 9 years ago

In working on some complex framework components written in Less, I have a few times already come across situations where it would have been beneficial to be able to reflect the current set of CSS selectors into a variable for processing.

I've succesfully built a custom function that does just this, but it runs afoul of an issue deeper in the Less compiler.

My usecase

My current case and the one I'll treat here as an example for the reflection feature and how it leads up to the problem with the compiler, is a stylized button that contains both a left/right positioned font-icon and a text span that has horizontal ellipsis applied for overflow and can have its own text left/right/center aligned within the remaining space on the button. E.g.

<button type="button" class="imp-button" data-iconpos="left">
  <i class="imp-icon" data-icon="check"></i>
  <span class="imp-button-text">OK</span>
</button>

Currently, I have a mixin which renders the base of the button's CSS; handling things like stripping default button styles, setting proper padding, line-height, etc. and handling the quirks of ::moz-focus-inner on older Firefox builds.

As part of this mixin, I have to set both CSS for the button itself and for the text span inside it and both class names have a shared root in the name. (OOCSS principles. This pattern is also re-used across multiple of our sites.)

No problem right?

.lib-button(@height; @hor-padding) {
   // Set some properties for the main button here

   & > &-text {
     // And for the text span here.
   }
}

Call via:

.imp-button {
  .lib-button(40px, 10px);
}

And done:

.imp-button { }
.imp-button > .imp-button-text {}

Where my usecase breaks

This is a nice pattern, but it still breaks when we subclass the button, e.g. make a compact variation:

.imp-button--compact {
  .lib-button(20px, .5px);
  font-size : 10px;
  // etc.
}
.imp-button--compact { }
.imp-button--compact > .imp-button--compact-text {} /* <-- Uh oh! */

It should generate .imp-button--compact > .imp-button-text instead, so that we don't have to double up on all the nested classes on subcomponents for different variations.

Arriving at a solution

I've come up with a few complex builder patterns to work around this, but none of them really fit the bill 100% and really, they all feel like just stacking on needless complexity, when I could solve this tout-de-suite if I could have a little look at the right-hand key-selector and chop off the part after the double dash that we use in our convention and thus simply hide all the complexity from my libraries' consumers.

E.g.

.lib-button(@height; @hor-padding) {
   // Pull up the current right-hand key selectors and work with the first one.
   // (KISS: assume people aren't calling this with multiple selectors.)
   @selector : extract(key-selectors(),1);

   // Some extraction of the base and variation part of a classname. Probably regex match?
   @class-base      : ...
   @class-variation : ...

   .lib-button-text() {
     // Shared text span implementation here.
   }

   &-text when ( @class-variation = "" ) { .lib-button-text();  }
   & > @{class-base}-text  when not ( @class-variation = "" ) { .lib-button-text();  }
}

.imp-button {
  .lib-button(40px, 10px);
}

.imp-button--compact {
  .lib-button(20px, .5px);
}

Output:

.imp-button {}
.imp-button-text {} /* <-- Nicely shortened, removing the redundant child combinator! */

.imp-button--compact {}
.imp-button--compact > .imp-button-text {} /* <-- Perfect! */

I've already implemented a generic selectors() function to get the full set of active selectors (taking into account & and variable-name token replacement) and a key-selectors() function that does the same, but returns the key-selector (i.e. the most right-hand element) only.

It took some digging to figure out how to work with the data structures that the compiler has in place, but it was very doable as a pluggable extension function that walks back over the stack of selectors maintained in the Eval context of the function call node and assembles them into the full set of complete selectors. If that sounds as a surprise: yes they are accessible via this.context.selectors, as functions are executed with this bound to the node instance.

(Friendly Note: The fact that you can acccess the current eval context that way is also a god-sent that empowers plugin functions to fullfill several very complex scenarios. Please, please, please; don't ever change that!)

The actual problem

The problem is that this works fine for regular nested rulesets, but currently BREAKS when performed directly or indirectly inside a mixin or detached ruleset.

I traced down the root cause, which is the fact that when callEval is executed for a DetachedRuleset or MixinDefinition node, a new Eval context is created and the selectors are not part of the copy-constructed properties. Ergo, inside a mixin or ruleset call this.context.selectors resets to the empty set.

Would there be any harm in adding "selectors" to the array of cloned properties for Eval contexts? (I could imagine it breaking the join-selector visitor in some way, but I'm not sure.)

matthew-dean commented 9 years ago

You lost me at:

It should generate .imp-button--compact > .imp-button-text instead, so that we don't have to double up on all the nested classes on subcomponents for different variations.

I'm not sure why it should generate something other than what you've written. If you call the mixin from within .imp-button--compact, then what was output seems perfectly expected.

To me, this seems like a situation of designing yourself into a corner. You designed your mixins and classes in a particular way, but then want to change the compiler from expected output?

I dunno, it just seems like the proposed solution is a product of overthinking. There are a number of other ways to get to the proposed output other than adding a new feature.

rjgotten commented 9 years ago

I'm not sure why it should generate something other than what you've written.

Let me rephrase: Ideally I would like to write a mixin that generates .imp-button--compact > .imp-button-text instead of doubling up the --compact suffix. The pattern of using the & replacement token inside the URL selector does not agree with that; it will double up the suffix.

There's no built-in way to capture what the & represents and modify it before use. That's why I built a custom plugin function to reflect the current selector (aka what the & represents) into a variable: so I can pick out the part I need and then use a variable replacer token in my nested selector instead of a & replacer token...

There are a number of other ways to get to the proposed output other than adding a new feature.

While there are other ways to accomplish this than reflecting on the current selector and extracting the base of the CSS class that way, those methods are all leaky abstractions that place additional burden on the consumers of the mixin library. That's something I specifically want to try and avoid, because none of it scales. (Keep in mind; this presentation of my use-case is a severe reduction of what the actual thing looks like!)

As for this being a request to 'add new features': i'm not asking for new core features at all. I'm only asking whether the non-propagation of the selector set through mixin calls or ruleset calls is a conscious choice, or whether it is in fact an oversight or bug. (And if it's a bug: whether fixing it would break other things that are implicitly relying on the 'broken' behavior.)

I'm not asking for the custom functions that do the actual reflecting to become part of Less core. Those are supposed to stay as plugins.

I dunno, it just seems like the proposed solution is a product of overthinking.

Reflecting or capturing the current selector has been a part of Less's main competitor Sass for a long, long time. And it's most commonly used for exactly the kind of pattern I'm explaining here. It's not a product of overthinking. (Again; my real scenario is a bit more complex than the reduced use-case I present here!) This kind of thing has proven worth.

seven-phases-max commented 9 years ago

I agree with @matthew-dean, the use-case example is too close to an XY problem. Actually for me the example looks like just a standard (one of "top 5") development pitfall: one tries to design the only function/mixin "to rule them all", and then (since this non-modular approach always fails in the end) he tries to invent some stuff to put into that "I can do everything" entity to use as conditions/exceptions for each (usually orthogonal) specific context he want that function/mixin in. (This is absolutely fine during drafting and/or when you need something fast-n-dirty to work, but this obviously not a good example to back a new language feature).

In other words, the code (using the proposed feature) in your example only tries to fight/cancel & > &-text of your initial mixin (i.e. instead of solving the root of the problem itself, you use the proposed feature only to work out the consequences of the initial method you used to make it in a simple case).

So not diminishing the feature itself in general, I also think the example just prays for another solutions. And though I probably would sympathize some extra "selector manipulation facility" in general, if it's only about "sniffing a ruleset the mixin is invoked in and change the output depending on that" - ouch... If you need to play with selectors in a mixin, pass them explicitly, e.g.:

1 - simple and straight-forward 2 - the same in reverse 3 - same as 1 w/o "redundant .base-button > .base-button-text" Personally I guess I would even prefer less DRY but more explicit variant of 2 like 4 ... etc. and so on.

Obviously those are simplified examples just for illustration. Indeed, there're many ways and exact code may vastly differ depending on the actual needs. Especially considering those "stripping default button styles, the quirks of ::moz-focus-inner" you mentioned, which means that most likely your actual button styles differ only in a few properties and the rest of their styles is the same. I.e. I'd expect some extend going somewhere inside of the mixin(s), which in its turn leads to favour other impl. methods and so on (e.g. crafting things to use the extend w/o bloating there may be much more important than inventing an extra code to eliminate "redundant .base-button > .base-button-text" stuff etc.).


So in summary: I can imagine how other new features can make my above examples more DRY or more readable (#1546, #1075, "ability to use . identifiers as mixin args to avoid hardcoded class dots or ugly ~".class" values" etc. and so on). But for this particular feature-request (in short: "allow expand/evaluate & in value statements"?) I'm afraid you'll need to find another use-case example (I recall there were some issues where something like this seemed to be helpful, but I can't find those right now). But so far my -1 too.

rjgotten commented 9 years ago

if it's only about "sniffing a ruleset the mixin is invoked in and change the output depending on that"

It's not. Please go back and re-read.

The fact that I change output (i.e. either render with or without child combinator) is a nice optimization extra that makes the CSS cheaper to process by a browser during the style matching phase. It's not the core motivation behind the use-case.

The core motivation is being able to generate a child class selector based on a common prefix in the parent class selector without having to explicitly hand-type the child class selector and pass it in as a parameter, which is nothing but extranuous noise.


Actually for me the example looks like just a standard (one of "top 5") development pitfall: one tries to design the only function/mixin "to rule them all", and then (since this non-modular approach always fails in the end) he tries to invent some stuff to put into that "I can do everything" entity to use as conditions/exceptions for each (usually orthogonal) specific context he want that function/mixin in.

The full use-case involves some 150 odd lines of CSS with intricate border/margin/padding computations to correctly align a font-icon on the left-hand/right-hand/etc. side of a text-label within the button while both text and icon can be individually sized with either px or em units and the button itself either follows a content-width (stretch-to-fit) model or fills out the available continer width.

This is not trivial stuff that you want an inexperienced end-consumer to end up having to call by hand.

Moreover, it's already cut up into componentized logic for setting button dimensions, icon dimensions, icon alignments, text-label alignment, spacing between icon and text label, etc. This is not a 'non-modular approach' at all.

1 - simple and straight-forward 2 - the same in reverse 3 - same as 1 w/o "redundant .base-button > .base-button-text" Personally I guess I would even prefer less DRY but more explicit variant of 2 like 4

Method 1 is the first I tried, actually: it breaks when you want to add additional properties to the ruleset itself. E.g.

.imp-button {
  .lib-button(--compact, B);
   font-size: .8em;
}

Methods 2 and 3 break in the same way. Method 4 actually works though, and it's what I'm currently using. However, it's an ugly wart to have to pass in the name of the base class that way and to have to use two separate mixins (or an overloaded signature). Being able to capture the current ruleset selector would mean you could pull out that base class name automatically and automatically determine which version (main class or subclass) of the logic to run.


(This is absolutely fine during drafting and/or when you need something fast-n-dirty to work, but this obviously not a good example to back a new language feature).

Again, as I have already mentioned in my last reply: there is NO NEW LANGUAGE FEATURE INVOLVED. The actual reflecting on the selector is A PLUGIN FUNCTION and is designed to REMAIN AS SUCH.

I only need to know if it's a compiler bug or oversight that the actual running set of selectors is not copy-constructed into new eval contexts, because that's the only thing which is breaking the plugin I already wrote. (Geez... it would've been quicker to just hack in the change and run the unit test suite to find out for myself.)

seven-phases-max commented 9 years ago

@rjgotten

It's not. Please go back and re-read.

With "changing output" I directly mean "child class name extracted as a substring from a parent class name".

Actually I'm a bit surprised that the idea of a technique like this comes from you (considering critiques like this and similar). Because, to be honest, it's nothing but: "I don't won't to pass an argument to sqrt function because it's nothing but extranuous noise, I need a method to extract that value within sqrt itself from the callee scope variable of some predefined name". (yet again it's nothing wrong with the feature, it's just the example which smells like a dirty hack).

I'm only asking whether the non-propagation of the selector set through mixin calls or ruleset calls is a conscious choice

I probably missed this statement earlier, sorry (I was more focused on those parts you've stresses with BOO :). Actually, current context including parent selectors is available in functions (this.context.frames should include selectors too unless something wrong with the calling code).

rjgotten commented 9 years ago

Actually I'm a bit surprised that the idea of a technique like this comes form you (considering critiques like this and similar). Because, to be honest, it's nothing but: "I don't won't to pass an argument to sqrt function because it's nothing but extranuous noise, I need a method to extract that value within sqrt itself from the callee scope variable of some predefined name".

Detached rulesets in Less query the full call-site scope before the declaration scope when evaluating variables and yes; I still believe that's a stupid thing to do.

Reading back the current selector is not the same.

The critical differences are:

  1. that variable names in your paraphrased quote don't follow a known naming system like CSS class names can follow BEM or similar system with implied hierarchy encoded into the name, and
  2. that such an approach of reading back the selector would only look at the right-hand key-selector (i.e. the deepest level) instead of looking all the way up the chain potentially pulling a value from who knows how far up the scope.
rjgotten commented 9 years ago

Actually, current context including parent selectors is available in functions (this.context.frames should include selectors too unless something wrong with the calling code).

Then you do have a bug; because this.context.frames.selectors seems to always be the 'empty' selector set, i.e. , an array with a single null element ( [null] ), when pulled up inside a mixin. :-)

I probably missed this statement earlier, sorry (I was more focused on those parts you've stresses with BOO :).

Ehm.. Yeah. Sorry about that; had a few long stressful days...

seven-phases-max commented 9 years ago

@rjgotten Sorry, for false closing (I was editing my post, for a third time today, doh!, and accidentaly pushed the wrong button).

this.context.frames.selectors seems to always be the 'empty' ... when pulled up inside a mixin. :-)

Most likely you've accidentally tried this.context.selectors. Apparently it's this.context.frames[*].selectors to be used and not this.context.selectors as the latter (seems to) represents only selectors of the current context definition (hence it's empty for mixins defined in the global scope) while the former is the chain of (evaluation) contexts from the current scope to the global one. Here's almost working prototype - the only problem with it is that beside actual parent selector it also includes selectors of the mixins it's invoked in (if any). Currently I'm trying to find what would be a proper way to filter those.

P.S. I updated above gist with "mixin selectors" fix, now it works as expected (test). Notice however it does not handle & (not supported by Selector.toCSS()) and selector lists (just because I used dumb traversing that merges sibling selectors like if they are nested), i.e. a final version needs to be a bit more complicated of course.

matthew-dean commented 9 years ago

If there's a legitimate bug, yes, by all means let's fix it. My comment was only that there a variety of other supported patterns with no additional complexity that would achieve the same output. But if that wasn't what you were asking about, my mistake.

seven-phases-max commented 9 years ago

Btw. I did more tests with the my prototype above and so far found the only bug related to detached rulesets. If a DR is defined before it's called - everything is fine. However, if it's defined after use, its frames chain seems to be duplicated (though I'm not sure if it's not some bogus of my prototype code).

P.S. I have added some labels just to have this categorised in some way (mostly just for others to have a clue w/o reading it all (+ easy query)).

rjgotten commented 9 years ago

@seven-phases-max Some very interesting stuff here. I'm going to have to have a look at this over the weekend. Thank you. ^_^

stale[bot] commented 6 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.