SomMeri / less4j

Less language is an extension of css and less4j compiles it into regular css. Less adds several dynamic features into css: variables, expressions, nested rules, and so on. Less was designed to be compatible with css and any correct css file is also correct less file.
146 stars 47 forks source link

List of selectors appended to parent selector are treated as a single selector #337

Open twhoff opened 8 years ago

twhoff commented 8 years ago

This is maybe more of a feature request than an issue since Node LESS has the same behaviour, but this is really annoying, useless behaviour and it would be great if it could be fixed (especially when it comes to generating efficient CSS code).

The problem:

@classes: ~".col-1", ~".col-2", ~".col-3";

.wrapper {
    .container {
        .grid {
            @{classes} {
                width: 100px;
            }
        }
    }
}

I would expect this to generate the following CSS output:

.wrapper .container .grid .col-1,
.wrapper .container .grid .col-2,
.wrapper .container .grid .col-3 {
  width: 100px;
}

Instead, it generates this:

.wrapper .container .grid .col-1, .col-2, .col-3 {
  width: 100px;
}

Obviously the context is lost on the last 2 selectors, rendering this code dangerous.

Granted, this also happens in Node LESS, I would still say this is buggy behaviour as the naturally expected output is not what actually gets output, and this code may still actually work (since .col-2 and .col-3 are still globally selected) - it may not be apparent there is a problem until these styles end up overriding something else on the website!

Could you please fixed this problem in LESS4J, even if it isn't yet on Node? It would be very much appreciated and very useful for the framework I'm developing.

Thanks for all your hard work with LESS4J :)

SomMeri commented 8 years ago

Unfortunately, I am trying to keep Less4j as close as possible to less.js, so this would need to be first in there. This relates to other issues where interpolated selector is not re-parsed and interpolated part is treated as a string, so it would be medium to large change.

The best workaround I can think of is to use looping mixin to loop through that list and generate classes inside. Maybe detached ruleset for body.

twhoff commented 8 years ago

Thanks for the reply.

The problem with looping is that it creates a significant slowdown in compilation.

With the grid framework I'm working on, I'm currently creating an adapter which allows a very simple syntax to be used for creating custom width columns. For example:

// Parameters are: .custom-columns(x-axis-gutter; ...)
// This basically says create a grid with 20px gutters, col-1, 2 and 3 have 100px width
// col-4 and 5 have 50% each
.custom-columns(20px; col-1, col-2, col-3, 100px; col-4, col-5, 50%; m)

To interpret each column declaration, I'm using looping, something similar to this (just pseudo code for demo):

LOOP FOR EACH args IN arguments
    LOOP FOR EACH arg IN args
        IF IS fixed width
            CREATE fixed width column
        ELSE IF IS fluid width
             CREATE fluid width column
        END IF
    END LOOP
END LOOP

In LESS I have to do two loops like this, the first to collect meta data about how many fixed and fluid columns there are and the total width of the fixed columns. I have this running quite efficiently using mixin chaining (basically when the condition is true and the loop needs to iterate, it does so from within the matching mixin, creating many levels of nesting in scope I imagine)

The second loop which actually processes the columns uses the same approach, but it's very slow - it can take about 7 seconds to compile a page with about 20 columns on it. There is some weirdness in the scope and I've tried using the & {} selector to separate it out a bit, but I'm not having any luck.

With looping then in LESS4J, considering a mixin call can inject all its parents' scopes into itself and this is probably why recursive mixin calls are so slow, what would you recommend is the best approach?

// METHOD 1
.loop(@counter) when (@counter > 0) {
    .loop(@counter - 1);

    .condition() when (something) {
    }
    .condition() when (something-else) {
    }
    .condition() when (default()) {
    }
    .condition();
}

// METHOD 2
.loop(@counter; @count-to) when (@counter <= @count-to) {
    .condition() when (something) {
        // Loop from here
        .loop(@counter + 1);
    }
    .condition() when (something-else) {
        // Or loop from here
        .loop(@counter + 1);
    }
    .condition() when (default()) {
        // Or exit loop
        .loop(@count-to);
    }
    .condition();
}

// METHOD 3
.loop(@counter) when (@counter > 0) {
    .loop(@counter - 1);
    .condition();
}
.condition() when (something) {
}
.condition() when (something-else) {
}
.condition() when (default()) {
}

// METHOD 4
.loop(@counter; @count-to) when (@counter <= @count-to) {
    .condition();
}
.condition() when (something) {
        // Loop from here
        .loop(@counter + 1);
}
.condition() when (something-else) {
        // Or loop from here
        .loop(@counter + 1);
}
.condition() when (default()) {
        // Or exit loop
        .loop(@count-to);
}
.condition();

Thanks for your feedback, and of course, hard work! :)

SomMeri commented 8 years ago

It is interesting problem. First thing that comes to mind is to use custom functions which is a bit of workaround and not optimal solution, but if you know you will use only less4j it could speed things up.

twhoff commented 8 years ago

I did some experimentation with different methods and found methods 3 and 4 to be far more efficient when dealing with larger loops. In fact, with every level of nesting removed, the LESS compiles 2x faster... not sure if this is a bug or has something to do with our local cache implementation (which was added in version 1.17).

Further, I also discovered that passing explicit parameters to mixins rather than allowing the mixin to inherit the parameters from its parents creates much more stable looping and ensures scopes don't mix-up or affect parent scopes through bubbling.

Back to the original issue I reported though, regarding the list of selectors being treated as a single selector - shall I first make a feature request for this to be supported in Node LESS?

SomMeri commented 8 years ago

The slow down may be due to scopes being stacked upon each other when you call mixin. If inside uses variable that is defined only somewhere deep down, nested call will double scope size it needs to travel.

I think it would be better to bump up already existing issue in less.js rather then creating a new thread. I would not expect it to be done fast through, it is not a small change unfortunately.