Closed
Bug 1262919
Opened 8 years ago
Closed 8 years ago
Original source file doesn't load in the Style Editor when the sourcemap is in a <style> tag
Categories
(DevTools :: Style Editor, defect)
Tracking
(firefox48 fixed, firefox49 verified)
RESOLVED
FIXED
Firefox 48
People
(Reporter: nchevobbe, Assigned: nchevobbe)
Details
Attachments
(2 files)
When a <style> tag contains a sourcemap reference, the original files referenced in the sourcemap don't load in the Style Editor . STR : - Open attached file - Open Style Editor tab Expected : A test.scss sass file loads in the stylesheet list Actual result : "Style sheet could not be loaded" appears on top of the panel
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → chevobbe.nicolas
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45085/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45085/
Attachment #8739173 -
Flags: review?(pbrosset)
Updated•8 years ago
|
Attachment #8739173 -
Flags: review?(pbrosset)
Comment 2•8 years ago
|
||
Comment on attachment 8739173 [details] MozReview Request: Bug 1262919 - Fix style editor error when inline style tag has a sourcemap. r=pbro https://reviewboard.mozilla.org/r/45085/#review42293 This works well. I just would like to take a second look after you've made some changes to the new test. ::: devtools/client/styleeditor/StyleEditorUI.jsm:836 (Diff revision 1) > > - let linkedCSSFile = ""; > + let linkedCSSSource = ""; > if (editor.linkedCSSFile) { > - linkedCSSFile = OS.Path.basename(editor.linkedCSSFile); > + linkedCSSSource = OS.Path.basename(editor.linkedCSSFile); > + } else if (editor.styleSheet.relatedEditorName) { > + linkedCSSSource = editor.styleSheet.relatedEditorName || ""; In this branch of the `if`, `editor.styleSheet.relatedEditorName` is never going to be falsy, so the `|| ""` part should be removed. ::: devtools/client/styleeditor/test/browser_styleeditor_sourcemaps_inline.js:11 (Diff revision 1) > +const sassContent = [ > + "body {", > + " background-color: black;", > + " & > h1 {", > + " color: white; ", > + " }", > + "}", > + "" > +].join("\n"); > + > +const cssContent = [ > + "body {", > + " background-color: black;", > + "}", > + "body > h1 {", > + " color: white;", > + "}", > + "/*# sourceMappingURL=data:application/json;base64,ewoidmVyc2lvbiI6IDMsCi" + > + "JtYXBwaW5ncyI6ICJBQUFBLElBQUs7RUFDSCxnQkFBZ0IsRUFBRSxLQUFLOztBQUN2QixTQU" + > + "FPO0VBQ0wsS0FBSyxFQUFFLEtBQUsiLAoic291cmNlcyI6IFsidGVzdC5zY3NzIl0sCiJzb3" + > + "VyY2VzQ29udGVudCI6IFsiYm9keSB7XG4gIGJhY2tncm91bmQtY29sb3I6IGJsYWNrO1xuIC" + > + "AmID4gaDEge1xuICAgIGNvbG9yOiB3aGl0ZTsgIFxuICB9XG59XG4iXSwKIm5hbWVzIjogW1" + > + "0sCiJmaWxlIjogInRlc3QuY3NzIgp9Cg== */" > +].join("\n"); Instead of using arrays that you later join with `\n`, you could simply use template strings since they support multiline. ::: devtools/client/styleeditor/test/browser_styleeditor_sourcemaps_inline.js:36 (Diff revision 1) > + "VyY2VzQ29udGVudCI6IFsiYm9keSB7XG4gIGJhY2tncm91bmQtY29sb3I6IGJsYWNrO1xuIC" + > + "AmID4gaDEge1xuICAgIGNvbG9yOiB3aGl0ZTsgIFxuICB9XG59XG4iXSwKIm5hbWVzIjogW1" + > + "0sCiJmaWxlIjogInRlc3QuY3NzIgp9Cg== */" > +].join("\n"); > + > +waitForExplicitFinish(); This isn't needed when using `add_task`. ::: devtools/client/styleeditor/test/browser_styleeditor_sourcemaps_inline.js:38 (Diff revision 1) > + "0sCiJmaWxlIjogInRlc3QuY3NzIgp9Cg== */" > +].join("\n"); > + > +waitForExplicitFinish(); > + > +add_task(function*() { This produces an eslint warning since we moved to eslint 2 (make sure you run `./mach eslint --setup` to upgrade). Please use `function* ()` ::: devtools/client/styleeditor/test/browser_styleeditor_sourcemaps_inline.js:44 (Diff revision 1) > + let {ui} = yield openStyleEditorForURL(TESTCASE_URI); > + > + is(ui.editors.length, 1, > + "correct number of editors with source maps enabled"); > + > + testEditor(ui.editors[0], "test.scss", sassContent); `testEditor` returns a promise, so you should use `yield` here. ::: devtools/client/styleeditor/test/browser_styleeditor_sourcemaps_inline.js:57 (Diff revision 1) > +function testEditor(editor, expectedName, expectedText) { > + let name = getStylesheetNameFor(editor); > + is(expectedName, name, name + " editor name is correct"); > + > + return openEditor(editor).then(() => { > + let text = editor.sourceEditor.getText(); > + is(text, expectedText, name + " editor contains expected text"); > + }); > +} Because `testEditor` is called from an async task, you can make it a generator and flatten the code like so: ``` function* testEditor(editor, expectedName, expectedText) { let name = getStylesheetNameFor(editor); is(expectedName, name, name + " editor name is correct"); yield openEditor(editor); let text = editor.sourceEditor.getText(); is(text, expectedText, name + " editor contains expected text"); } ``` ::: devtools/client/styleeditor/test/sourcemaps-inline.html:5 (Diff revision 1) > +<!doctype html> > +<html> > +<head> > + <title>testcase for testing CSS source maps in inline style</title> > + <style type="text/css">body { Please also add `<meta charset="utf-8">` to avoid warnings in the console.
Assignee | ||
Comment 3•8 years ago
|
||
Comment on attachment 8739173 [details] MozReview Request: Bug 1262919 - Fix style editor error when inline style tag has a sourcemap. r=pbro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45085/diff/1-2/
Attachment #8739173 -
Flags: review?(pbrosset)
Comment 4•8 years ago
|
||
Comment on attachment 8739173 [details] MozReview Request: Bug 1262919 - Fix style editor error when inline style tag has a sourcemap. r=pbro https://reviewboard.mozilla.org/r/45085/#review42363 Perfect!
Attachment #8739173 -
Flags: review?(pbrosset) → review+
Assignee | ||
Comment 5•8 years ago
|
||
TRY is over : https://treeherder.mozilla.org/#/jobs?repo=try&revision=095f6ff934d1 There was an exception on dt4 on Linux opt ( https://treeherder.mozilla.org/#/jobs?repo=try&revision=095f6ff934d1&selectedJob=19364694 ), but a re-launched the test and it passed ( https://treeherder.mozilla.org/#/jobs?repo=try&revision=095f6ff934d1&selectedJob=19373753 )
Keywords: checkin-needed
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c56ff321953f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment 8•8 years ago
|
||
I have reproduced this bug on Nightly 48.0a1 (2016-04-07) on ubuntu 16.04 LTS, 64 bit! The bug's fix is now verified on Latest Nightly 49.0a1! Build ID: 20160518030234 User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
QA Whiteboard: [bugday-20160518]
status-firefox49:
--- → verified
Comment 9•8 years ago
|
||
Reproduced this bug on firefox nightly 48.0a1 according to(2016-04-07) It's verified on Latest Developer Edition Build ID - 20160528004028 User Agent - Mozilla/5.0 (Windows NT 10.0; rv:48.0) Gecko/20100101 Firefox/48.0 Tested OS-- Windows 10(32bit) [bugday-20160525]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•