Closed Bug 1446572 Opened 7 years ago Closed 7 years ago

Can't inspect elements from targeted iframe

Categories

(DevTools :: Inspector, defect, P2)

Product:
DevTools
Component:
Inspector
Version:
53 Branch
Type:
defect

Tracking

(firefox-esr52 unaffected, firefox59 wontfix, firefox60 fixed, firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- wontfix
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: Oriol, Assigned: jryans)

References

Details

(Keywords: regression)

Attachments

(3 files)

testcase.htm
259 bytes, text/html
Details
Bug 1446572 - Record node inspection selectors starting with root.
59 bytes, text/x-review-board-request
pbro
: review+
RyanVM
: approval-mozilla-beta+
Details
Bug 1446572 - Adjust node inspection for toolbox's selected frame.
59 bytes, text/x-review-board-request
pbro
: review+
RyanVM
: approval-mozilla-beta+
Details
Attached file testcase.htm —
1. Load a page with an iframe 2. Open devtools 3. Select the iframe as the current targeted document. 4. Right-click some element inside the iframe and inspect it Expected: the element is shown in the inspector. Result: The <body> element of the iframe is shown instead. The browser console shows: Error writing request: children generateRequestMethods/</frontProto[name] resource://devtools/shared/protocol.js:1424:9 querySelectors resource://devtools/client/framework/devtools.js:640:31 inspectNode resource://devtools/client/framework/devtools.js:647:23 inspectNode chrome://devtools-shim/content/DevToolsShim.jsm:206:12 inspectNode chrome://browser/content/nsContextMenu.js:754:12 oncommand chrome://browser/content/browser.xul:1:1 TypeError: v is undefined resource://devtools/shared/protocol.js:321:7 Regression window: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=96cb95af530477edb66ae48d98c18533476e57bb&tochange=aa3e49299a3aa5cb0db570532e3df9e75d30c2d1 Probably bug 1326753.
Flags: needinfo?(poirot.alex)
Priority: -- → P2
Are we going to try to get this fixed in 59?
Flags: needinfo?(pbrosset)
I think the main problem is what to do when some iframe is targeted and you inspect an element from outside. I guess I would change the targeted iframe to be the nearest common iframe that contains the currently targeted iframe and the inspected element.
(In reply to Andrew Overholt [:overholt] from comment #1) > Are we going to try to get this fixed in 59? No. This only happens when using the iframe switcher button, which itself needs to be enabled first from the settings. So, it's one we need to fix for sure, but I'm not going to risk various uplifts for this if the fix is complex.
status-firefox59: affected → wontfix
Flags: needinfo?(pbrosset)
Alex, you're already needinfo'd here. What do you think of this: in the inspectNode method, we receive an array of nodeSelectors from the context menu component. In cases where an element inside frames has been selected, then the array will contain multiple entries, so that we can target each iframe and then eventually the node. So, my idea is, whenever we know we've selected an iframe using the frame switcher, we filter this array to only preserve the items that make sense. For instance in \devtools\client\framework\devtools.js in inspectNode, we add: // Filter the nodeSelectors in cases where we've selected a child frame. if (toolbox.selectedFrameId) { let currentFrameId = toolbox.selectedFrameId while (true) { let frame = toolbox.frameMap.get(currentFrameId); if (frame.parentID) { // There's a parent, start again from this parent currentFrameId = frame.parentID; nodeSelectors.pop(); } else { break; } } } This seems to work fine on the provided test case. But I would like your opinion on the approach.
(In reply to Patrick Brosset <:pbro> from comment #4) > For instance in \devtools\client\framework\devtools.js in inspectNode, we > add: > > // Filter the nodeSelectors in cases where we've selected a child frame. > if (toolbox.selectedFrameId) { > let currentFrameId = toolbox.selectedFrameId > while (true) { > let frame = toolbox.frameMap.get(currentFrameId); > if (frame.parentID) { > // There's a parent, start again from this parent > currentFrameId = frame.parentID; > nodeSelectors.pop(); > } else { > break; > } > } > } The overall idea looks good, but shouldn't you use shift instead of pop? Otherwise, instead of involving selectedFrameId/frameMap/parentID into inspectNode, I would suggest exposing an helper method returning the frame depth and using this helper from inspectNode. inspectNode is already complex enough.
Flags: needinfo?(poirot.alex)
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Comment on attachment 8963434 [details] Bug 1446572 - Record node inspection selectors starting with root. https://reviewboard.mozilla.org/r/232336/#review237848
Attachment #8963434 - Flags: review?(pbrosset) → review+
Comment on attachment 8963435 [details] Bug 1446572 - Adjust node inspection for toolbox's selected frame. https://reviewboard.mozilla.org/r/232338/#review237852 ::: devtools/client/inspector/test/browser_inspector_inspect_node_contextmenu.js:29 (Diff revision 1) > + }); > + > let tab = await addTab(TEST_URI); > let testActor = await getTestActorWithoutToolbox(tab); > > - await testContextMenuWithinIframe(testActor); > + // Use content menu with root frame selected in toolbox s/content/context ::: devtools/client/inspector/test/browser_inspector_inspect_node_contextmenu.js:55 (Diff revision 1) > + let inspector = getActiveInspector(); > + let { toolbox } = inspector; The `inspector` variable isn't used, maybe shorten this to one line: `let { toolbox } = getActiveInspector();`
Attachment #8963435 - Flags: review?(pbrosset) → review+
Comment on attachment 8963435 [details] Bug 1446572 - Adjust node inspection for toolbox's selected frame. https://reviewboard.mozilla.org/r/232338/#review237852 > s/content/context Thanks, fixed. > The `inspector` variable isn't used, maybe shorten this to one line: > `let { toolbox } = getActiveInspector();` Thanks for catching, fixed.
Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/autoland/rev/eac0947f9e41 Record node inspection selectors starting with root. r=pbro https://hg.mozilla.org/integration/autoland/rev/3fb23173dcbf Adjust node inspection for toolbox's selected frame. r=pbro
https://hg.mozilla.org/mozilla-central/rev/eac0947f9e41 https://hg.mozilla.org/mozilla-central/rev/3fb23173dcbf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment on attachment 8963434 [details] Bug 1446572 - Record node inspection selectors starting with root. Approval Request Comment [Feature/Bug causing the regression]: Broken by bug 1326753 in 53. [User impact if declined]: If you switch the toolbox to focus on an inner frame via the frame switching menu, node inspection via the context menu won't work. [Is this code covered by automated tests?]: Yes, tests were added for this. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: All patches in this bug. [Is the change risky?]: No. [Why is the change risky/not risky?]: The change is specific to the node inspection path way and has tests. [String changes made/needed]: None.
Attachment #8963434 - Flags: approval-mozilla-beta?
Comment on attachment 8963435 [details] Bug 1446572 - Adjust node inspection for toolbox's selected frame. Approval Request Comment See comment 15.
Attachment #8963435 - Flags: approval-mozilla-beta?
Comment on attachment 8963434 [details] Bug 1446572 - Record node inspection selectors starting with root. Fix for a longstanding Inspector regression with new tests added. Approved for 60.0b9.
Attachment #8963434 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8963435 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
The second patch needs rebasing for Beta uplift.
Flags: needinfo?(jryans)
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: