If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Impossible to inspect elements in iframe from context menu

VERIFIED FIXED in Firefox 53

Status

()

Firefox
Developer Tools: Inspector
P1
normal
VERIFIED FIXED
9 months ago
8 months ago

People

(Reporter: arni2033, Assigned: ochameau)

Tracking

(Depends on: 1 bug, {regression})

Trunk
Firefox 53
regression
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox52 unaffected, firefox53 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

9 months ago
>>>   My Info:   Win7_64, Nightly 53, 32bit, ID 20161219030207 (2016-12-19)
STR_1:
1. Open url   data:text/html,<iframe src="data:,Firefox regressions since version 28">
2. Right-click text "regression", click "Inspect Element (Q)"

AR:  No element is selected in markup
ER:  Element <pre> should be selected in markup

This is regression from bug 1154645. Regression range:
> https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c7c83bfdd97ad41bb33f7767f8d5a24cb64f4dd9&tochange=2d0a61de6a100621b53c5771c6701a2ddbc665a0
(Reporter)

Updated

9 months ago
Component: Untriaged → Developer Tools: Animation Inspector
(Reporter)

Updated

9 months ago
No longer blocks: 1277113
(Reporter)

Updated

9 months ago
Component: Developer Tools: Animation Inspector → Developer Tools: Inspector
Alex, do you have some bandwidth to investigate this regression? 
Would be nice to get it fixed before branching 53.
Flags: needinfo?(poirot.alex)
Priority: -- → P1
Depends on: 1326809
(Assignee)

Updated

8 months ago
Assignee: nobody → poirot.alex
Flags: needinfo?(poirot.alex)
(Assignee)

Comment 2

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28d8bc86415f
Comment hidden (mozreview-request)
(Assignee)

Comment 4

8 months ago
Comment on attachment 8826134 [details]
Bug 1326753 - Fix inspect node from browser context menu against elements in iframes.

In this patch I introduces "contentDocument" request on WalkerActor.
I imagine there is many other ways to address the lack of "cross iframe query selector". May be it is better to implement such cross iframe query selector method on the actor? Or use WalkerActor.children like this helper:
http://searchfox.org/mozilla-central/source/devtools/client/inspector/test/head.js#192-197
I also wonder if the cross iframe query selector code I put in devtools-browser should be move to some shared helper as it may be convenient for other usages.
(it is more powerful than getNodeFrontInFrame test helper)

I don't have enough experience on the various inspector actor and requests to make the call here.

If contentDocument looks good to you, I would like to introduce a test against it in inspector actor tests.
Attachment #8826134 - Flags: review?(pbrosset)

Comment 5

8 months ago
mozreview-review
Comment on attachment 8826134 [details]
Bug 1326753 - Fix inspect node from browser context menu against elements in iframes.

https://reviewboard.mozilla.org/r/104150/#review105992

Looks great Alex. Thanks for addressing this problem. 
Let me know what you think of using children instead of introducing a new method though.

