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

Should throw error when & has no parent #1644

Closed
NielsDoucet opened this issue Oct 27, 2015 · 9 comments
Closed

Should throw error when & has no parent #1644

NielsDoucet opened this issue Oct 27, 2015 · 9 comments

Comments

@NielsDoucet
Copy link

This is a followup to issue #1569 .
Libsass no longer crashes in the specified test-case, but instead outputs the "&" in the css.

$tablet-portrait:                 768px;
$tablet-landscape:                980px;
$desk-normal:                     1120px;
$desk-big:                        1280px;
$grid-breakpoints-immobile: (
        'tablet-portrait':   '(min-width: ' + $tablet-portrait + ') and (max-width: ' + $tablet-landscape + ')',
        'tablet-landscape':  '(min-width: ' +  $tablet-landscape + ') and (max-width: ' + $desk-normal + ')',
        'desk-normal':       '(min-width: ' +  $desk-normal + ') and (max-width: ' + $desk-big + ')',
        'desk-big':          '(min-width: ' +  $desk-big + ')'
);
@mixin grid-media-query($media-query, $breakpointDefinitions) {
  $breakpoint-found: false;

  @each $breakpoint, $breakpointvalue in $breakpointDefinitions{
    $name: $breakpoint;
    $declaration: $breakpointvalue;

    @if $media-query == $name and $declaration{
      $breakpoint-found: true;

      @media only screen and #{$declaration} {
        @content;
      }
    }
  }
}

@each $name in map-keys($grid-breakpoints-immobile) {
  @include grid-media-query($name, $grid-breakpoints-immobile) {
    body.immobile & {
      margin-bottom: 0;
    }
  }
}

Produces the following outputs:
libsass version 3.2.5

@media only screen and (min-width: 768px) and (max-width: 980px) {
  body.immobile {
    margin-bottom: 0;
  }
}

@media only screen and (min-width: 980px) and (max-width: 1120px) {
  body.immobile {
    margin-bottom: 0;
  }
}

@media only screen and (min-width: 1120px) and (max-width: 1280px) {
  body.immobile {
    margin-bottom: 0;
  }
}

@media only screen and (min-width: 1280px) {
  body.immobile {
    margin-bottom: 0;
  }
}

libsass version 3.3.1

@media only screen and (min-width: 768px) and (max-width: 980px) {
  body.immobile & {
    margin-bottom: 0;
  }
}

@media only screen and (min-width: 980px) and (max-width: 1120px) {
  body.immobile & {
    margin-bottom: 0;
  }
}

@media only screen and (min-width: 1120px) and (max-width: 1280px) {
  body.immobile & {
    margin-bottom: 0;
  }
}

@media only screen and (min-width: 1280px) {
  body.immobile & {
    margin-bottom: 0;
  }
}
@xzyfer
Copy link
Contributor

xzyfer commented Oct 27, 2015

Thanks for the report @HeadOnAPlate. We're looking into it.

@xzyfer
Copy link
Contributor

xzyfer commented Nov 4, 2015

The previous behaviour was a bug. This code should error because there is no parent selector to replace & with.

Error: Base-level rules cannot contain the parent-selector-referencing character '&'.
        on line 30 of test.scss, in `@content'
        from line 22 of test.scss, in `grid-media-query'
        from line 29 of test.scss
  Use --trace for backtrace.

@xzyfer
Copy link
Contributor

xzyfer commented Nov 4, 2015

Give that the output is essentially invalid CSS the current behaviour is more correct than the previous behaviour. As such I'm moving this to 3.3.3.

@xzyfer xzyfer modified the milestones: 3.3.3, 3.3.2 Nov 4, 2015
@NielsDoucet
Copy link
Author

I understand. In the ruby-sass implementation this appears to throw an error as well.
Is there a way to detect that a mixin is called from within a parent selector? Or should we change its signature and pass in the parent directly?

Also: has the spec changed regarding this parent selector '&'? Because we have been using this code since april 2014, without any problem.

@xzyfer
Copy link
Contributor

xzyfer commented Nov 4, 2015

Since & is just a list you can test length(&) to see if you're current context has a parent selector.
This may be buggy in LibSass atm. I would develop against Ruby Sass then see if it works on LibSass.

@NielsDoucet
Copy link
Author

Ok, I experimented a bit and it turns out length(&) seems to return 1, even if there is no context.
It contains an empty string as its element.

I managed to guard the code by using this function instead:
@function hasContext() { @return nth(&, 1); }

Thanks for pointing me in the right direction.

@xzyfer xzyfer changed the title libsass includes "&" in output when it is used referencing the base-level Should throw error when & has no parent Nov 13, 2015
@saper
Copy link
Member

saper commented Nov 13, 2015

I guess this might be #1709 and/or #1729

saper added a commit to saper/sass-spec that referenced this issue Nov 14, 2015
@saper
Copy link
Member

saper commented Nov 14, 2015

It looks like

@mixin parent {
  @content;
}

@include parent() {
  body.immobile & {
    margin-bottom: 0;
  }
}

is enough to reproduce that.

@xzyfer
Copy link
Contributor

xzyfer commented Mar 17, 2016

I've got a patch for this locally.

xzyfer added a commit to xzyfer/sass-spec that referenced this issue Mar 17, 2016
wabain added a commit to DDMAL/cantus that referenced this issue May 8, 2016
Until recently (sass/libsass#1644),
libsass silently ignored the & character used to reference the
parent selector when there was no parent. Since that behaviour has
been changed, introduce a workaround to get the ua-support mixin
to work either at the base level or nested in another rule.
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

4 participants