988887 - [rule view] Relative image URL link from <style> element is resolving to chrome://browser/content/devtools/

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Brian Grinstead [:bgrins]]

STR:
Inspect the div here: http://jsfiddle.net/bgrins/5SCC8/
Hover the "fake.png" link in the rule view
Notice that it is resolving to chrome://browser/content/devtools/fake.png

It should resolve relative to the window

Comment 1

[Brian Grinstead [:bgrins]]

There is test coverage for this when set in an inline style attribute here: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/test/browser_styleinspector_bug_677930_urls_clickable.js and here https://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/test/browser_styleinspector_bug_677930_urls_clickable.html, but it is not working from an external CSS file

Comment 2

[Brian Grinstead [:bgrins]]

Happens when the URL is specified inside of a <style> tag

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: styletag-relative-link.patch

Falling back to domRule.nodeHref when setting the baseURI for output parser to handle the case of a relative URL specified in an inline style tag.  Also added a case for this and did some refactoring in the test.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=b1a12a9c5242

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8398039 [details] [diff] [review]
styletag-relative-link.patch

Review of attachment 8398039 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. R+.
I'd like a little bit of clarification of why we use nodeHref in the code (with a comment perhaps).
Also, would be good to move the 2 node selection lines in the test that are repeated several times.

::: browser/devtools/styleinspector/rule-view.js
@@ +1927,5 @@
>    this.browserWindow = this.doc.defaultView.top;
>    this.removeOnRevert = this.prop.value === "";
>  
>    let domRule = this.prop.rule.domRule;
> +  let href = domRule ? (domRule.href || domRule.nodeHref) : null;

It's cool that you found the fix for that (there was another bug about this, which I've dup'd).
I noticed that the nodeHref comes from the stylesheet actor in fact. I was a bit lost at first when looking for it in rule-view.js (client side) and styles.js (server-side) and couldn't find anything relevant.
The link between the rule actors and stylesheet actors isn't very very clear today I'd say, and the name "nodeHref" isn't great either in the context of a rule. So maybe one thing we could do in this patch is simply either add a comment or move this line to a function with jsdoc in order to explain when domRule may be undefined and why, and when domRule.href may be undefined too, and why we switch to nodeHref instead.
It's just that there are 2 conditions here that are not trivial.

::: browser/devtools/styleinspector/test/browser_styleinspector_bug_677930_urls_clickable.js
@@ +36,5 @@
>    ok(base64, "captain, we have the base64 div");
>    ok(noimage, "captain, we have the noimage div");
>    ok(inlineresolved, "captain, we have the inlineresolved div");
>  
> +  Task.spawn(function*() {

Nice! Getting rid of this callback cascade is cool.

@@ +39,5 @@
>  
> +  Task.spawn(function*() {
> +
> +    inspector.selection.setNode(relative1);
> +    yield inspector.once("inspector-updated");

I think it makes more sense to write these 2 lines as these 3 instead:

let onUpdated = inspector.once("inspector-updated");
inspector.selection.setNode(relative1);
yield onUpdated;

This way, you start listening before changing the node.
I know the fact that when we select our inspector panels update themselves async and therefore you can start listening after without any problem today, but it's safer for the future if we start caching stuff and update synchronously.
(note that I'm refactoring the styleinspector tests at the moment, and I have extracted these 3 lines in a selectNode function)

@@ +46,5 @@
>      ok (relativeLink, "Link exists for relative node");
> +    is (relativeLink.getAttribute("href"), TEST_IMAGE, "href matches");
> +
> +    inspector.selection.setNode(relative2);
> +    yield inspector.once("inspector-updated");

Same comment

@@ +55,3 @@
>  
>      inspector.selection.setNode(absolute);
> +    yield inspector.once("inspector-updated");

And again

@@ +62,3 @@
>  
> +    inspector.selection.setNode(inline);
> +    yield inspector.once("inspector-updated");

And again :)
Better move these lines to a function that yields.

yield setNode(inline, inspector);

...

function* setNode(node, inspector) {
  let updated = inspector.once("inspector-updated");
  inspector.selection.setNode(node);
  yield updated;
}

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: styletag-relative-link.patch

Can you have a quick look at the changes?  Moving the href to a function but keeping the sheetURI far away in the constructor seemed weird, so I refactored a bit (into two getters, sheetURI and sheetHref).  Also updated the test to use a setNode function.  Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1f02ac9f00a4

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8398591 [details] [diff] [review]
styletag-relative-link.patch

Review of attachment 8398591 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. I like this version better.

Comment 8

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/d35aab7ba875
https://tbpl.mozilla.org/?tree=Fx-Team&rev=d35aab7ba875

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d35aab7ba875

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8398591 [details] [diff] [review]
styletag-relative-link.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 677930
User impact if declined: Clicking a relative URL from a style tag in the devtools ruleview will open a URL starting with chrome://browser/content/devtools/
Testing completed (on m-c, etc.): On m-c since 3-31
Risk to taking this patch (and alternatives if risky): Low risk - change to DevTools inspector ruleview only, includes test changes
String or IDL/UUID changes made by this patch:

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dd6209a8d63d

Comment 12

[u279076]

Flagging in-testsuite+ since this landed with tests.