1027991 - [system] Can't create child windows via window.open anymore

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

[Jim Porter (:squib)]

To triagers: I've requested blocking-b2g:2.0 on this because it breaks creating new ringtones from the ringtone manager. See below if you're interested in the details.

Since bug 1021658 landed, we can't create child windows (in-app sheets) anymore. I was using this for the ringtones app as follows:

1) Open the ringtone manager as an inline activity from the settings app
2) Click "+" which opens a pick activity, launching the music app
3) Pick a song from the music app, which returns the result to the ringtone manager
4) Taking the result from the pick activity, use window.open to open share.html (which is also a part of the ringtones app) and send it the pick activity's data via postMessage once share.html has loaded.

Step (4) used to create a child window, but now it creates a popup window. This adds an extra title bar from the system app, which looks weird. I'm also seeing this error some of the time:

E/GeckoConsole( 7239): [JavaScript Error: "TypeError: popup is null" {file: "app://ringtones.gaiamobile.org/gaia_build_defer_manage.js" line: 89}]

That's referring to the second line of this code[1]:

      var popup = window.open('share.html');
      popup.addEventListener('load', function loaded() { // popup is null here
        popup.removeEventListener('load', loaded);
        popup.postMessage(result, window.location.origin);
      });

It's not immediately obvious to me why window.open sometimes returns null.

I looked at bug 1021658, which sounds pretty serious, so we probably can't just re-enabled in-app sheets entirely. Maybe we could specify an argument to window.open to open a child window, like how you can pass 'dialog' or 'attention' now?

[1] https://github.com/mozilla-b2g/gaia/blob/bcb2115984ffdabc873f392678723f151cd880fd/apps/ringtones/js/manage.js#L27

Comment 1

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Jim Porter (:squib) from comment #0)
> To triagers: I've requested blocking-b2g:2.0 on this because it breaks
> creating new ringtones from the ringtone manager. See below if you're
> interested in the details.
> 
> Since bug 1021658 landed, we can't create child windows (in-app sheets)
> anymore. I was using this for the ringtones app as follows:
> 
> 1) Open the ringtone manager as an inline activity from the settings app
> 2) Click "+" which opens a pick activity, launching the music app
> 3) Pick a song from the music app, which returns the result to the ringtone
> manager
> 4) Taking the result from the pick activity, use window.open to open
> share.html (which is also a part of the ringtones app) and send it the pick
> activity's data via postMessage once share.html has loaded.
> 
> Step (4) used to create a child window, but now it creates a popup window.
> This adds an extra title bar from the system app, which looks weird. I'm
> also seeing this error some of the time:
> 
> E/GeckoConsole( 7239): [JavaScript Error: "TypeError: popup is null" {file:
> "app://ringtones.gaiamobile.org/gaia_build_defer_manage.js" line: 89}]
> 
> That's referring to the second line of this code[1]:
> 
>       var popup = window.open('share.html');
>       popup.addEventListener('load', function loaded() { // popup is null
> here
>         popup.removeEventListener('load', loaded);
>         popup.postMessage(result, window.location.origin);
>       });
> 
> It's not immediately obvious to me why window.open sometimes returns null.
> 
> I looked at bug 1021658, which sounds pretty serious, so we probably can't
> just re-enabled in-app sheets entirely. Maybe we could specify an argument
> to window.open to open a child window, like how you can pass 'dialog' or
> 'attention' now?
> 
> [1]
> https://github.com/mozilla-b2g/gaia/blob/
> bcb2115984ffdabc873f392678723f151cd880fd/apps/ringtones/js/manage.js#L27

Is it okay to you if I make the X button work again? Do you really want a in-app sheet or you just want to open another page and don't care it's a popup or sheet?

Comment 2

[Jim Porter (:squib)]

For this case, I'd like a sheet if at all possible (otherwise, I'll have to implement this in a totally different way), since popups have a header/title-bar created by the system app. We need to have our own header so that it uses the right color theme (the system app gives us a light one and we want a dark one) and also because our header has two buttons: an X on the left and "Save" on the right. I don't think the system app can do that for us.

Thanks!

Comment 3

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Jim Porter (:squib) from comment #2)
> For this case, I'd like a sheet if at all possible (otherwise, I'll have to
> implement this in a totally different way), since popups have a
> header/title-bar created by the system app. We need to have our own header
> so that it uses the right color theme (the system app gives us a light one
> and we want a dark one) and also because our header has two buttons: an X on
> the left and "Save" on the right. I don't think the system app can do that
> for us.
> 
> Thanks!

OK I understand what's you need now, thanks for the detail.
I am wondering if we want to reopen inner sheet to current app in v2.0 because it's still buggy.

Etienne, what do you think?

Comment 4

[Etienne Segonzac (:etienne)]

(In reply to Alive Kuo [:alive][NEEDINFO!][OOO from 6/23 until 6/30] from comment #3)
> (In reply to Jim Porter (:squib) from comment #2)
> > For this case, I'd like a sheet if at all possible (otherwise, I'll have to
> > implement this in a totally different way), since popups have a
> > header/title-bar created by the system app. We need to have our own header
> > so that it uses the right color theme (the system app gives us a light one
> > and we want a dark one) and also because our header has two buttons: an X on
> > the left and "Save" on the right. I don't think the system app can do that
> > for us.
> > 
> > Thanks!
> 
> OK I understand what's you need now, thanks for the detail.
> I am wondering if we want to reopen inner sheet to current app in v2.0
> because it's still buggy.
> 
> Etienne, what do you think?

It's fine as long as the app is showing a button that does window.close, and the ringtone app does that so it's fine for the ringtone apps.

Now, since we currently explicitly require the app to implement a close button, I think triggering inner sheets explicitly with a keyword makes more sense.
I know it's **** to have a virtually undocumented keyword like this but I don't think we have a better option.

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/20865

Proposed fix: use mozhaidasheet as keyword

Comment 6

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/45b6e78dffcfeb1c1213d32d24f3c61c84ca52d0

Comment 7

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/d6b21e1d08d33ff351691bc91375f20326ed990c