tpope/vim-haml

Incorrect comment highlighting in scss files

Closed this issue · 12 comments

In scss files, when using the "//" comment string, sometimes the comment doesn't get highlighted like a comment. For example:

@media (max-width:350px) {
  .foo {
    //border: 1px solid;
}

Here neither the "//" nor anything to the right of it on that line are colored like comments.

If I delete the '@' from the first line, they suddenly become colored properly. Or, if I replace @media with @import, the line becomes colored properly.

While playing round with how to perhaps fix this, I uncovered what I think is a separate error wrt comment highlighting.

In scss syntax, the double-slash is only a single-line comment. Unlike indent-syntax, it's not supposed to carry down to subsequent indented lines. But scss.vim treats double-slash just like sass.vim does.

I'm testing a workaround of inserting

syn clear sassComment

underneath line 12 of scss.vim. I'm not sure if that's the right way to fix it or not.

OK, back to the problem mentioned in my first post. I'm not sure if this solution is correct, or even if it's a reasonable workaround, but the following makes the example code render with proper highlighting. Change line 13 of scss.vim from:

syn match scssComment "//.*" contains=sassTodo,@Spell

to

syn match scssComment "//.*" contains=sassTodo,@Spell containedin=cssDefinition

I'll make a PR if you think one or both of the two fixes here are worthy.

ao2 commented

Hi, I noticed that the comments behavior also depends on the indentation, for instance the following code:

#main-content {
  .block {

   // @include media(1, 0) {
     @include all-breakpoints {
      @include type-box(1rem, 1, 1rem, 1rem, 3px);
      border-color: cyan;
      border-radius: 1rem;
      border-style: solid;
    }
    //}
  }

  #block1 {
    /*
    @include media(3, 0) {
      @include span(4 of 12);
    }
    */
  }
}

Produces this result:
vim-scss-syntax-comments-test

And I tested with with the original runtime:

$ vim -u NONE -c "syntax on" vim-scss-syntax-comments-test.scss

As you can see there are problems with multi-line comments too.

The proposed fix:

syn match scssComment "//.*" contains=sassTodo,@Spell containedin=cssDefinition

does not work in my test case, however the previously suggested workaround seemed to help:

syn clear sassComment

So i guess the problem is actually about the interaction with the sass rules about indentation.

The following patch seems to fix all the problems for me:

--- scss.vim.orig	2017-07-12 09:30:29.759401102 +0200
+++ scss.vim	2017-07-12 09:30:33.051346796 +0200
@@ -10,10 +10,13 @@ endif
 
 runtime! syntax/sass.vim
 
+syn clear sassComment
+syn clear sassCssComment
+
 syn match scssComment "//.*" contains=sassTodo,@Spell
 syn region scssComment start="/\*" end="\*/" contains=sassTodo,@Spell
 
-hi def link scssComment sassComment
+hi def link scssComment cssComment
 
 let b:current_syntax = "scss"

Additionally the single-line regex could be made a little stricter: "//.*$".

If someone could double check my rationale I'll send a pull request.

Thanks,
Antonio

tpope commented

Looks reasonable at first glance.

ao2 commented

Mmh, I am also testing sass files and the comments act weird there too, so maybe the actual fix should be in syntax/sass.vim instead.

This snippet:

/*
multi-line comment
*/
#element-id
// single-line comment. Indentation should not matter, should it?
  overflow: hidden

gets rendered like this:
vim-sass-syntax-comments-test

The github syntax highlighter seems to get it right.

I'll see if I can find a proper fix.

Ciao,
Antonio

ao2 commented

Ah, I see , in the indented SASS syntax comments do follow some indentation rules:
sass/sass#338 (comment)
http://sass-lang.com/documentation/file.INDENTED_SYNTAX.html#comments

And the github syntax highlighter is getting it wrong, so I guess the solution that resets the matching rules is OK.

@ao2 I'm not sure what work your addition to my workaround, namely syn clear sassCssComment, is doing. On my machine, with vim 8.0.711, that is unnecessary for the correction of all the wrongly-highlighted sample code in this thread. I.e., the original workaround of syn clear sassComment all by itself cures every example in this thread.

Also, I'm not sure what the reason is for this proposed change:

-hi def link scssComment sassComment
+hi def link scssComment cssComment

I have yet to find a case where a comment in a scss file was recognized as a comment but not given the same color as a regular css comment.

Strangely, my change no longer seems to be necessary so long as my workaround syn clear sassComment is present. I can no longer reproduce the original example of improper highlighting.

ao2 commented

@chdiza we need to clear both sassComment and sassCssComment, see this example:

#main {
 // Indentation after single-line comments should NOT matter in SCSS.
 // The rule below should NOT be highlighted as a comment.
 //
 // This is fixed by "syn clear sassComment"
    #elemid {
      color:blue;
    }
}

// Weirdly the problem does not occur for top-level comments.
//
// I would expect the same behavior as above here,
// however this could be a bug in the comment match rules in sass.vim,
// unrelated to scss.vim
   #test {
   }

 /* Highlighting for UNTERMINATED multi-line comments should NOT end when the
  * _indentation_ is reset in SCSS.
  *
  * The rest of the file should be highlighted as a comment.
  *
  * This is fixed by "syn clear sassCssComment"
#elemid2 {
  color:blue;
}

I tried to explain in the comments above why both are needed.

To see both problems, be sure to have a vanilla runtime (sudo apt-get reinstall vim-runtime on Debian systems) and skip any local runtime files (vim -u NONE -c "syntax on" test.scss).

As long as the last change goes I decided for hi def link scssComment Comment just to make clear that the comments definitions in scss.vim are distinct from the ones in sass.vim.

I explained it in the commit message in https://github.com/ao2/vim-haml/commit/373d58288c387846e5fc12f9460bd15bfb43cd3f

Ciao,
Antonio

Thanks for the test file! 😄 I see now what the second syn clear is for.

As long as the last change goes I decided for hi def link scssComment Comment just to make clear that the comments definitions in scss.vim are distinct from the ones in sass.vim.

Yeah, I saw that, but it doesn't actually do anything. And it violates the spirit of scss.vim, which appears to be "import sass.vim and make the bare minimum number of changes necessary". If the point is just to make it clear to anyone reading the code in syntax/scss.vim that scss comments are distinct from sass comments, that purpose seems better served by adding some comments directly in the file.

But that's just a suggestion for improvement, not a blocking complaint.

ao2 commented

hi def link scssComment Comment does the actual linking of the visual style, because scssComment was defined in this file, without that (or the previous equivalent link to sassComment, which links to cssComment, which links to Comment) the Comment visual style is not applied.

Since it works either way I am OK to leave that part out, it looked just more consistent to me because we actually define the comment matches in scss.vim, so we might as well link to the Comment style directly.

But it's just a matter of taste, I don't have a strong opinion, I'd let @tpope decide.