Martijn Wargers (dead) Assignee
	Description • 12 years ago

Window.open doesn't work with the mochitest app.

From bug 807137, comment 8:
"
You're running these tests inside <iframe mozbrowser>, I take it?

You have to handle the mozbrowseropenwindow and mozbrowserclose events (fired on the main iframe and the popup iframe, respectively), otherwise window.open and window.close won't do anything.

You'd want to do something like

iframe.addEventListener('mozbrowseropenwindow', function(e) {
  var popupIframe = e.detail.frameElement;
  popupIframe.addEventListener('mozbrowserclose', function(e) {
    document.body.removeChild(popupIframe);
  });

  popupIframe.addEventListener('mozbrowseropenwindow', function(e) {
    // yes, the popup can call window.open too!
  });

  document.body.appendElement(popupIframe);
});

in addition, you'd probably want to make the test fail if any popups are still open when the test completes.
"

I tried adding this code here:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtestsb2g.py#71

But that doesn't seem to work.
I can't seem to get the 'mozbrowseropenwindow' to fire on the 'container' iframe, even though I used:
container.setAttribute('mozbrowser', 'true');

At this point, I don't know where to add this code to the mochitest framework.

	Justin Lebar (not reading bugmail)
	Comment 1 • 11 years ago

This looks suspicious:

> let homescreen = document.getElementById('homescreen');
> let container = homescreen.contentWindow.document.getElementById('test-container');
> container.setAttribute('mozapp', 'http://mochi.test:8888/manifest.webapp');

I wonder if container is even a mozapp frame.  You can't set that property whenever you want; I'm pretty sure you have to set mozapp before you insert the iframe into the DOM.

Also you have to have the mozbrowser attribute on the frame before you insert it into the DOM, otherwise it's not an app.

What does container.hasAttribute('mozbrowser') return?  Do you get any other mozbrowser events on the frame (e.g. mozbrowserloadstart)?

	Martijn Wargers (dead) Assignee
	Comment 2 • 11 years ago

Ok, thanks. I'm on vacation for 2 weeks and don't have my computer with me.
I'll try your suggestion as soon as I can.

	Martijn Wargers (dead) Assignee
	Comment 3 • 11 years ago

I can't seem to find where the iframe id="test-container" is being created:
http://mxr.mozilla.org/mozilla-central/search?string=test-container

I can only find iframe id="testframe":
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/server.js#631

	Jonathan Griffin (:jgriffin)
	Comment 4 • 11 years ago

It's part of a gaia test app that mochitest is loaded in:

https://github.com/mozilla-b2g/gaia/blob/master/test_apps/test-container/index.html

	Martijn Wargers (dead) Assignee
	Comment 5 • 11 years ago

Ok, thanks.
I guess that this part should be removed then in order for it to work, perhaps:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtestsb2g.py#70
70 container.setAttribute('mozapp', 'http://mochi.test:8888/manifest.webapp');

	Jonathan Griffin (:jgriffin)
	Comment 6 • 11 years ago

Yes, that line seems redundant.

	Martijn Wargers (dead) Assignee
	Comment 7 • 11 years ago

Ok, got it working now. I can see problems with this patch, but it seems to make most of the mochitests working that uses window.open() calls.
I can see problems when doing 2 times window.open('blah.html', 'a', '') for instance. The 2nd time, no new window should be opened.
Window sizes are completely wrong of course, but I wonder if that even can be fixed.

I ran these through the content/ subdirectory for the tests that use window.open.
These were passing with this test:
        "content/base/test/test_bug426646.html": "",
        "content/base/test/test_bug557892.html": "",
        "content/events/test/test_bug322588.html" : "",
        "content/events/test/test_bug493251.html" : "",
        "content/events/test/test_bug545268.html" : "",
        "content/events/test/test_bug656379-1.html" : "",
        "content/html/content/test/test_iframe_sandbox_general.html" : "",
        "content/xml/document/test/test_bug691215.html" : "",
        "content/html/content/test/forms/test_formaction_attribute.html" : ""

These were still failing:
        "content/html/content/test/test_iframe_sandbox_navigation.html" : "unknown error",
        "content/html/content/test/test_fullscreen-api.html" : "focus issue, then full screen issue",
        "content/events/test/test_bug426082.html" : "forms.css has button:active:hover rule, b2g content.css has only button:acive rule",
        "content/html/document/test/test_bug741266.html" : "popup window needs correct size, also innerWidth",
        "content/xbl/test/test_bug310107.html" : "xbl not working currently",
        "content/media/test/test_delay_load.html" : "video files not found?",
        "content/html/document/test/test_bug199692.html" : "this needs popup window to have correct size",
        "content/events/test/test_bug659071.html" : "ctrl-wheel zoom not working",
        "content/html/document/test/test_bug369370.html" : "needs to be converted to specialpowers pref setting",
        "content/events/test/test_bug457672.html" : "needs to be converted to specialpowers pref setting",
        "content/base/test/test_x-frame-options.html" : "addobserver problem",
        "content/base/test/test_plugin_freezing.html": "test plugin needed",
        "content/base/test/test_bug326337.html": "document.domain setting?",
        "content/media/test/test_access_control.html" : "other domain",
        "content/events/test/test_wheel_default_action.html" : "specialpowers problem"

