Closed Bug 847518 Opened 7 years ago Closed 6 years ago

window.open(url, "_blank") should open in default browser

Categories

(Firefox Graveyard :: Webapp Runtime, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 30

People

(Reporter: dietrich, Assigned: marco)

References

Details

Attachments

(1 file, 7 obsolete files)

All desktop apps, and even app runtimes like Fluid and Ubuntu's webapps, open links in the system default browser.

Mozilla web apps open links in new windows with no controls, tied to the originating app window.

This is terribly broken behavior, once you start using Mozilla web apps for things that you actually work with every day - email, calendars, music apps, etc.

Your passwords aren't there, you have to re-login every time, you can't bookmark anything, etc, etc, etc.

Talked with Felipe a bit about it:

<felipe> dietrich: some apps needed to work with authentication popups from fb connect/twitter oauth etc, so that's why links opens in new webapp windows.. but if you set target="_blank" it will load in the default browser

<dietrich> felipe: hm, that seems backwards. like auth popups should specify new window, and default behavior should be like normal browser link handling

<felipe> dietrich: yeah, there's some backstory there to the current behavior.. but we went fine tuning as the use cases at the time wanted

<felipe> dietrich: basically the auth popups code usually need named windows, so they already had to do target="something"

<dietrich> felipe: hm, ok thanks for the backstory. kinda seems like it breaks how browsers behave in desktop OSes though :(
<dietrich> ubuntu's web apps don't behave this way
<dietrich> and Fluid doesn't either
see also: bug 707836, subsequent regressions bug 750568 and bug 729853, and then bug 752666
Indeed, webapps should open links to content in the default browser.  But they should also be able to open additional application windows, since windows are a core affordance of desktop platforms, and because third-party authentication providers like Persona require it.

sicking, I, and others looked into how to make both things possible and determined that the best way would be to open a link in the default browser when the link is specified via <link target="_blank"> and in a new application window otherwise.

See Gavin's bug references for the details, but the summary of our reasoning is that <link target="something"> is already a mature convention for opening new application windows (i.e. windows tied to the opener and with which the opener interacts); while <link target="_blank"> is already a mature convention for opening links to content in a new, unaffiliated browser tab, which is akin to opening them in the default browser, and it seems to be used for that purpose by popular content aggregators like Facebook, Twitter, and Google News.

Similar arguments apply to window.open(), which some apps use instead of <link> tags; and while we originally deferred the question, I think we ultimately decided that window.open(url, "_blank") should open in the default browser, while window.open(url, "something") should open in a new application window (cc: sicking for confirmation).

But we haven't implemented that yet in the desktop runtime.  So <link target="_blank"> opens in the default browser, but window.open(url, "_blank") still opens in a new application window.  Which is probably the cause of the behavior you're seeing.  And it is indeed incorrect.  And we should fix it.
OS: Mac OS X → All
Hardware: x86 → All
Summary: links from apps should open in default browser → window.open(url, "_blank") should open in default browser
On b2g we're creating a new iframe, on desktop we want to open the url in the default browser.
I think you have full control over this with mozbrowser.

When a mozbrowser tries to open a new window (via <a target="_blank">, or window.open(url, "_blank"), or whatever), the mozbrowser's "embedder" (the page which contains the mozbrowser) gets a mozbrowseropenwindow event.  That event contains all of the information passed to window.open().

Your options once you receive the event are to either

 a) Add the <iframe mozbrowser> provided in the event to somewhere in the DOM.   The new iframe's window will be returned to the page which called window.open.  Or,

 b) Don't add the <iframe mozbrowser> to somewhere in the DOM.  We'll return null to the page which called window.open.

If you take option (b), you can still open the given URI somewhere else.  By necessity, it will not be connected to the opener -- that is, its window.opener will be null.
(In reply to Justin Lebar [:jlebar] from comment #4)
> I think you have full control over this with mozbrowser.
> 
> When a mozbrowser tries to open a new window (via <a target="_blank">, or
> window.open(url, "_blank"), or whatever), the mozbrowser's "embedder" (the
> page which contains the mozbrowser) gets a mozbrowseropenwindow event.  That
> event contains all of the information passed to window.open().
> 
> Your options once you receive the event are to either
> 
>  a) Add the <iframe mozbrowser> provided in the event to somewhere in the
> DOM.   The new iframe's window will be returned to the page which called
> window.open.  Or,
> 
>  b) Don't add the <iframe mozbrowser> to somewhere in the DOM.  We'll return
> null to the page which called window.open.
> 
> If you take option (b), you can still open the given URI somewhere else.  By
> necessity, it will not be connected to the opener -- that is, its
> window.opener will be null.

