609872 - Ability to execute code in sub-documents (iframes/frames)

Status: VERIFIED FIXED

(DevTools :: Console, enhancement, P3)

Description

[Maniac Vlad Florin (:vladmaniac)]

Build ID:

Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101103 Firefox/4.0b8pre

If you have any simple JS example, such as a simple alert box 
onclick, you can call that though the console without loading the actual 
page. Given that function intergrated in an iframe, it is not accessible 
any more from the web console. Can't do that even if moving the function 
to a sepparate .js file. 

This is not a "must", but would be very comforting to have. 

=========================================================================

Here is David's solution, which is simple and elegant, but I'm sure not 
all console users can come up with this: 

 
var iframeDoc = document.querySelectorAll("iframe")[0].contentWindow.document;

then you can call:

iframeDoc.fooFunction(); 

where fooFunction() is something you defined in your page and want to separately call from the Web Console.

Comment 1

[Kevin Dangoor]

I actually think that any behavior other than that which you describe would be confusing. The console would need to evaluate expressions in some sort of merged namespace, and randomly merging namespaces is likely to yield odd results.

FWIW, you can shorten your workaround like this:

var iframeDoc = $$('iframe')[0].contentWindow.document;

Comment 2

[Rob Campbell [:rc] (:robcee)]

alternatively, we could implement a "cd" helper function to change our context to the iframe's contentWindow. I believe Firebug has this.

Comment 3

[Kevin Dangoor]

Yeah, that's an option (though for me "cd" just seemed to send Firebug spinning). On the one hand, this seems like a bit of a niche need... but on the other, I can see someone with a lot of iframes wanting to be able to switch into one or another.

I'll repurpose the bug toward that end, though this is still a feature to come sometime after Firefox 4.

Comment 4

[Rob Campbell [:rc] (:robcee)]

yeah, cd never worked well in my experience. Neglected feature, I think.

Agree that we shouldn't hold for this bug. It's a feature request and would be nice to have.

Comment 7

[jdavid.net]

Chrome does have this behavior at the GUI level, and allows you to set the 'window' context, and the 'browser' context.  The window context, switches between layers of iframes, while the 'browser' context allows you to switch to extension background pages, content script context, and extension page context.

Comment 8

[jdavid.net]

doing this as defined in the 1st couple of comments is confusing because you don't know the clear scoping of variables, and secondly the scoping is mostly at the page level and not at the iframe level, so it's hard to access globals in the iframe context.

Comment 9

[Mihai Sucan [:msucan]]

This will be done with a nice UI, not as explained in the first comments. I marked the other bug as a duplicate of this one simply for tidying up - I've been doing web console bugs triage lately.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

This was probably the most requested feature in my devtools booth today. We should probably consider raising the priority on this. Doing a simple UI like Firebug's cd command would be fine by me:

http://getfirebug.com/wiki/index.php/Cd

Comment 12

[Joe Walker [:jwalker]]

Let's remember that JSTerm is coming quite soon.

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Do we have a plan for handling this in JSTerm? Could we just add a "cd" command there?

Comment 14

[Joe Walker [:jwalker]]

