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

nested styles refer to top-level parent in sourcemap line number #1747

Closed
opeologist opened this issue Nov 21, 2015 · 14 comments · Fixed by #1759
Closed

nested styles refer to top-level parent in sourcemap line number #1747

opeologist opened this issue Nov 21, 2015 · 14 comments · Fixed by #1759
Assignees
Milestone

Comments

@opeologist
Copy link

i'm creating this issue here to get more visibility on an issue i originally opened in the node-sass repo. link here: sass/node-sass#1206

i'm having an issue where my nested styles' sourcemap line number in chrome is referencing the top-most parent selector line and not the line that the nested selector starts.

body {
  h1 {
    background: #f00;
  }
}

will show file.scss:1 instead of file.scss:2 for the h1

i've tried outputting the map file to a separate file and embedding it, all the same.



just ran a test with the latest ruby sass and it's sourcemapping nested children on the child's line and not the top-most parent like it should be. this is definitely a bug and isn't working as intended.

Ruby Sass 3.4.19:

body {
  h1 {
    background: red;
  }
}

compiles to:

body h1 {
  background: red; }

/*# sourceMappingURL=core.css.map */

core.css.map:

{
"version": 3,
"mappings": "AACE,OAAG;EACD,UAAU,EAAE,GAAG",
"sources": ["core.scss"],
"names": [],
"file": "core.css"
}

node-sass 3.4.1:

body {
  h1 {
    background: red;
  }
}

compiles to:

body h1 {
  background: red; }

/*# sourceMappingURL=core.css.map */

core.css.map:

{
    "version": 3,
    "file": "core.css",
    "sources": [
        "core.scss"
    ],
    "mappings": "AAAA,IAAI,CACF,EAAE,CAAC;EACD,UAAU,EAAE,GAAI,GACjB",
    "names": []
}
@nek4life
Copy link

Having the same issue with python-libsass, it seems it's the underlying libsass implementation of sourcemaps

@mgreter
Copy link
Contributor

mgreter commented Nov 25, 2015

I do see your point. But unfortunately libsass technically does the correct thing and in some way chrome also reports the correct thing. It actually shows where the first selector in the sequence is declared. And this is basically not what one would probably expect, specially given that chrome doesn't use much of the information that would be available via the source-map. BTW. with less you also get that line reported.

Also be aware that sourcemaps v3 are still just a proposal, so any implementation could be questioned. Unfortunately for libsass it seems that all browsers that support css sourcemaps agree to report this information that way. IMHO this is not really correct, since in that situation I would expect them to report where the scope/ruleset for the styles is opened (and we actually have a sourcemap pointer there).

I'm reluctant to remove all mappings we currently have, since they are technically correct and could be very usefull for more advanced inspector tools. But I see the pain point and currently no such advanced tooling exists, so I'm willing to add a crutch to make this more usefull in the meantime (#1759). Btw. the v3 proposals were lately extended with some notions of Multi-level Mapping Notes, which is pretty much what we try to do with sourcemaps already.

@opeologist
Copy link
Author

thank you so much @mgreter, so i should use that branch for now to get the sourcemapping i'd like?

@mgreter
Copy link
Contributor

mgreter commented Nov 27, 2015

Here are the corresponding bug reports for less (less/less.js#1492) and the one for chromium (https://code.google.com/p/chromium/issues/detail?id=287382).

@mgreter
Copy link
Contributor

mgreter commented Nov 27, 2015

@tehOPEologist I would advise people to just ctrl click on the actual (ie. first) style rules 😉

@opeologist
Copy link
Author

@mgreter so there isn't a fix for this currently is what you're saying? i'm confused by your "ctrl+click" comment, as that would just bring up the file itself and the first selector.. doesn't go to the line where the style you've clicked on is.

@mgreter
Copy link
Contributor

mgreter commented Nov 27, 2015

ctrl-click

Not sure if we add the crutch mentioned before, since it is a browser issue and there is a way in chrome to get to the correct place by ctrl+click (see screenshot, you could also ctrl+click on the color). Just to be clear, hold ctrl and click on background (or red) will bring you to the right position.

@opeologist
Copy link
Author

ahh nice, thanks i guess i can use that for now till it gets cleared up in chrome. thanks again!

@mgreter
Copy link
Contributor

mgreter commented Nov 27, 2015

Just a note: this is not possible in firefox currently, but I hope they will adapt the behavior of chrome (and I'm pretty positive since the dev tools in firefox are pretty new and quite actively developed atm).

@xzyfer xzyfer reopened this Nov 28, 2015
@xzyfer xzyfer added this to the 3.3.3 milestone Nov 28, 2015
@xzyfer
Copy link
Contributor

xzyfer commented Nov 28, 2015

Although I agree with @mgreter that we're likely technically correct, the fact that we differ from Ruby Sass in this is a enough to provide a fix.

@opeologist
Copy link
Author

i agree @xzyfer and i appreciate this problem being addressed! will continue to monitor. thank you again!

@mgreter
Copy link
Contributor

mgreter commented Nov 28, 2015

@xzyfer ruby sass sourcemaps are lacking a lot according to previous discussions with @am11. I decided to do better than that and therefore libsass has much more detailed source-maps than ruby sass does (not sure if this is still true, since those discussion are over a year old). The question is really if we want to merge the crutch in #1759 into master or not. That change is techically not correct, but will make chrome and firefox report a more usefull mapping. But it could confuse more advanced tools, since that mapping is technically not correct.

@ggedde
Copy link

ggedde commented Oct 17, 2017

@mgreter @tehOPEologist This issue is still going on.
I understand both sides to this and it seems that making a hard change is not ideal.

So why not just have a simple compile option to allow for it.
ie.
--single-level

This way you leave it up to the developer to use it accordingly depending on their tools and IDE, etc.

OR change "single-level" to the default and have --multi-level as an option. Since no IDE currently requires the multi-level source maps.

Either-way works.

@SweetsXob
Copy link

This problem does not seem to be solved now.
It seems less doesn't have this problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants