Closed Bug 724505 Opened 8 years ago Closed 4 months ago
Minified sheets need a sourcemap in the style editor
47 bytes, text/x-phabricator-request
|Details | Review|
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.
:harth, reminder to attach your WIP patch.
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.
Whiteboard: [devedition-40] → [devedition-40][difficulty=medium]
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.
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.
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
Assignee: nobody → rcaliman
Priority: P3 → P2
Attachment #8531013 - Attachment is obsolete: true
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/7d8c3d43cd53 Map minified CSS to prettyfied CSS. r=gl
You need to log in before you can comment on or make changes to this bug.