Closed Bug 1039528 Opened 10 years ago Closed 10 years ago

[e10s] "Inspect Element" page contextual menu doesn't work with e10s

Categories

(DevTools :: Inspector, defect)

Product:
DevTools
Component:
Inspector
Type:
defect
Priority:
Not set
Severity:
normal
Points:
8

Tracking

(e10sm3+)

Status:
VERIFIED FIXED
Milestone:
Firefox 35
Iteration:
35.3
Tracking Flags:
Tracking Status
e10s m3+ ---

People

(Reporter: pbro, Assigned: billm)

References

Details

(Whiteboard: [e10s-m3])

Attachments

(1 file, 2 obsolete files)

inspector-ctx-menu v3
(deleted), patch
pbro
: review+
Details | Diff | Splinter Review
STR: - Open a new e10s window - On any page, right-click a specific element - Choose "Inspect Element" => Expected: the inspector opens and the element is selected => Actual: The inspector opens and the <body> element (default selection) is selected See http://dxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#488 : inspectNode: function CM_inspectNode() { let {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}); let gBrowser = this.browser.ownerDocument.defaultView.gBrowser; let tt = devtools.TargetFactory.forTab(gBrowser.selectedTab); return gDevTools.showToolbox(tt, "inspector").then(function(toolbox) { let inspector = toolbox.getCurrentPanel(); inspector.selection.setNode(this.target, "browser-context-menu"); }.bind(this)); }, There are 2 problems with this: - this.target is a CPOW in e10s mode, not the real DOM node, - and anyway, this method uses inspector.selection.setNode which isn't remote-safe at all It should be using inspector.selection.setNodeFront instead, but to do that, it needs to get a reference to the NodeFront that corresponds to this.target. I can't see a way to do this yet though. Loading a frame script in the content process would help get the real node, but from that, there won't be any way of getting the NodeFront. This is a major problem with switching to E10S since this contextual-menu item probably represents the majority of use cases when inspecting a node.
This is pretty close to what 'pick an element' does with the highlighter. It is handled through _findAndAttachElement in highlighter.js. As you said: 'Loading a frame script in the content process would help get the real node'. So if we did that, then I think it would be as simple as a call to attachElement. // get realNode from CPOW somehow let nodeFront = inspector.walker.attachElement(realNode)
Blocks: dte10s
Brian and I discussed on IRC and the best idea that came up was to use coordinates instead of object references. We can get coordinates from the click event, and implement a new function on selection: selection.setNodeFromCoordinates(x, y) which would call a new method on the inspector actor that gets the node from the supplied coordinates. A little bit like the highlighter actor handles element picking: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/highlighter.js#198
Assignee: nobody → wmccloskey
Blocks: old-e10s-m2
tracking-e10s: ? → +
Flags: firefox-backlog+
Flags: firefox-backlog+
No longer blocks: old-e10s-m2
Whiteboard: [e10s-m3]
Flags: firefox-backlog+
Moving to M3 milestone. These bugs affect e10s dogfooding but Brad and Gavin did not think they were M2 blockers.
tracking-e10s: + → m3+
I have the same problem. Nightly under Open Suse.
Attached patch inspector-ctx-menu (obsolete) (deleted) — — Splinter Review
I tried to implement something here. When the user asks to inspect an element, we take the CPOW for the element and send it to a frame script. The frame script receives the element and tries to make an actor for it. Then it should return the actor ID to the parent process, which can then select the right element. I'm able to get the actor for the node in the child process. However, I can't figure out how to return it to the parent. If I just send the actor ID, the parent's connection has never heard of it. It seems like the child process somehow needs to send the node actor over the remote debugging channel so that the parent knows about it. I copied some code that sort of seems to do that, but I don't understand it. Patrick, could you give some feedback here?
Attachment #8494906 - Flags: feedback?(pbrosset)
Flags: qe-verify?
Comment on attachment 8494906 [details] [diff] [review] inspector-ctx-menu Review of attachment 8494906 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for working on this. I'm not confident this is the right approach though, but there's a number of things that I haven't worked too closely with, so I can't be sure. See my comments inline, most of them being of the type "I'm not sure ...". So this might work, but if you decide to go on with this approach, you should ping dcamp. One thing is I'm not sure the child.js frame script is loaded in all cases. In non e10s mode on desktop for instance, I don't think it is, so this fix would break the "inspect element" ctx menu for non e10s windows. Let me suggest another, simpler (I think) approach: The parent process knows which node has been right-clicked on (at least it has a cpow version of it, and can get more information via a frame script). So why not make the parent process open the toolbox (as it does right now) and send some information about that node to the walker actor, through the usual devtools protocol, in order to get a node front response. I think there are 2 types of information we could sent to uniquely identify the node: - a unique css selector - the coordinates of the right-click Using a css selector, we'd just need to call the WalkerActor's querySelector method: inspectNode: function CM_inspectNode() { let {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}); let gBrowser = this.browser.ownerDocument.defaultView.gBrowser; let tt = devtools.TargetFactory.forTab(gBrowser.selectedTab); return gDevTools.showToolbox(tt, "inspector").then(function(toolbox) { let inspector = toolbox.getCurrentPanel(); let uniqueSelector = getUniqueCssSelector(this.target); // that's the magic right here inspector.walker.querySelector(inspector.walker.rootNode, uniqueSelector).then(nodeFront => { inspector.selection.setNodeFront(nodeFront, "browser-context-menu"); }); }.bind(this)); }, Turns out computing a selector isn't that hard: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/styleinspector/css-logic.js#858 Using click coordinates, we'd need to implement a new protocol method on the WalkerActor, something like getNodeAt(coordinates): inspectNode: function CM_inspectNode() { let {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}); let gBrowser = this.browser.ownerDocument.defaultView.gBrowser; let tt = devtools.TargetFactory.forTab(gBrowser.selectedTab); return gDevTools.showToolbox(tt, "inspector").then(function(toolbox) { let inspector = toolbox.getCurrentPanel(); let coordinates = getCoordinates(this.target); inspector.walker.getNodeAt(coordinates).then(nodeFront => { inspector.selection.setNodeFront(nodeFront, "browser-context-menu"); }); }.bind(this)); }, This method should return something very similar to what querySelector returns (and hence, should have a WalkerFront's custom method implementation too: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#2539). I hope this helps. Don't hesitate to ping me on IRC for more info. ::: toolkit/devtools/server/actors/inspector.js @@ +2411,5 @@ > ownerNode: RetVal("nullable:disconnectedNode") > } > }), > + > + frontForRawNode: function(rawNode) { I think the whole content of this method can be changed with: return this.attachElement(rawNode); But anyway, since this function isn't called as a result of a devtools protocol request, it isn't going to lead to the expected result. Returning attachElement(node) normally makes sure the actor references all parents of the node at the same time, but since this isn't a protocol method, I don't think this mechanism will work and so the node and its parents won't be correctly known by the walker. @@ +2769,5 @@ > + let self = this; > + browser.messageManager.addMessageListener("debug:inspect", function handler(msg) { > + browser.messageManager.removeMessageListener("debug:inspect", handler); > + > + let nodeActor = self.conn._transport._serverConnection.getActor(msg.data); I think this isn't remote safe, this would only work on a local non-e10s tab. Also, why do you need the actor here? I think frontForCPOWNode needs to return a NodeFront (which is the client-side representation of a NodeActor). ::: toolkit/devtools/server/child.js @@ +67,5 @@ > + let onInspect = DevToolsUtils.makeInfallible(function(msg) { > + let inspectorID = msg.data.inspectorID; > + > + // FIXME: I'm not sure how to get the right connection. Will there > + // be more than one per tab? For now just pick one. I'm not sure if one connection per tab is an assumption we can make. I don't think anything in the devtools code ensures that this is true.
Attachment #8494906 - Flags: feedback?(pbrosset)
Attached patch inspector-ctx-menu v2 (obsolete) (deleted) — — Splinter Review
Thanks, that was very helpful. I tried the CSS selector method, but it doesn't work for iframes. I considered using the element's position, but it seems like it would also be complicated by iframes and scrolling. This patch seems simpler to me. It relies on a global variable, but otherwise I think it's pretty clean. I've tested it with iframes and it works.
Attachment #8494906 - Attachment is obsolete: true
Attachment #8495622 - Flags: feedback?(pbrosset)
Comment on attachment 8495622 [details] [diff] [review] inspector-ctx-menu v2 Review of attachment 8495622 [details] [diff] [review]: ----------------------------------------------------------------- This approach looks fine to me. I didn't think of iframes when proposing the selector solution indeed. The coordinates solution could work, we have a bunch of helpers in LayoutHelpers.jsm to help with this, but I like your solution better. ::: toolkit/devtools/server/actors/inspector.js @@ +169,5 @@ > exports.setValueSummaryLength = function(val) { > gValueSummaryLength = val; > }; > > +var gInspectingNode = null; Can you precede this line with a comment explaining what this is exactly, and when we use it? @@ +171,5 @@ > }; > > +var gInspectingNode = null; > + > +exports.setInspectingNode = function(val) { And this one by where this is expected to be called from. @@ +1622,5 @@ > ret.reverse(); > return ret; > }, > > + findInspectingNode: method(function() { This new protocol method also requires commenting. ::: toolkit/devtools/server/child.js @@ +65,5 @@ > } > }); > addMessageListener("debug:disconnect", onDisconnect); > + > + let onInspect = DevToolsUtils.makeInfallible(function(msg) { Also a comment here would help. The comment should probably mention that we're storing a global var on the inspector module.
Attachment #8495622 - Flags: feedback?(pbrosset) → feedback+
Attached patch inspector-ctx-menu v3 (deleted) — — Splinter Review
Thanks again.
Attachment #8495622 - Attachment is obsolete: true
Attachment #8496189 - Flags: review?(pbrosset)
Comment on attachment 8496189 [details] [diff] [review] inspector-ctx-menu v3 Review of attachment 8496189 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, thanks!
Attachment #8496189 - Flags: review?(pbrosset) → review+
https://hg.mozilla.org/mozilla-central/rev/af6565f01cf1
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Iteration: --- → 35.3
Points: --- → 8
Flags: qe-verify? → qe-verify+
QA Contact: bogdan.maris
Reproduced the initial issue using an old version of Nightly (2014-07-16), verified that after hitting Inspect Element the inspector opens and the element that is correctly selected using latest Nightly on Windows 7 64bit, Ubuntu 14.04 64bit and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: