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 nested media query merge with "or" condition, when a variable is used for sub-media query #2964

Closed
falkenhawk opened this issue Sep 20, 2016 · 5 comments

Comments

@falkenhawk
Copy link

falkenhawk commented Sep 20, 2016

Invalid media query is compiled when using a variable containing a media query definition with OR conditions (either with comma or "or"-keyword). Top media query should be prepended to each sub-query.

Less:

@highdpi: ~"(-webkit-min-device-pixel-ratio: 1.25), (min-device-pixel-ratio: 1.25), (min-resolution: 1.25dppx)";
.bg {
    @media (max-width: 1000px) {
        background: url(bg-small.png);

        @media @highdpi {
            background: url(bg-small@2x.png);
        }
    }
}

Output:

@media (max-width: 1000px) {
  .bg {
    background: url(bg-small.png);
  }
}
@media (max-width: 1000px) and (-webkit-min-device-pixel-ratio: 1.25), (min-device-pixel-ratio: 1.25), (min-resolution: 1.25dppx) {
  .bg {
    background: url(bg-small@2x.png);
  }
}

Expected output - "(max-width: 1000px) and " before each comma-separated part:

[...]
@media (max-width: 1000px) and (-webkit-min-device-pixel-ratio: 1.25), (max-width: 1000px) and (min-device-pixel-ratio: 1.25), (max-width: 1000px) and (min-resolution: 1.25dppx) {
[...]

On the other hand, it compiles fine, when not using a variable:

.bg {
    @media (max-width: 1000px) {
        background: url(bg-small.png);

        @media (-webkit-min-device-pixel-ratio: 1.25), (min-device-pixel-ratio: 1.25), (min-resolution: 1.25dppx) {
            background: url(bg-small@2x.png);
        }
    }
}
@falkenhawk falkenhawk changed the title Invalid nested media query merge with "or" condition, when media query is added via a mixin Invalid nested media query merge with "or" condition, when a variable is used for sub-media query Sep 20, 2016
@seven-phases-max
Copy link
Member

seven-phases-max commented Sep 20, 2016

This is more a feature request than a bug. Less is not supposed to understand what is inside those ~"" (anywhere, not just for media queries). So it's expected for Less to never know about any commas or other special symbols there.

Technically, the code like above ("list of media queries in an escaped string") is more like an old quirk/syntax-workaround rather than ever considered proper code (yet again because of ~"" itself). In today's Less, the proper pattern to achieve the same stuff would be using anonymous/detached rulesets/mixins instead of "the magic value variable", e.g.:

.highdpi(@rules) {
    @media (-webkit-min-device-pixel-ratio: 1.25), 
           (min-device-pixel-ratio: 1.25), 
           (min-resolution: 1.25dppx) {
        @rules();
    }
}

.bg {
    @media (max-width: 1000px) {
        background: url(bg-small.png);

        .highdpi({
            background: url(bg-small@2x.png);
        });
    }
}

or similar methods.

Related to #1421 and similar issues/requests.


Note that even as a feature request (if proposed) this feature will have to use different syntax instead of ~"" anyway, i.e. the example is never supposed to work as expected above - so I'm not marking this as "feature request" (as the FR would require a little bit more insight).

Merging to #1421 maybe?

@falkenhawk
Copy link
Author

falkenhawk commented Sep 21, 2016

@seven-phases-max thank you for clearing this up. I'm going to change my mixin to utilize another ".highdpi" mixin instead of the variable with escaped string.

@matthew-dean
Copy link
Member

matthew-dean commented Nov 7, 2016

@seven-phases-max I think there are two separate issues / feature requests here, so not sure if we should merge to #1421 without specifying those.

  1. Allow the Less parser to be more lax with expressions and lists. Allow lists to be made up of media query values or selectors, for example. So that string escaping doesn't have to be a requirement when wanting to make a media query a variable. (Which is a common need.) This is Variable selector "string" improperly parses comma #1421, and the @media example could just be something else added to that issue.
  2. Merge lists of nested media query values (depends on #1, but should have a separate pass/fail from #1. Although having done that #1, #2 is a logical next step. So, I think we can track that here in Invalid nested media query merge with "or" condition, when a variable is used for sub-media query #2964.

@seven-phases-max
Copy link
Member

Referencing #3059 here, since essentially it's almost the same problem. I.e. basically the question is: "Can we afford full featured reparsing of @media and @supports queries, etc. etc after variables are substituted with their values?" (This either requires a major refactoring of the parser itself so it can be used on an arbitrary string at arbitrary moment, or duplicating the parsing code at each specific Node and its eval/css-gen funcs).

@stale
Copy link

stale bot commented Nov 14, 2017

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.

@stale stale bot added the stale label Nov 14, 2017
@stale stale bot closed this as completed Nov 28, 2017
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