1326753 - Impossible to inspect elements in iframe from context menu

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[arni2033]

>>>   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

Comment 1

[Julian Descottes [:jdescottes]]

Alex, do you have some bandwidth to investigate this regression? 
Would be nice to get it fixed before branching 53.

Comment 2

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28d8bc86415f

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1326753 - Fix inspect node from browser context menu against elements in iframes.

Review commit: https://reviewboard.mozilla.org/r/104150/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104150/

Comment 4

[Alexandre Poirot [:ochameau]]

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.

Comment 5

[Patrick Brosset <:pbro>]

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.

Comment 7

[Alexandre Poirot [:ochameau]]

(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?

Comment 8

[Patrick Brosset <:pbro>]

(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 9

[Alexandre Poirot [:ochameau]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104150/diff/1-2/

Comment 10

[Patrick Brosset <:pbro>]

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

Comment 11

[Alexandre Poirot [:ochameau]]

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.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2b174f46eb04

Comment 13

[Nazir Ahmed Sabbir [:NaSb]]

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)

Comment 15

[Sebastian Zartner [:sebo]]

As of comment 13.

Sebastian