Original source file doesn't load in the Style Editor when the sourcemap is in a <style> tag

RESOLVED FIXED in Firefox 48

Status

()

Firefox
Developer Tools: Style Editor
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: nchevobbe, Assigned: nchevobbe)

Tracking

47 Branch
Firefox 48
Points:
---

Firefox Tracking Flags

(firefox48 fixed, firefox49 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
Created attachment 8739120 [details]
sourcemap-inline.html

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

2 years ago
Assignee: nobody → chevobbe.nicolas
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
Created attachment 8739173 [details]
MozReview Request: Bug 1262919 - Fix style editor error when inline style tag has a sourcemap. r=pbro

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)
Attachment #8739173 - Flags: review?(pbrosset)
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

2 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 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

2 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 6

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/c56ff321953f
Keywords: checkin-needed

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c56ff321953f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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
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]
You need to log in before you can comment on or make changes to this bug.