On Desktop we don't embed the application in a <iframe> but in a <browser>.
I think we could send the URL to the default browser in nsGlobalWindow::Open either by checking a preference or by checking if we're an app (with a #define that makes the code run only on desktop).

Another option is switching to <iframe>, but in this case we'd need to modify more code in our implementation.
> On Desktop we don't embed the application in a <iframe> but in a <browser>.

How do you tell Gecko what app is associated with the <browser>?  Do you call a function on nsIDocShell or something?  I am not certain that works properly.  AFAIK the only correct, supported, and tested way to put an app inside a frame is to use <iframe mozbrowser mozapp>.

Anyway, it sounds like you understand your options wrt fixing this bug.  I'd really prefer not to add more ifdef/pref soup to the nsGlobalWindow::Open code, but I'll reserve judgement until we have a patch.
(In reply to Justin Lebar [:jlebar] from comment #6)
> How do you tell Gecko what app is associated with the <browser>?  Do you
> call a function on nsIDocShell or something?  I am not certain that works
> properly.  AFAIK the only correct, supported, and tested way to put an app
> inside a frame is to use <iframe mozbrowser mozapp>.

We're setting the docShell app ID, this allows us to support all the security features needed (the only thing we don't support is the "embed-apps" permission, but we don't need that on desktop).

> Anyway, it sounds like you understand your options wrt fixing this bug.  I'd
> really prefer not to add more ifdef/pref soup to the nsGlobalWindow::Open
> code, but I'll reserve judgement until we have a patch.

Another option would be to override window.open in the webapp runtime code.
> We're setting the docShell app ID, this allows us to support all the security features needed

If it works, it works, I guess.  But I doubt we have any tests for this setup.  It would be good to fix that.
Priority: -- → P2
Attached patch window_open_fix_v2 (obsolete) — Splinter Review
Could you review the nsGlobalWindow.cpp changes?
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
Attachment #794941 - Flags: review?(jst)
Comment on attachment 794941 [details] [diff] [review]
window_open_fix_v2

The nsGlobalWindow codee is all my code, so I can do it.  I don't feel comfortable reviewing the rest of the patch, though.
Attachment #794941 - Flags: review?(jst) → review?(justin.lebar+bug)
(In reply to Justin Lebar [:jlebar] from comment #10)
> Comment on attachment 794941 [details] [diff] [review]
> window_open_fix_v2
> 
> The nsGlobalWindow codee is all my code, so I can do it.  I don't feel
> comfortable reviewing the rest of the patch, though.

Thank you!
Attached patch window_open_fix_v2 (obsolete) — Splinter Review
Added test for the _blank case.
Attachment #794941 - Attachment is obsolete: true
Attachment #794941 - Flags: review?(justin.lebar+bug)
Attachment #794974 - Flags: review?(myk)
Attachment #794974 - Flags: review?(justin.lebar+bug)
Comment on attachment 794974 [details] [diff] [review]
window_open_fix_v2

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

I like the tests here a lot, especially the way they not only test that the expected behavior occurs but also that no unexpected behavior occurs alongside it.

I would just add tests for loading a non-app URL in _self and new windows within the app, to make sure those get loaded in the current/a new window in the app, while also marking the frame as not (or no longer) being an app frame.


Note that the patch no longer applies, although it's trivial to fix the conflicts.


(In reply to Justin Lebar [:jlebar] from comment #10)
> The nsGlobalWindow codee is all my code, so I can do it.  I don't feel
> comfortable reviewing the rest of the patch, though.

No worries, I can review the rest of it!

::: webapprt/WebappRT.jsm
@@ +18,5 @@
>  });
>  
> +XPCOMUtils.defineLazyGetter(this, "DOMApplicationRegistry", function() {
> +  Cu.import("resource://gre/modules/Webapps.jsm");
> +  return DOMApplicationRegistry;

In addition to returning the symbol in question, this also imports Webapps.jsm symbols into the global namespace, which is generally undesirable (even if it doesn't matter at the moment, because that module only exports DOMApplicationRegistry right now).

To prevent that from happening, we could do what other code does (f.e. <https://github.com/mozilla/mozilla-central/blob/0a2f887c304cc40a93e5ed518b2c1617d8b756a4/browser/base/content/browser.js#L111-L115>) and import the symbol into a temporary object:

  XPCOMUtils.defineLazyGetter(this, "DOMApplicationRegistry", function() {
    let tmp = {};
    Cu.import("resource://gre/modules/Webapps.jsm", tmp);
    return tmp.DOMApplicationRegistry;
  });

But the better solution is to use defineLazyModuleGetter, which does that for you <https://github.com/mozilla/mozilla-central/blob/1deef8e2595e8947552b03d4be0f49784c23c380/js/xpconnect/loader/XPCOMUtils.jsm#L206-L229>.

(We should also fix the other use of defineLazyGetter in this file, which has the same problem.)

::: webapprt/content/webapp.js
@@ +48,5 @@
> +function onOpenWindow(event) {
> +  let features = event.detail.features;
> +
> +  if (features.indexOf("_blank") != -1 ||
> +      event.detail.name.indexOf("_blank") != -1) {

We should be more precise about determining the target of the window, splitting features into an array:

  features.split(",").indexOf("_blank") != -1

(Or whatever substring will appear as an entire field in the features string.)

However, in my testing, the string didn't contain such a substring.  What are the circumstances under which it does, and what specifically triggers that?

We should also check that event.detail.name is the exact string "_blank":

  event.detail.name == "_blank"

@@ +57,5 @@
> +    getService(Ci.nsIExternalProtocolService).
> +    getProtocolHandlerInfo(uri.scheme).
> +    launchWithURI(uri);
> +  } else if (features.indexOf("_self") != -1) {
> +    gAppBrowser.setAttribute("src", event.detail.url);

If the URL in question is not an app URL, i.e. if it belongs to another origin (or, in a world with multiple apps per origin, perhaps meets some other criteria), then this needs to unset the "app" status of the docshell.  Otherwise, it risks giving the app's privileges any page on the web.

@@ +63,5 @@
> +    let win = Services.ww.openWindow(window,
> +                                     "chrome://webapprt/content/webapp.xul",
> +                                     event.detail.name,
> +                                     "chrome," + features,
> +                                     null);

If the app has fullscreen: true, then this causes the window to open in fullscreen mode (with some issues on Mac, where the new window moves to its own space, but it retains its size and sprouts a black title bar and a thin black bar at the bottom of the window).

We shouldn't open subsequent windows in fullscreen mode, even if fullscreen: true, since that field is only intended to specify what to do with the initial window on startup.

We might want to let apps specify whether or not to open subsequent windows in fullscreen mode, f.e. with a window feature; but we should do that separately, looping in the apps leads and architects on any such change, especially given what the window.open docs say about the fullscreen=true feature that IE implemented a while ago:

https://developer.mozilla.org/en-US/docs/Web/API/window.open

And finally, we should make sure that any window at a non-app URL shows the title bar, since the title bar is how we warn the user about off-app content.  In practice, I think this means that such windows cannot be in fullscreen mode, and if they're in fullscreen mode when the navigation happens, then we have to turn off fullscreen mode for them, even for the initial window, if it gets navigated elsewhere.

@@ +65,5 @@
> +                                     event.detail.name,
> +                                     "chrome," + features,
> +                                     null);
> +
> +    win.addEventListener("load", function onLoad() {

Can we do this a bit sooner by listening for DOMContentLoaded?

@@ +67,5 @@
> +                                     null);
> +
> +    win.addEventListener("load", function onLoad() {
> +      win.removeEventListener("load", onLoad, false);
> +      win.document.getElementById("content").docShell.setIsApp(WebappRT.appID);

As with _self, we should only setIsApp on the frame in the new window if the URL we load in it is an app URL.
Attachment #794974 - Flags: review?(myk) → review-
(In reply to Myk Melez [:myk] [@mykmelez] from comment #13) 
> However, in my testing, the string didn't contain such a substring.  What
> are the circumstances under which it does, and what specifically triggers
> that?
> 
> We should also check that event.detail.name is the exact string "_blank":
> 
>   event.detail.name == "_blank"

My bad, I thought "_blank" could be a feature :D

> @@ +57,5 @@
> > +    getService(Ci.nsIExternalProtocolService).
> > +    getProtocolHandlerInfo(uri.scheme).
> > +    launchWithURI(uri);
> > +  } else if (features.indexOf("_self") != -1) {
> > +    gAppBrowser.setAttribute("src", event.detail.url);
> 
> If the URL in question is not an app URL, i.e. if it belongs to another
> origin (or, in a world with multiple apps per origin, perhaps meets some
> other criteria), then this needs to unset the "app" status of the docshell. 
> Otherwise, it risks giving the app's privileges any page on the web.

I think we have the security features anyway, because security checks are based on an extended origin, that contains both the host and the app id.
For example: http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#1081
So I think we should still set the app ID to say that the content belongs to an app window (because some features depend on that).
We may want to get an opinion from someone more expert with security here, though.
Comment on attachment 794974 [details] [diff] [review]
window_open_fix_v2

Looking through this code more closely, this makes GetRealFrameElement behave
just like GetFrameElementInternal.

I don't think you want to do this; I think you probably just want the
browser-element code to call GetFrameElementInternal.

Sorry I lead you astray when we talked about this on IRC!

Note that Friday is my last day at Mozilla.  You can probably get bz or smaug to review this change, particularly if it's as simple as I think it should be.  (I hope I'm right!)
Attachment #794974 - Flags: review?(justin.lebar+bug) → review-
Attached patch Patch (obsolete) — Splinter Review
(In reply to Myk Melez [:myk] [@mykmelez] from comment #13)
> And finally, we should make sure that any window at a non-app URL shows the
> title bar, since the title bar is how we warn the user about off-app
> content.  In practice, I think this means that such windows cannot be in
> fullscreen mode, and if they're in fullscreen mode when the navigation
> happens, then we have to turn off fullscreen mode for them, even for the
> initial window, if it gets navigated elsewhere.

Filed bug 912301 for this.
Attachment #794974 - Attachment is obsolete: true
Attachment #799196 - Flags: review?(myk)
Attachment #799196 - Flags: review?(bzbarsky)
Comment on attachment 799196 [details] [diff] [review]
Patch

r=me on the BrowserElementParent.cpp bits.
Attachment #799196 - Flags: review?(bzbarsky) → review+
Comment on attachment 799196 [details] [diff] [review]
Patch

(In reply to Marco Castelluccio [:marco] from comment #14)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #13) 
> > @@ +57,5 @@
> > > +    getService(Ci.nsIExternalProtocolService).
> > > +    getProtocolHandlerInfo(uri.scheme).
> > > +    launchWithURI(uri);
> > > +  } else if (features.indexOf("_self") != -1) {
> > > +    gAppBrowser.setAttribute("src", event.detail.url);
> > 
> > If the URL in question is not an app URL, i.e. if it belongs to another
> > origin (or, in a world with multiple apps per origin, perhaps meets some
> > other criteria), then this needs to unset the "app" status of the docshell. 
> > Otherwise, it risks giving the app's privileges any page on the web.
> 
> I think we have the security features anyway, because security checks are
> based on an extended origin, that contains both the host and the app id.
> For example:
> http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/
> nsPermissionManager.cpp#1081
> So I think we should still set the app ID to say that the content belongs to
> an app window (because some features depend on that).
> We may want to get an opinion from someone more expert with security here,
> though.

Yes, we should get input from someone more familiar with this code.  It looks like jlebar was responsible for the original implementation; perhaps bz can give feedback on this (he already gave r+, but presumably that was only for the platform code he reviewed).


(In reply to Myk Melez [:myk] [@mykmelez] from comment #13)

> If the app has fullscreen: true, then this causes the window to open in
> fullscreen mode (with some issues on Mac, where the new window moves to its
> own space, but it retains its size and sprouts a black title bar and a thin
> black bar at the bottom of the window).
> 
> We shouldn't open subsequent windows in fullscreen mode, even if fullscreen:
> true, since that field is only intended to specify what to do with the
> initial window on startup.

This patch still has this problem.
Attachment #799196 - Flags: review?(myk)
Attachment #799196 - Flags: review-
Attachment #799196 - Flags: feedback?(bzbarsky)
(In reply to Myk Melez [:myk] [@mykmelez] from comment #18)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #13)
> 
> > If the app has fullscreen: true, then this causes the window to open in
> > fullscreen mode (with some issues on Mac, where the new window moves to its
> > own space, but it retains its size and sprouts a black title bar and a thin
> > black bar at the bottom of the window).
> > 
> > We shouldn't open subsequent windows in fullscreen mode, even if fullscreen:
> > true, since that field is only intended to specify what to do with the
> > initial window on startup.
> 
> This patch still has this problem.

Ah, I though moving the fullscreen initialization in the Startup module would've fixed this. I didn't actually test, though.
Anyway something has changed in the last few days and the patch doesn't work anymore (it works for links, but doesn't work for opening windows). The tests are still working, though...
It was my app that was buggy :D
(In reply to Myk Melez [:myk] [@mykmelez] from comment #18)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #13)
> 
> > If the app has fullscreen: true, then this causes the window to open in
> > fullscreen mode (with some issues on Mac, where the new window moves to its
> > own space, but it retains its size and sprouts a black title bar and a thin
> > black bar at the bottom of the window).
> > 
> > We shouldn't open subsequent windows in fullscreen mode, even if fullscreen:
> > true, since that field is only intended to specify what to do with the
> > initial window on startup.
> 
> This patch still has this problem.

Ok, looks like this problem is Mac-only.
Attached patch Patch (obsolete) — Splinter Review
I've fixed the problem on Mac by canceling fullscreen mode before opening the new window.
This (opening new windows while in fullscreen mode) isn't a really common use case, so I don't know what native apps usually do, but I think this should be the correct behavior on all the platforms (otherwise the first window would still be in fullscreen mode, it'd be weird).
Attachment #799196 - Attachment is obsolete: true
Attachment #799196 - Flags: feedback?(bzbarsky)
Attachment #802738 - Flags: review?(myk)
Attachment #802738 - Flags: feedback?(bzbarsky)
(In reply to Marco Castelluccio [:marco] from comment #23)
> I've fixed the problem on Mac by canceling fullscreen mode before opening
> the new window.
> This (opening new windows while in fullscreen mode) isn't a really common
> use case, so I don't know what native apps usually do, but I think this
> should be the correct behavior on all the platforms (otherwise the first
> window would still be in fullscreen mode, it'd be weird).

Apple's Terminal, Mozilla's Firefox, and ActiveState's Komodo all open the second window in fullscreen mode, so it seems like that is the expected behavior.  But Komodo, which is also built on the Mozilla platform, has the same issue as the runtime, so Terminal and Firefox must be doing something differently to open the second window correctly.

And it may be simply that they open the window in regular mode and then switch it to fullscreen after it's open, which is what appears to be the case, judging by the appearance of the window open animation in those apps.
(In reply to Myk Melez [:myk] [@mykmelez] from comment #24)
> (In reply to Marco Castelluccio [:marco] from comment #23)
> > I've fixed the problem on Mac by canceling fullscreen mode before opening
> > the new window.
> > This (opening new windows while in fullscreen mode) isn't a really common
> > use case, so I don't know what native apps usually do, but I think this
> > should be the correct behavior on all the platforms (otherwise the first
> > window would still be in fullscreen mode, it'd be weird).
> 
> Apple's Terminal, Mozilla's Firefox, and ActiveState's Komodo all open the
> second window in fullscreen mode, so it seems like that is the expected
> behavior.  But Komodo, which is also built on the Mozilla platform, has the
> same issue as the runtime, so Terminal and Firefox must be doing something
> differently to open the second window correctly.

Ah ok, instead a page that is using the fullscreen API will exit fullscreen mode to open a new tab.
Comment on attachment 802738 [details] [diff] [review]
Patch

I'm not really that familiar with the app id stuff.  Trying sicking...
Attachment #802738 - Flags: feedback?(bzbarsky) → feedback?(jonas)
I've managed to fix the problem on Mac. I had to use window.openDialog instead of window.open or the WindowWatcher service.
I haven't dug deep, but looks like both window.open and the WW service use nsIWindowProvider while window.openDialog doesn't, so probably there's a bug there.

I had to switch back to a "load" event listener, because my manual test with "DOMContentLoaded" wasn't working correctly.

I've tried to follow Firefox's behavior (in the fullscreen case):
On Mac we open a new window in a new virtual desktop.
On Linux we open a new fullscreen window (note that it's impossible to exit fullscreen mode in the new opened window with the ESC button, both with Firefox and the Webapp Runtime. So this should be a platform bug)
On Win, I haven't tested yet.
On Win the behavior is different, the second window shouldn't be opened in fullscreen mode. I've fixed this.

There's still a Windows-only problem, a call to window.open(url, "_self") makes the application exit fullscreen mode...
Attached patch Patch (obsolete) — Splinter Review
There's still the window.open(url, "_self") problem, but it's unrelated to this patch so I think we should fix it in another bug.

It's funny because the three platforms do different things:
Windows - Exit fullscreen mode, both if the app is opened from cmdline or from a shortcut
Mac - Doesn't exit fullscreen mode if the app is opened from cmdline. Exit fullscreen mode if the app is executed from Launchpad or the dock.
Linux - Always in fullscreen mode.
So at the beginning I thought it was a Windows-only problem because I usually execute apps from the command line (to see if they print useful debug messages).
Attachment #802738 - Attachment is obsolete: true
Attachment #802738 - Flags: review?(myk)
Attachment #802738 - Flags: feedback?(jonas)
Attachment #804096 - Flags: review?(myk)
Attachment #804096 - Flags: feedback?(jonas)
Comment on attachment 804096 [details] [diff] [review]
Patch

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

(In reply to Marco Castelluccio [:marco] from comment #28)
> I've managed to fix the problem on Mac. I had to use window.openDialog
> instead of window.open or the WindowWatcher service.
> I haven't dug deep, but looks like both window.open and the WW service use
> nsIWindowProvider while window.openDialog doesn't, so probably there's a bug
> there.

Can you file a bug on that?


> I had to switch back to a "load" event listener, because my manual test with
> "DOMContentLoaded" wasn't working correctly.

Ok, no worries.


> I've tried to follow Firefox's behavior (in the fullscreen case):
> On Mac we open a new window in a new virtual desktop.
> On Linux we open a new fullscreen window (note that it's impossible to exit
> fullscreen mode in the new opened window with the ESC button, both with
> Firefox and the Webapp Runtime. So this should be a platform bug)

Can you file a bug on this?


(In reply to Marco Castelluccio [:marco] from comment #30)
> Created attachment 804096 [details] [diff] [review]
> Patch
> 
> There's still the window.open(url, "_self") problem, but it's unrelated to
> this patch so I think we should fix it in another bug.

Ok.


> It's funny because the three platforms do different things:
> Windows - Exit fullscreen mode, both if the app is opened from cmdline or
> from a shortcut
> Mac - Doesn't exit fullscreen mode if the app is opened from cmdline. Exit
> fullscreen mode if the app is executed from Launchpad or the dock.
> Linux - Always in fullscreen mode.
> So at the beginning I thought it was a Windows-only problem because I
> usually execute apps from the command line (to see if they print useful
> debug messages).

Welcome to the wonderfully wonky world of cross-platform development! ;-)

On a more serious note: getting these details right is a pain, but they make the difference between a comfortable experience and the uncanny valley <http://en.wikipedia.org/wiki/Uncanny_valley>.  So it's important to pay attention to platform-specific behavior and emulate it as closely as feasible (modulo other factors, like inconsistencies within platforms and appearance/behavior we intentionally want to make consistent across platforms).


(In reply to Marco Castelluccio [:marco] from comment #16)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #13)
> > And finally, we should make sure that any window at a non-app URL shows the
> > title bar, since the title bar is how we warn the user about off-app
> > content.  In practice, I think this means that such windows cannot be in
> > fullscreen mode, and if they're in fullscreen mode when the navigation
> > happens, then we have to turn off fullscreen mode for them, even for the
> > initial window, if it gets navigated elsewhere.
> 
> Filed bug 912301 for this.

That bug is marked fixed, but opening a new window to an external origin still opens the window fullscreen.  Would you like to fix that here or in a new bug?

::: dom/browser-element/BrowserElementParent.cpp
@@ +239,5 @@
>    nsCOMPtr<nsIDOMWindow> topWindow;
>    aOpenerWindow->GetScriptableTop(getter_AddRefs(topWindow));
>  
> +  nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(topWindow);
> + 

Nit: extra space character.

::: webapprt/content/webapp.js
@@ +71,5 @@
> +#ifndef XP_MACOSX
> +#ifndef XP_WIN
> +      // On Linux and Mac we open new windows in fullscreen mode if the opener
> +      // window is in fullscreen mode, but on Mac we don't need to hide the
> +      // menubar.

Nit: this comment was hard to understand at first, especially given the order of the #ifndefs.  I think it would make more sense like this:

#ifndef XP_WIN
#ifndef XP_MACOSX
      // On non-Windows platforms, we open new windows in fullscreen mode
      // if the opener window is in fullscreen mode, so we hide the menubar;
      // but on Mac we don't need to hide the menubar.
Attachment #804096 - Flags: review?(myk) → review+
(In reply to Myk Melez [:myk] [@mykmelez] from comment #31)
> That bug is marked fixed, but opening a new window to an external origin
> still opens the window fullscreen.  Would you like to fix that here or in a
> new bug?

I've fixed this problem and filed the necessary bugs. I'll wait Jonas' feedback before posting the new patch (that is mostly similar to the already reviewed one).
Feedback ping?
Let's move forward with the patch in the absence of Jonas's feedback.  We can always update the implementation if he decides to provide input at a later date.
Comment on attachment 804096 [details] [diff] [review]
Patch

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

I don't really know the relevant code here enough to comment on the patch itself.

If what it does is to ensure that all navigations targetted at _blank open in the default browser, then that sounds good. We should make sure that this covers all of <a target="_blank">, <form target="_blank"> and window.open(url, "_blank").

::: webapprt/Startup.jsm
@@ +97,5 @@
> +      // otherwise the app isn't aware of the fullscreen status.
> +      appBrowser.addEventListener("load", function onLoad() {
> +        appBrowser.removeEventListener("load", onLoad, true);
> +        appBrowser.contentDocument.
> +          documentElement.wrappedJSObject.mozRequestFullScreen();

Why are you using wrappedJSObject here? Doesn't that create a security problem? At the very least won't that mean that strange things happen if the page overrides that function?
Attachment #804096 - Flags: feedback?(jonas)
Attached patch Patch (obsolete) — Splinter Review
There's still the problem on Mac that if the app is in fullscreen mode all the new windows will be opened in fullscreen mode (even if they're off-origin).
Can we fix this later?

(In reply to Jonas Sicking (:sicking) needinfo? or feedback? encouraged. from comment #35)
> I don't really know the relevant code here enough to comment on the patch
> itself.

The feedback was requested to ensure comment 14 is correct.

> ::: webapprt/Startup.jsm
> @@ +97,5 @@
> > +      // otherwise the app isn't aware of the fullscreen status.
> > +      appBrowser.addEventListener("load", function onLoad() {
> > +        appBrowser.removeEventListener("load", onLoad, true);
> > +        appBrowser.contentDocument.
> > +          documentElement.wrappedJSObject.mozRequestFullScreen();
> 
> Why are you using wrappedJSObject here? Doesn't that create a security
> problem? At the very least won't that mean that strange things happen if the
> page overrides that function?

I think wrappedJSObject is safe (at least if https://developer.mozilla.org/en-US/docs/Mozilla/XPConnect/XPConnect_wrappers#XPCSafeJSObjectWrapper and https://developer.mozilla.org/en-US/docs/Safely_accessing_content_DOM_from_chrome are up to date).
Attachment #804096 - Attachment is obsolete: true
Attachment #8349749 - Flags: review?(myk)
(In reply to Marco Castelluccio [:marco] from comment #36)
> I think wrappedJSObject is safe (at least if
> https://developer.mozilla.org/en-US/docs/Mozilla/XPConnect/
> XPConnect_wrappers#XPCSafeJSObjectWrapper and
> https://developer.mozilla.org/en-US/docs/
> Safely_accessing_content_DOM_from_chrome are up to date).

"safe", sure, but it still seems unnecessary... you're calling a "native" method.
wrappedJSObject is "safe" in that it won't run page script with chrome privileges.

But it also offers no guaruantees that the names will map to what you expect.  So documentElement.wrappedJSObject.mozRequestFullScreen() might end up actually calling window.close() in this case, say.
Comment on attachment 8349749 [details] [diff] [review]
Patch

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

(In reply to Marco Castelluccio [:marco] from comment #36)
> There's still the problem on Mac that if the app is in fullscreen mode all
> the new windows will be opened in fullscreen mode (even if they're
> off-origin).
> Can we fix this later?

Yes, since that's an existing bug, not a regression from this patch, and it is not the subject of this bug.  But can you file a bug on it?  I don't see one at the moment (bug 912301 is related but not the same).


Another issue I just noticed with this patch on Mac is that opening a second window fullscreen and then taking it out of fullscreen mode causes the first window to leave fullscreen mode as well.  That's a regression from the previous behavior, which would take the second window out of fullscreen mode but leave the first window fullscreen.

And I think we want the original behavior, since that's how other Mac apps behave (f.e. Terminal, Sublime Text, and Firefox).

::: webapprt/Startup.jsm
@@ +146,5 @@
> +      // otherwise the app isn't aware of the fullscreen status.
> +      appBrowser.addEventListener("load", function onLoad() {
> +        appBrowser.removeEventListener("load", onLoad, true);
> +        appBrowser.contentDocument.
> +          documentElement.wrappedJSObject.mozRequestFullScreen();

As "Vacation" (the hacker who used to be known as bz) has noted, this may be "safe" in one sense, but it doesn't necessarily do what you expect, and it isn't necessary to unwrap the object, as you should be able to call mozRequestFullScreen on the wrapped variant.

::: webapprt/content/webapp.js
@@ +25,5 @@
> +  if (origin != WebappRT.config.app.origin) {
> +    return false;
> +  }
> +
> +  return true;

Nit: this can be simply:

  return (origin == WebappRT.config.app.origin);
Attachment #8349749 - Flags: review?(myk) → review-
Attached patch Patch (obsolete) — Splinter Review
(In reply to Myk Melez [:myk] [@mykmelez] from comment #39)
> Another issue I just noticed with this patch on Mac is that opening a second
> window fullscreen and then taking it out of fullscreen mode causes the first
> window to leave fullscreen mode as well.  That's a regression from the
> previous behavior, which would take the second window out of fullscreen mode
> but leave the first window fullscreen.

I've fixed this problem.
Attachment #8349749 - Attachment is obsolete: true
Attachment #8374920 - Flags: review?(myk)
Comment on attachment 8374920 [details] [diff] [review]
Patch

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

Looks and works great, modulo a trivial merge conflict. r=myk

Marco: when updating this patch to resolve the merge conflict, add "r=myk,bz" to the comment, as it might not be obvious to the committer that bz reviewed the BrowserElementParent.cpp bits (all the way back in comment 17).

::: webapprt/content/webapp.js
@@ +88,5 @@
> +
> +    win.addEventListener("load", function onLoad() {
> +      win.removeEventListener("load", onLoad, false);
> +
> +

Nit: one line of whitespace should be sufficient.
Attachment #8374920 - Flags: review?(myk) → review+
Attached patch PatchSplinter Review
Carrying r+.
Attachment #8374920 - Attachment is obsolete: true
Attachment #8383001 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8693eace6c33
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
I run Tweetdeck as an app, and _blank links in tweets are still not opened in the default browser after this patch landed. Screenshot of link from Tweetdeck in the Inspector: https://www.dropbox.com/s/ggu1matsq64aako/Screenshot%202014-03-05%2010.18.28.png

Also, ctrl+click used to open any link in default browser, but no longer does. I'll file a separate bug and block this one on it.
Depends on: 980010
Filed bug 980010 for re-enabling ctrl+click to open any link in default browser.
Should this get reopened and backed out until it doesn't regress existing behavior?

I use the following apps on desktop running via webrt and since this patch, clicking links in any of them will never open in default browser (with modifier or without):

zimbra
gmail
airmoz
soundcloud
tweetdeck
Flags: needinfo?(mar.castelluccio)
Do these links open in the browser on b2g?
Do you know which links in particular (in apps from the marketplace) have the "_blank" modifier? (it would ease my testing)
I've talked with Dietrich and we figured that the problem is that apps installed through his addon:
 - didn't have a manifestURL (so now he's simply added a fake manifestURL);
 - aren't in the registry (he could add the app by manually modifying the registry, or wait for bug 906223).

Apps installed through the DOM API work correctly.
Flags: needinfo?(mar.castelluccio)
Updated my add-on to open webapps.json and add new entries to it, and _blank links work correctly now, thanks for your help Marco, and great to meet you in person!

We still need the escape hatch of bug 980010, since the user should be able to decide where they want links opened. Take the following example:

When running Tweetdeck as an app, all t.co links (eg, all external links) get opened in new windows of that app, instead of the system default browser. It's very frustrating, because over in the default browser is where my bookmarks are, where my awesomebar is, where my save-to-pocket bookmarklet/addon-button is, etc.

Before the patch in this bug, I could click with a modifier to force open the link.
(In reply to Dietrich Ayala (:dietrich) from comment #50)
> Updated my add-on to open webapps.json and add new entries to it, and _blank
> links work correctly now,

Hey Dietrich, could you let the folks in bug 965775 know about your addon?  It sounds like it satisfies the feature request in that bug, so it'd be nice for the folks there to know about it!
(In reply to Myk Melez [:myk] [@mykmelez] from comment #51)
> Hey Dietrich, could you let the folks in bug 965775 know about your addon? 
> It sounds like it satisfies the feature request in that bug, so it'd be nice
> for the folks there to know about it!

Done, thanks Myk!
QA Whiteboard: [qa-]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.