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

RESOLVED WORKSFORME

Status

defect
RESOLVED WORKSFORME
5 years ago
Last year

People

(Reporter: pbro, Assigned: miker)

Tracking

(Blocks 1 bug)

unspecified
Dependency tree / graph

Firefox Tracking Flags

(e10s+)

Details

(Whiteboard: [e10s-m7])

Attachments

(1 attachment, 1 obsolete attachment)

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.
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
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
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.
Blocks: dte10s
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.
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.
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Summary: [e10s] rule-view links to inline stylesheets do not work with e10s → [e10s] rule-view links to inline styles do not work with e10s
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
Attachment #8508639 - Flags: review?(bgrinstead)
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
Attachment #8508639 - Flags: review?(bgrinstead)
(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.
Attachment #8508639 - Attachment is obsolete: true
Attachment #8508739 - Flags: review?(bgrinstead)
BTW: The oranges can be ignored... they are from the highlighter patch.
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)
Attachment #8508739 - Flags: review?(bgrinstead) → review+
(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
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
These DevTools bugs should probably block e10s from riding to Aurora.
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.
Whiteboard: [e10s-m6]
Whiteboard: [e10s-m6] → [e10s-m7]
Mike, this looks close to landing.  What happens with a fresh try run?
Flags: needinfo?(mratcliffe)
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.
Yes, I remember that somebody fixed this and meant to hunt this bug down and close it.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: needinfo?(mratcliffe)
Resolution: --- → WORKSFORME
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.