::: devtools/client/framework/devtools-browser.js:267
(Diff revision 1)
> +          return Promise.resolve(nodeFront);
> +        }
> +        return inspector.walker.querySelector(nodeFront, selector)
> +          .then(node => {
> +            if (selectors.length > 0) {
> +              return inspector.walker.contentDocument(node);

I think the `children` method can do the same thing, I don't think we need to introduce a new one here.
`children` is the method we used to walk the DOM in the inspector, and it already knows how to go through iframes. So this, for instance, should work:

```
inspector.walker.children(node).then(children => {
  children[0] // This is the NodeFront for the document node inside the iframe
});
```

::: devtools/client/inspector/test/browser_inspector_inspect_node_contextmenu.js:39
(Diff revision 1)
> +  let b = inspector.breadcrumbs;
> +  let expectedText = b.prettyPrintNodeAsText(nodeFront);
> +  let button = b.container.querySelector("button[checked=true]");
> +  ok(button, "A crumbs is checked=true");
> +  is(button.getAttribute("title"), expectedText,
> +     "Crumb refers to the right node");

Why testing the breadcrumbs here? Seems unrelated to your change.
Attachment #8826134 - Flags: review?(pbrosset)

Updated

8 months ago
Duplicate of this bug: 1331775
status-firefox52: --- → unaffected
status-firefox53: --- → affected
(Assignee)

Comment 7

8 months ago
(In reply to Patrick Brosset <:pbro> from comment #5)
> Comment on attachment 8826134 [details]
> 
> ::: devtools/client/framework/devtools-browser.js:267
> (Diff revision 1)
> > +          return Promise.resolve(nodeFront);
> > +        }
> > +        return inspector.walker.querySelector(nodeFront, selector)
> > +          .then(node => {
> > +            if (selectors.length > 0) {
> > +              return inspector.walker.contentDocument(node);
> 
> I think the `children` method can do the same thing, I don't think we need
> to introduce a new one here.

I suggested this option in comment 4, I didn't knew about that method when I started writing this patch. I only discovered it while polishing the test...
I'm fine with using that. My only concern would be that children may be less efficient with the creation of a DocumentWalker. But given this action is a one shot, it should be fine.

> Why testing the breadcrumbs here? Seems unrelated to your change.

I just copied browser_inspector_initialization.js test, which ensures that both the markup view and breadcrumbs are ok, we may as well assert the rule view to be complete.
Do you thing I should keep only markup view assertions?
(In reply to Alexandre Poirot [:ochameau] from comment #7)
> > I think the `children` method can do the same thing, I don't think we need
> > to introduce a new one here.
> 
> I suggested this option in comment 4, I didn't knew about that method when I
> started writing this patch. I only discovered it while polishing the test...
Right, sorry for missing this.

> I'm fine with using that. My only concern would be that children may be less
> efficient with the creation of a DocumentWalker. But given this action is a
> one shot, it should be fine.
I agree, I don't think there are any performance concerns here. Let's go with that.

> > Why testing the breadcrumbs here? Seems unrelated to your change.
> 
> I just copied browser_inspector_initialization.js test, which ensures that
> both the markup view and breadcrumbs are ok, we may as well assert the rule
> view to be complete.
> Do you thing I should keep only markup view assertions?
My approach for this kind of change would be: assume there are other tests that already check that when a node is selected in the inspector, then the breadcrumbs and rule-view show the correct things (and there are), and try to write the smallest test possible.
Checking the breadcrumbs and rule-view here works for me too, I just think that use case is covered already.

(In reply to Alexandre Poirot [:ochameau] from comment #4)
> I also wonder if the cross iframe query selector code I put in
> devtools-browser should be move to some shared helper as it may be
> convenient for other usages.
> (it is more powerful than getNodeFrontInFrame test helper)
It does seem very useful indeed. I don't have any current use cases in mind for it though, but I haven't touched this part of the code in a while. Maybe people working on the inspector more than me now would find it useful. Maybe ask Gabriel and Julian?
Comment hidden (mozreview-request)

Comment 10

8 months ago
mozreview-review
Comment on attachment 8826134 [details]
Bug 1326753 - Fix inspect node from browser context menu against elements in iframes.

https://reviewboard.mozilla.org/r/104150/#review106608
Attachment #8826134 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 11

8 months ago
We silently regressed "inpect node" feature from browser context menu in bug 1154645.
Here this broke on documents having iframes. But it looks like it fixed bug 1326809 (would be good to confirm).
Bug 1154645 may have broke some other edge case, with document having specifics... (having special things in the html: svg, plugins, flash, canvas, special ad tricks?)
It would be great to test that bug 1154645 didn't introduced other regressions.
Flags: qe-verify+

Comment 12

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2b174f46eb04
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Successfully managed to reproduce this bug on Nightly 53.0a1 (2016-12-19)(64-bit) (Build ID: 20161219030207) by the following Comment 0's instruction!

This issue is now verified as fixed on Latest Firefox Nightly 53.0a1 (2017-01-19) (64-bit)

Build ID: 20170119222621
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
OS: Linux 4.4.0-57-generic; elementary OS 0.4 (64 Bit)
QA Whiteboard: [bugday-20170118]
Duplicate of this bug: 1327793
As of comment 13.

Sebastian
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
You need to log in before you can comment on or make changes to this bug.