I'm surprised that window.opener seems to work magically, apparently. I wonder if doing stuff like window.opener.document.body.innerHTML = 'foopy' is working.

	Justin Lebar (not reading bugmail)
	Comment 8 • 11 years ago

> +  popupIframe.remote = 'true';
> +  popupIframe.mozbrowser = 'true';
> +  popupIframe.mozapp = 'http://mochi.test:8888/manifest.webapp';
> +  popupIframe.src = aEvent.detail.url;
> +  popupIframe.name = aEvent.detail.name;

None of this should be necessary.  If it is necessary, something is probably wrong.

> when doing 2 times window.open('blah.html', 'a', '') for instance. The 2nd time, no new window 
> should be opened.

This should work properly today.  Does it not work?

> I'm surprised that window.opener seems to work magically, apparently.

Yep.

> I wonder if doing stuff like window.opener.document.body.innerHTML = 'foopy' is working.

It should.

> +    container.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader.activateRemoteFrame();
> +    popupIframe.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader.activateRemoteFrame();

These should not be necessary.  If they are, something is wrong.

Can you please verify if things still work with these bits of code removed?

	Justin Lebar (not reading bugmail)
	Comment 9 • 11 years ago

> Window sizes are completely wrong of course, but I wonder if that even can be fixed.

Sure can, but you have to parse e.detail.features yourself.

	Martijn Wargers (dead) Assignee
	Comment 10 • 11 years ago

(In reply to Justin Lebar [:jlebar] from comment #8)
> > +    container.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader.activateRemoteFrame();
> > +    popupIframe.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader.activateRemoteFrame();
> 
> These should not be necessary.  If they are, something is wrong.
> 
> Can you please verify if things still work with these bits of code removed?

The activateRemoteFrame() stuff is needed, otherwise I get failures and timeouts.
The other things were indeed not needed.

	Justin Lebar (not reading bugmail)
	Comment 12 • 11 years ago

> The activateRemoteFrame() stuff is needed, otherwise I get failures and timeouts.

AFAICT activateRemoteFrame is not called outside this file, which makes me suspect pretty strongly that there's probably some other way to accomplish whatever this accomplishes.  If we can avoid calling it, that would be advantageous, since it would make the test environment more closely match the production environment, which does not make this call.

What are the failures and timeouts?  Does an iframe.focus() call suffice?  I also asked for help in bug 788866.

Since smaug already r+'ed an activateRemoteFrame, I'm not going to gate my review on it.

	Martijn Wargers (dead) Assignee
	Comment 13 • 11 years ago

Ok, .focus() on the iframe seems to work too, so using that instead of .activateRemoteFrame().

It makes me wonder what activateRemoteFrame() is useful for, though.

	Justin Lebar (not reading bugmail)
	Comment 15 • 11 years ago

> It makes me wonder what activateRemoteFrame() is useful for, though.

If you remove the other activateRemoteFrame() call in this file, nobody will be using it!

	Martijn Wargers (dead) Assignee
	Comment 16 • 11 years ago

(In reply to Justin Lebar [:jlebar] from comment #15)
> > It makes me wonder what activateRemoteFrame() is useful for, though.
> 
> If you remove the other activateRemoteFrame() call in this file, nobody will
> be using it!

Yeah, and since iframe.focus() seems to be doing the same thing, I wonder what the use case for activateRemoteFrame() is.

	Justin Lebar (not reading bugmail)
	Comment 17 • 11 years ago

I suspect it has to do with the subtlety in Olli's comment previously: /if the iframe is focusable/, then .focus() will work.  Perhaps there are cases when we have a remote frame that's inside something that's not focusable.  But I don't know more than that.

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed313842afec

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/ed313842afec

	Martijn Wargers (dead) Assignee
	Comment 20 • 11 years ago

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #7)
>         "content/html/document/test/test_bug369370.html" : "needs to be
> converted to specialpowers pref setting",
>         "content/events/test/test_bug457672.html" : "needs to be converted
> to specialpowers pref setting",

This is being done in bug 861674.