Closed Bug 724505 Opened 8 years ago Closed 4 months ago

Minified sheets need a sourcemap in the style editor

Categories

(DevTools :: Style Editor, defect, P2)

defect

Tracking

(firefox69 fixed)

RESOLVED FIXED
Firefox 69
Tracking Status
firefox69 --- fixed

People

(Reporter: miker, Assigned: rcaliman)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [polish-backlog][difficulty=medium])

Attachments

(1 file, 1 obsolete file)

The style editor automatically tidies stylesheets. Unfortunately this means that when we go to a specific line (e.g. from the rule view) it may not be the line that the rule is on. We need a sourcemap in order to fix this.
Duplicate of this bug: 1024464
Assignee: nobody → fayearthur
Duplicate of this bug: 723460
:harth, reminder to attach your WIP patch.
Flags: needinfo?(fayearthur)
Here's the WIP I have from a few months ago.

It's using rework's parser/prettifier (https://github.com/reworkcss/css) to automatically prettify files as they're loaded into the style editor, on the client side. The patch does *not* attempt to hook up the source maps yet.

The beginnings of that are in toolkit/devtools/server/actors/pretty-printer.js. A lot of that code is stolen from the debugger's script.js, with the intention of sharing pretty-printer.js with the debugger after getting it working with the style editor.

Though it's prettifying on the client right now, there was a reason I thought that it would have to be done on the server. I think it had to do with the fact that both the inspector and style editor have to know about the new code locations. Which is why I wanted to fix bug 871423 first so we could share the style sheet actors between the two.

Looking at how long prettifying and generating the source map can take, it seems like we might want to go the route of the debugger and Chrome, and wait to prettify until the user presses a button.
Flags: needinfo?(fayearthur)
Assignee: fayearthur → nobody
Whiteboard: [devedition-40]
Whiteboard: [devedition-40] → [devedition-40][difficulty=medium]
Duplicate of this bug: 1036698
Whiteboard: [devedition-40][difficulty=medium] → [polish-backlog][difficulty=medium]
One simple thing to do here instead is just change _getSourceTextAndPrettify
to call _updateStyleSheet if the sheet changes.  The the inspector picks up
the new lines and everything seems to work.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
This plan turns out to be too simple in the case where the user opens the inspector
and clicks on a rule's link.  In this situation the style editor will open to the
wrong line.
A couple more notes on this.

The current model seems a bit weird.

If the rule view does an edit, the result is re-prettified on the style editor side.
That is, from this point of view the edits are done on the original text and then
re-presented to the user in a prettified way.

On the other hand, if the user does an edit in the style editor, then that exact
text is parsed and now becomes the canonical source.


It's a bit stranger for users using source maps.
In this case edits are necessarily only done to the minified code.
So if you are looking at "original sources", you won't see any edits.
(I don't think we can truly handle this case but at least we can give
a visual warning that something weird is going on.)


I tend toward trying to salvage the plan in comment #6.
However this plan also makes the diffing feature more difficult, at least if your
source form is "ugly" according to our prettifier.
It's possible to salvage the comment #6 plan by noticing whether the style editor
is open before the user clicks on a link in the rule view.  If it is not open, then
the target location would be (optionally) translated to the new value at some point.

The downside of this is that it is ugly.  Maybe it wouldn't be terrible if I moved the
checking-and-rewriting logic into the style editor itself somewhere.


Another take on this problem is not to automatically prettify.  That is, provide a button
that the user must press.  This avoids the above problem entirely.  However, it comes with
its own issue, which is that after editing the text, the button can't be un-clicked --
there's no way to un-prettify the source.  This might matter for the "diffing" feature.
Duplicate of this bug: 1182044
A further issue is that CSS warnings emitted by the platform will use the
"uglified" locations before the style editor is opened; and (assuming the
comment #6 plan) the prettified locations after the style editor is opened.
Blocks: 1327774
Priority: P2 → P3
I'm no longer working on this.

I still think the best long-term result is to have the style editor copy the way the
debugger works; which now is to let users explicitly request pretty-printing, and to
keep both the original and generated sources available (not toggle as the style editor
does now).

I think the best route for this particular bug, in isolation, is to change
prettifyCSS:

https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/devtools/shared/inspector/css-logic.js#155

... to also construct a mapping between the generated and original location of
each token.  This requires some logic changes, because currently the token-copying
is batched, but that may be less convenient when tracking locations.

Then, this spot:

https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/devtools/client/styleeditor/StyleSheetEditor.jsm#275

can apply the new mappings using the |applySourceMap| method on the toolbox's sourceMapService.
Assignee: ttromey → nobody
Status: ASSIGNED → NEW
Product: Firefox → DevTools
Assignee: nobody → rcaliman
Priority: P3 → P2

Ensure source links for selectors in the Rules view and warning links in Web Console for minified CSS go to the right location in the Style Editor after applying automatic prettification.

This only works for linked stylesheets. Bug 1169770 needs to be fixed first before applying this logic to inline minified stylesheets.

This patch augments the prettifyCSS() method to generate the mappings necessary to generate a sourcemap from the original to the prettified source. It uses these mappings to translate the cursor position when invoking the Style Editor be opened at a specific location.

The mappings from the minified to p the rettified source are used only until the stylesheet is changed in the Style Editor. Upon editing the source in the Style Editor, the associated mappings are cleared because it's likely they have been rendered invalid by the editing.

The updated stylesheet will already be prettified so it bypasses the prettifyCSS() method, thus avoiding the need to re-generate mappings. New CSS warnings will be listed in the console which point to the right location in the edited stylesheet (the old warnings no longer point to the right place, but that's an acceptable side-effect). The Rules view in the Inspector also lists selectors with the new positions within the edited stylesheet.

Attachment #8531013 - Attachment is obsolete: true
Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7d8c3d43cd53
Map minified CSS to prettyfied CSS. r=gl
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69
Regressions: 1567164
You need to log in before you can comment on or make changes to this bug.