(In reply to Panos Astithas [:past] from comment #13)
> Do we have a plan for handling this in JSTerm? Could we just add a "cd"
> command there?

That's the plan. Except that I called the command 'global' because this has nothing to do with directories.

Comment 15

[Paul Rouget [:paul]]

Why "global"?

"cd" is what firebug users use. I think we should re-use it. Or maybe "context".

Comment 16

[Joe Walker [:jwalker]]

I don't like "cd" for the obvious reason that it's not about changing directories, but it is about changing the global object.

Comment 17

[Paul Rouget [:paul]]

(In reply to Joe Walker [:jwalker] from comment #16)
> I don't like "cd" for the obvious reason that it's not about changing
> directories, but it is about changing the global object.

Apparently it worked well for Firebug users. Let's at least have an alias.

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

I find the fact that other browsers didn't add this quite telling. I'm fine with either though.

Comment 19

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Chrome's solution is not terrible:

https://www.evernote.com/shard/s1/sh/154250a8-92e3-4b4e-9e99-249a5cea74cc/d3e890be27f1300481705a9199addaef/deep/0/Screen%20Shot%202013-10-11%20at%2011.51.03%20AM.png

I don't mind having a console helper to help navigate the frames, it's *also* great to have a visual list of frames like chrome does. 

Mihai - when you say this will be done with a 'nice ui', do we have design input there?

Comment 20

[Mihai Sucan [:msucan]]

Jeff, I meant something like a drop-down selection for picking the iframe based on URL, similar to that of Chrome. A |cd| command is not easily discoverable, but I do agree we also need a console helper.

On the topic of |cd| versus |global| I am of the opinion that |cd| is more appropriate - other tools use this, and there's not much benefit of using |global|.

Comment 21

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Mihai Sucan [:msucan] from comment #20)
> Jeff, I meant something like a drop-down selection for picking the iframe
> based on URL, similar to that of Chrome. A |cd| command is not easily
> discoverable, but I do agree we also need a console helper.

Ok, sounds good.

> On the topic of |cd| versus |global| I am of the opinion that |cd| is more
> appropriate - other tools use this, and there's not much benefit of using
> |global|.

I think I agree. When this gets landed make sure you log a docs bug.

Comment 22

[Paul Rouget [:paul]]

We want iframes and any sub-docshell (window.open(…))

Comment 23

[Tim Nguyen :ntim]

why not just a select box with all the availble frames, that would be way easier than a cd command.

Comment 24

[Hallvord R. M. Steen [:hallvors]]

I know from experience that debugging for example Facebook's apps/games is just about impossible without this feature. Voted, hope it's in progress.

Comment 25

[Mihai Sucan [:msucan]]

Attachment: wip1

Feature implementation - this is fully working. Need to write tests.

Comment 26

[Panos Astithas (he/him) [:past] (please ni?)]

CCing Alex who was trying something similar, albeit b2g-focused, today for inspiration.

Comment 27

[Alexandre Poirot [:ochameau]]

Ahah funny that we start working on that at the same time!

I got something working as well, but it would be at the toolbox level.
I'd easily imagine we want to end up with both.
"cd" in webconsole may only switch the console scope and not the whole toolbox one.

Comment 28

[Jeff Griffiths (:canuckistani) (:⚡︎)]

I think "cd" should switch the console as well as scratchpad and any other JS input we come up with. Conversely, if we implement UI to do this ( chrome has a drop list for example ) the effect should be the same.

Comment 29

[Mihai Sucan [:msucan]]

(In reply to Jeff Griffiths (:canuckistani) from comment #28)
> I think "cd" should switch the console as well as scratchpad and any other
> JS input we come up with. Conversely, if we implement UI to do this ( chrome
> has a drop list for example ) the effect should be the same.

True, but unfortunately this feature has been postponed for too long, in the hope we'll do actual work on a toolbox-level frame selection UI. We need that, but in the meantime, this is good enough.

As for Scratchpad: you should be able to cd() from it as well. I'm not yet sure if cd() calls are 'shared'. I need to recheck how scratchpad uses the connection and the console actor.


(In reply to Alexandre Poirot (:ochameau) from comment #27)
> Ahah funny that we start working on that at the same time!
> 
> I got something working as well, but it would be at the toolbox level.
> I'd easily imagine we want to end up with both.
> "cd" in webconsole may only switch the console scope and not the whole
> toolbox one.

This is great. You should continue the work on that, we still need a toolbox-level UI for frame selection.

Comment 30

[Tim Nguyen :ntim]

Opera dragonfly had a toolbox-level UI for Frame selection (and even tab selection I think).

Comment 31

[Mihai Sucan [:msucan]]

Attachment: bug609872-2.diff

Patch with tests.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=93365572d617

cd() changes the eval scope for the console actor. Scratchpad reuses the console actor, which means it gets the new eval scope (tested). Scratchpad users can use cd() as well, when they need to change the eval scope.

Looking forward to your review. Thanks!

(take your time with the review - I know you guys are busy in Portland!)

Comment 32

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8378401 [details] [diff] [review]
bug609872-2.diff

Review of attachment 8378401 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/webconsole.js
@@ +213,5 @@
> +      this._progressListenerActive = true;
> +    }
> +  },
> +
> +  _progressListenerActive: false,

Can you clarify the purpose of this flag in a comment? Doesn't it need to be reset somewhere (maybe in _onWillNavigate)?

@@ +835,5 @@
>        makeDebuggeeValue: aDebuggerGlobal.makeDebuggeeValue.bind(aDebuggerGlobal),
>        createValueGrip: this.createValueGrip.bind(this),
>        sandbox: Object.create(null),
>        helperResult: null,
> +      consoleActor: this,

Is this needed anywhere? It doesn't look like it is.

::: toolkit/devtools/webconsole/utils.js
@@ +1515,5 @@
> +   *        is used to perform document.querySelector(), to find the iframe that
> +   *        you want to cd() to. A DOMElement can be given as well, the
> +   *        .contentWindow property is used. Lastly, you can directly pass
> +   *        a window object. If you call cd() with no arguments, the current
> +   *        eval scope is cleared back to its default (the top window).

Firebug seems to only accept a window as a parameter, but being smarter here sounds useful and trumps strict compatibility IMO. It's not like these helpers are going to be standardized any time soon anyway.

Comment 33

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #32)
> Comment on attachment 8378401 [details] [diff] [review]
> bug609872-2.diff
> 
> Review of attachment 8378401 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/server/actors/webconsole.js
> @@ +213,5 @@
> > +      this._progressListenerActive = true;
> > +    }
> > +  },
> > +
> > +  _progressListenerActive: false,
> 
> Can you clarify the purpose of this flag in a comment? Doesn't it need to be
> reset somewhere (maybe in _onWillNavigate)?

