880831 - support //# sourceURL annotations for css names

Status: NEW

(DevTools :: Style Editor, enhancement, P3)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

The user might be using a build step which renames CSS files and they might include a //# sourceURL=... annotation to let us know about the renaming. We should use that information :)

See also http://www.softwareishard.com/blog/firebug/firebug-tip-label-dynamic-scripts-with-sourceurl-directive/ which has a nice little regexp for us.

Comment 1

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 2

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 3

[Tom Tromey :tromey]

Now that both platform CSS parsers extract sourceMappingURL from comments, they
should also be changed to extract sourceURL as well, and expose it in StyleSheet.webidl, like:

https://dxr.mozilla.org/mozilla-central/rev/93dd2e456c0ecca00fb4d28744e88078a77deaf7/dom/webidl/StyleSheet.webidl#41

Comment 4

[Tom Tromey :tromey]

It's simplest to start way upstream:

https://github.com/servo/rust-cssparser/pull/197

Comment 5

[Tom Tromey :tromey]

To support the case where there are multiple <style>s, each with a sourceURL, we'll need a
way to differentiate them.

Two approaches come to mind.

One is to follow SpiderMonkey and have CSS errors be reported using the sourceURL.
This would require a bit of URL resolution when computing the display URL.
Also I think it would require queueing up the CSS errors, because the sourceURL
generally appears at the end of the text (when it appears at all), and so lexing
must be complete before the error can be emitted.

The other idea is to handle this in devtools.  In this situation it would be good to
capture the line+column of the <style>, so that the client-side source map service
could know what to do.

I'm leaning toward the client-side idea, but I figured I'd check to see if you have some
preference, since either way seems to require platform changes in layout/.

I did consider just looking at rule line numbers, but I think it's possible to get a CSS
console message from a sheet with no rules (?).

Comment 6

[Tom Tromey :tromey]

See also bug 1192882, where we're discussing changing how SpiderMonkey handles sourceURL.

Comment 7

[Cameron McCormack (:heycam)]

I'm confused why we need to do something special to handle multiple <style> elements in the document.

FYI we already record the line number of a <style> element (you can get it from nsStyleLinkElement::GetLineNumber) but not the column as far as I know.

(In reply to Tom Tromey :tromey from comment #5)
> I did consider just looking at rule line numbers, but I think it's possible
> to get a CSS console message from a sheet with no rules (?).

Yeah, you could have sheet that only had invalid content.  In the end it would have no rules but would still generate some errors.

Comment 8

[Tom Tromey :tromey]

(In reply to Cameron McCormack (:heycam) from comment #7)
> I'm confused why we need to do something special to handle multiple <style>
> elements in the document.

Suppose we have index.html with

<style> ..stuff.. /*# sourceURL: source1.css */ </style>
<style> ..stuff.. /*# sourceURL: source2.css */ </style>

CSS warnings will be emitted as "index.html:LINE".
In the client-side plan, the devtools will want to translate the location of this
message to read either "source1.css:LINE" or "source2.css:LINE".
But, I think you can't know which one is correct unless you have a map from
line numbers to style sheets.

Actual example here: https://tromey.github.io/source-map-examples/css-source-url/index.html

> FYI we already record the line number of a <style> element (you can get it
> from nsStyleLinkElement::GetLineNumber) but not the column as far as I know.

Yeah.  The column is only needed for the pathological case where everything is on one line.
But that seems at least possible given that minimizers do this already.

I guess the main bit here is exposing this info to JS so devtools can get at it.

Comment 9

[Cameron McCormack (:heycam)]

(In reply to Tom Tromey :tromey from comment #8)
> Suppose we have index.html with
> 
> <style> ..stuff.. /*# sourceURL: source1.css */ </style>
> <style> ..stuff.. /*# sourceURL: source2.css */ </style>
> 
> CSS warnings will be emitted as "index.html:LINE".
> In the client-side plan, the devtools will want to translate the location of
> this
> message to read either "source1.css:LINE" or "source2.css:LINE".
> But, I think you can't know which one is correct unless you have a map from
> line numbers to style sheets.

OK, so by the time devtools wants to translate the source URL, it has lost which style sheet generated the error?

I wonder if it's possible to pass the nsStyleLinkElement or the StyleSheet object down along with the error, so that you don't need to know the location of the sheet to find it, and instead can just call GetSourceURL() on it directly.

If the <style> element moves around in the DOM, then nsStyleLinkElement::mLineNumber doesn't get updated.

Comment 10

[Tom Tromey :tromey]

(In reply to Cameron McCormack (:heycam) from comment #9)

> OK, so by the time devtools wants to translate the source URL, it has lost
> which style sheet generated the error?

Yes.

There are two cases to consider for CSS locations.  One is an error arriving
at the console -- in this case just get an nsIScriptError.  The other case
is when dealing with stylesheets directly; and in this case the relevant
code can do much more, since it has access to the style sheets.

> I wonder if it's possible to pass the nsStyleLinkElement or the StyleSheet
> object down along with the error, so that you don't need to know the
> location of the sheet to find it, and instead can just call GetSourceURL()
> on it directly.
> 
> If the <style> element moves around in the DOM, then
> nsStyleLinkElement::mLineNumber doesn't get updated.

Yeah, and there's also the case of adding <style>s dynamically.

Maybe queueing errors until the initial parse is complete is best.
It isn't clear to me how possible it is to put a stylesheet into nsIScriptError.

Though nothing says we have to use nsIScriptError, there's a base class and we could
invent nsICSSError and have it carry whatever payload we like.
I would have to investigate more before committing to this, though, since I don't
know how hard this would be to deal with on the devtools side, like whether we
can make a style sheet actor at this point.

Comment 11

[Tom Tromey :tromey]

(In reply to Tom Tromey :tromey from comment #10)

> Though nothing says we have to use nsIScriptError, there's a base class and
> we could
> invent nsICSSError and have it carry whatever payload we like.
> I would have to investigate more before committing to this, though, since I
> don't
> know how hard this would be to deal with on the devtools side, like whether
> we
> can make a style sheet actor at this point.

Another idea I had (which might be simpler for devtools) would be to pass along
some kind of source map cookie -- just a string or other unique identifying marker --
so that a console message can later be associated with a style sheet (or JS
script).

This would avoid having to somehow ship actor IDs in the console messages.

Comment 12

[Cameron McCormack (:heycam)]

(In reply to Tom Tromey :tromey from comment #11)
> This would avoid having to somehow ship actor IDs in the console messages.

That sounds like a reasonable solution to me.  Just give each C++ StyleSheet object a uinque serial number that you could add to the nsIScriptError.  (I think you're right that adding the StyleSheet itself to the error object would be tricky.)

Comment 13

[Tom Tromey :tromey]

I'm not working on this now; but I think the ideas in comment #11 and comment #12 are the way to go.
I wrote these up a bit in the source map status document as well: https://docs.google.com/document/d/19TKnMJD3CMBzwByNE4aBBVWnl-AEan8Sf4hxi6J-eps/edit#