1040670 - [e10s] rule-view links to inline styles do not work with e10s

Status: RESOLVED WORKSFORME

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

STR:

- Open a new e10s window
- Navigate to this bugzilla page
- Open the inspector
- In the "Rules" sidebar, you should see an 'element { }' rule with a link (on the right side) that says 'inline'
- Click on that link

Expected: A new browser window opens up with the source code of the HTML page, containing the inline style

Actual: Nothing happens

In non-e10s windows, this works fine.

Comment 1

[Patrick Brosset <:pbro>]

Seems to be caused by 'viewSourceUtils.viewSource(href, null, contentDoc, rule.line || 0);' at http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/style-inspector.js#56

Comment 2

[Patrick Brosset <:pbro>]

Actually, I was wrong, I think it is due to this line: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/style-inspector.js#53

It accesses the document getter from selection.js which isn't remote safe (http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/selection.js#153) and the following error appears in the log:
this.node is null selection.js:156

Comment 3

[Patrick Brosset <:pbro>]

A very simple fix is to stop using the selection's document getter at all and replace style-inspector.js#56 with:
let contentDoc = this.inspector.panelDoc;

It seems that the document being passed to 'viewSourceUtils.viewSource(...)' is used to detect the charset and is passed via 'openDialog' to 'chrome://global/content/viewSource.xul' (see http://mxr.mozilla.org/mozilla-central/source/toolkit/components/viewsource/content/viewSourceUtils.js#39)

I'm not sure what this is for exactly and if using a different document will cause problems.
A solution could be to load a frame script before calling 'viewSourceUtils.viewSource(...)' to get charset and isForcedCharset from the content page directly.

Comment 4

[Heather Arthur [:harth]]

I propose getting rid of the link entirely.

It opens up a view source window that is a) jarring b) not even scrolled to the line where the element is defined and c) redundant because they can see inline styles in the markup view which has the element markup clearly highlighted.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Turns out that this bug is about inline styles and not inline stylesheets.

Harth is right, we should get rid of the inline link when there is no line number attached.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: fix-rule-view-inline-styles-1040670.patch

This is a really simple one. We no longer create a link next to element {} in the rule view.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=60daa0b609ef
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=60daa0b609ef

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8508639 [details] [diff] [review]
fix-rule-view-inline-styles-1040670.patch

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

Good idea, just requesting a minor change along with a test case

::: browser/devtools/styleinspector/rule-view.js
@@ +1956,5 @@
>        this.rule.sheet.href : this.rule.title;
>      let sourceLine = this.rule.ruleLine > 0 ? ":" + this.rule.ruleLine : "";
>  
> +    if (sourceHref === "inline" && sourceLine === "") {
> +      return;

I think it would be good to also set the unselectable attribute on the parent node (as it does for about:PreferenceStyleSheet) since that's what the event listener is actually looking for to ignore it.  Just in case (a) the parent ends up with some other content in the future or gets styled in a way that it's clickable and (b) so that we could still have a test that clicks on it and expects nothing to happen

::: browser/devtools/styleinspector/test/browser_ruleview_style-editor-link.js
@@ -67,5 @@
>    yield testExternalStyleSheet(view, toolbox);
>  });
>  
> -function* testInlineStyle(view, inspector) {
> -  info("Testing inline style");

May as well keep a test here that instead makes sure that the source label is empty and/or that clicking on it doesn't do anything

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: fix-rule-view-inline-styles-1040670.patch

(In reply to Brian Grinstead [:bgrins] from comment #7)
> Comment on attachment 8508639 [details] [diff] [review]
> fix-rule-view-inline-styles-1040670.patch
> 
> Review of attachment 8508639 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good idea, just requesting a minor change along with a test case
> 
> ::: browser/devtools/styleinspector/rule-view.js
> @@ +1956,5 @@
> >        this.rule.sheet.href : this.rule.title;
> >      let sourceLine = this.rule.ruleLine > 0 ? ":" + this.rule.ruleLine : "";
> >  
> > +    if (sourceHref === "inline" && sourceLine === "") {
> > +      return;
> 
> I think it would be good to also set the unselectable attribute on the
> parent node (as it does for about:PreferenceStyleSheet) since that's what
> the event listener is actually looking for to ignore it.  Just in case (a)
> the parent ends up with some other content in the future or gets styled in a
> way that it's clickable and (b) so that we could still have a test that
> clicks on it and expects nothing to happen
> 

Done... and for good measure I have also set sourceLabel.parentNode.hidden = true

> :::
> browser/devtools/styleinspector/test/browser_ruleview_style-editor-link.js
> @@ -67,5 @@
> >    yield testExternalStyleSheet(view, toolbox);
> >  });
> >  
> > -function* testInlineStyle(view, inspector) {
> > -  info("Testing inline style");
> 
> May as well keep a test here that instead makes sure that the source label
> is empty and/or that clicking on it doesn't do anything

We now test for an empty source label and also that the parent is hidden.

The test harness is not good at clicking something to ensure nothing happens but if the links parent is completely hidden the link is not clickable anyhow.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

BTW: The oranges can be ignored... they are from the highlighter patch.

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8508739 [details] [diff] [review]
fix-rule-view-inline-styles-1040670.patch

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

Please update comment to indicate that it's hiding the links for inline styles (and it's also not specific to e10s)

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> Comment on attachment 8508739 [details] [diff] [review]
> fix-rule-view-inline-styles-1040670.patch
> 
> Review of attachment 8508739 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please update comment to indicate that it's hiding the links for inline
> styles (and it's also not specific to e10s)

I meant commit message, not comment

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> Comment on attachment 8508739 [details] [diff] [review]
> fix-rule-view-inline-styles-1040670.patch
> 
> Review of attachment 8508739 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please update commit to indicate that it's hiding the links for inline
> styles (and it's also not specific to e10s)

Done.

I also want a better try run so pushing to a non-E10S try run:
https://tbpl.mozilla.org/?tree=Try&rev=c308ce09f9e1
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c308ce09f9e1

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Some earlier test(s) had not removed their observers so I am doing a try run with my fix for bug 1090913 (tests should fail when there are no passes, fails or todos) to help track the observers back to source.

https://tbpl.mozilla.org/?tree=Try&rev=686ad097c520

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Try:
https://tbpl.mozilla.org/?tree=Try&rev=1005735f36cb
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1005735f36cb

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Try again:
https://tbpl.mozilla.org/?tree=Try&rev=bcc70ea5e68b
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bcc70ea5e68b

Comment 16

[Chris Peterson [:cpeterson]]

These DevTools bugs should probably block e10s from riding to Aurora.

Comment 17

[Chris Peterson [:cpeterson]]

devtools bugs will block the e10s merge to Aurora, but blassey would like them to be tracked by dte10s meta bug 875871, not the tracking-e10s=m6+ flag.

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Mike, this looks close to landing.  What happens with a fresh try run?

Comment 19

[Joe Walker [:jwalker]]

Mike: ni? ping.

Comment 20

[Patrick Brosset <:pbro>]

I'll let Mike answer when he gets a chance, but I was looking at this per chance today, and it seems to work in e10s now. Clicking on the "inline" link in an e10s window opens the page's source in a new tab.

Comment 21

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Yes, I remember that somebody fixed this and meant to hunt this bug down and close it.