I will add a comment. The progress listener doesnt need to be readded after navigation. The tab actor doesn't do it either. It continues to work. If I am not mistaken, the outer window is used. I will double-check this.


> @@ +835,5 @@
> >        makeDebuggeeValue: aDebuggerGlobal.makeDebuggeeValue.bind(aDebuggerGlobal),
> >        createValueGrip: this.createValueGrip.bind(this),
> >        sandbox: Object.create(null),
> >        helperResult: null,
> > +      consoleActor: this,
> 
> Is this needed anywhere? It doesn't look like it is.

Yes, check the cd() jsterm helper.


> ::: toolkit/devtools/webconsole/utils.js
> @@ +1515,5 @@
> > +   *        is used to perform document.querySelector(), to find the iframe that
> > +   *        you want to cd() to. A DOMElement can be given as well, the
> > +   *        .contentWindow property is used. Lastly, you can directly pass
> > +   *        a window object. If you call cd() with no arguments, the current
> > +   *        eval scope is cleared back to its default (the top window).
> 
> Firebug seems to only accept a window as a parameter, but being smarter here
> sounds useful and trumps strict compatibility IMO. It's not like these
> helpers are going to be standardized any time soon anyway.

Yep. It's nicer to do cd('iframe') than cd($('iframe').contentWindow).

Thanks for the review!

Comment 34

[Mihai Sucan [:msucan]]

Landed: https://hg.mozilla.org/integration/fx-team/rev/9be1d1bc11fc

I can confirm the progress listener continues to work after page reloads. However, I updated the patch to use once(), so the event listener is cleared when it fires, and it also clears the _progressListenerActive flag. I did this to avoid potentially subtle bugs in the future when/if the behavior of that progress listener changes. Also added a comment for the flag as suggested.

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9be1d1bc11fc

Comment 36

[Will Bamberg [:wbamberg]]

Mihai: https://developer.mozilla.org/en-US/docs/Tools/Web_Console#Working_with_iframes - does that look OK?

Comment 37

[Mihai Sucan [:msucan]]

(In reply to Will Bamberg [:wbamberg] from comment #36)
> Mihai:
> https://developer.mozilla.org/en-US/docs/Tools/
> Web_Console#Working_with_iframes - does that look OK?

Yes. Thank you! Much nicer explanation than what I came up with in my blog post and jsdoc. :)

Comment 38

[Andrei Vaida [:avaida]]

I was able to confirm the fix for this issue on Windows 7 64-bit [1], using the documentation from Comment 36 [2] with Firefox 30 Beta 5 (Build ID: 20140515140857).

[1] Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
[2] https://developer.mozilla.org/en-US/docs/Tools/Web_Console#Working_with_iframes