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

Invalid media query merge of MQs inside @import with MQ #1508

Open
hparra opened this issue Aug 23, 2013 · 4 comments
Open

Invalid media query merge of MQs inside @import with MQ #1508

hparra opened this issue Aug 23, 2013 · 4 comments

Comments

@hparra
Copy link
Contributor

hparra commented Aug 23, 2013

Tested with 1.4.2 RELEASE and 1.5.0-wip 6fe1174

A media query in a LESS file imported using import with a media query results in an invalid query.

import-test-f.less

@media screen and (min-device-pixel-ratio: 2) {
  #css {
    color: purple;
  }
}

import.less

@import "import/import-test-f" screen and (max-width: 604px);

The expected output is

@media screen and (max-width: 604px) and (min-device-pixel-ratio: 2) {
  #css {
    color: purple;
  }
}

but instead we have

@media screen and (max-width: 604px) and screen and (min-device-pixel-ratio: 2) {
  #css {
    color: purple;
  }
}

which is an invalid media query. [http://www.w3.org/TR/css3-mediaqueries/#syntax]

Furthermore, in 1.4.2 we have an empty media query with just max-width. This is no longer appearing in the 1.5.0-wip commit above.

Coincidently, the original test fails in 1.4.2 despite the fact that bin/lessc itself outputs the expected CSS. This seems to have been corrected in 1.5.0-wip as well.

@hparra
Copy link
Contributor Author

hparra commented Aug 23, 2013

Issue #1502 may be related.

Down to hack. I just need advice on where in the tree I should be looking.

@lukeapage
Copy link
Member

Hi,

  1.    Furthermore, in 1.4.2 we have an empty media query with just max-width. This is no longer appearing in the 1.5.0-wip commit above.
    

If this is a bug, please can you report it seperately, sounds like it needs fixing

To fix this, in the following code,

        // Trace all permutations to generate the resulting media-query.
        //
        // (a, b and c) with nested (d, e) ->
        //    a and d
        //    a and e
        //    b and c and d
        //    b and c and e
        this.features = new(tree.Value)(this.permute(path).map(function (path) {
            path = path.map(function (fragment) {
                return fragment.toCSS ? fragment : new(tree.Anonymous)(fragment);
            });

            // look at path and remove duplicate entries

            for(i = path.length - 1; i > 0; i--) {
                path.splice(i, 0, new(tree.Anonymous)("and"));
            }

            return new(tree.Expression)(path);
        }));

see comment I've added // look at path and remove duplicate entries

@lukeapage
Copy link
Member

I will accept a pull request for this

@hparra
Copy link
Contributor Author

hparra commented Mar 10, 2014

Ran into this again. In general MQs within MQs only work in ideal case.

I tried fixing this months ago when I first reported issue but realized it was not trivial. Path needs to be re-parsed as it is not currently saved in a useful manner that would allow easy merging. It ended up being a can of worms.

Please let me know if someone will look at this, or if the tree will change significantly.

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

2 participants