1054215 - window.open and <a target="_blank" > from a browser window should open a new browser window

Status: RESOLVED FIXED

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

Description

[Ben Francis [:benfrancis]]

STR:

* Navigate to http://people.mozilla.org/~bfrancis/hyperlink/ in the system browser
* Click on "Hyperlink with target=_blank" or "window.open()" or "window.open() with _blank".

Expected:
* A new browser window is opened

Actual:
* The current browser window is navigated

Comment 1

[Kevin Grandon :kgrandon]

I have a fix already for this in review in bug 1055761. Taking.

Comment 2

[Kevin Grandon :kgrandon]

Attachment: Github pull request

This is a different issue than bug 1055761. Here is a test case, but working on a fix now.

Comment 3

[Kevin Grandon :kgrandon]

Comment on attachment 8479598 [details] [review]
Github pull request

Hey guys - could anyone give this a review? Thanks!

Comment 4

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

Comment on attachment 8479598 [details] [review]
Github pull request

This is a bug in browser.js
It is checking current displayed app and navigate it, which conflicts with this request. Please figure out what's your real requirement and fix it in browser.js side.

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/browser.js#L11

Comment 5

[Kevin Grandon :kgrandon]

Alive - can you explain why the user would want to open an activity here instead of just doing window.open()? How can we tell the difference between the activity and window.open() requests? I feel that we should deprecate activities now that we no longer have a browser app.

Comment 6

[Etienne Segonzac (:etienne)]

(In reply to Kevin Grandon :kgrandon from comment #5)
> Alive - can you explain why the user would want to open an activity here
> instead of just doing window.open()? How can we tell the difference between
> the activity and window.open() requests? I feel that we should deprecate
> activities now that we no longer have a browser app.

Some of those view activities come from gecko [1] and I think there's value in keeping a single code path here.

I'm really not sure about the patch from bug 941035, if I'm reading correctly we're launching a new activity with the specific intent of *not* opening a new window? Sounds extremely weird.

IMO the Search.navigate should use a |_port.postMessage| since this is the specific use case (changing the url of the current browser window), and we should keep the activity handling "vanilla" (and always open a new window).

[1] https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#617

Comment 7

[Kevin Grandon :kgrandon]

The patch in bug 941035 is obsoleted, sorry if that caused confusion.

I still don't think we should be using the activity path for this, but one quick hack might be to just have the activity open a new sheet?

Comment 8

[Kevin Grandon :kgrandon]

Attachment: Alternate approach, going the wrong direction

Comment 9

[Kevin Grandon :kgrandon]

Comment on attachment 8479598 [details] [review]
Github pull request

Vivien/Etienne - As suggested over e-mail, we should move away from MozActivity for web compat reasons. I'm marking you both as reviewers for either approach - we only need to land one of these, but both should solve the problem. Thanks!

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8479598 [details] [review]
Github pull request

As far as I can tell none of those patches resolved the |var w = window.open('foo.html', '_blank');| and window.opener issue.

In fact and as far as I know we will have the url opened twice. One in the opener process, and once in a new process.

Let's fix that the proper way. It will be nice if we can also differentiate between <a target='_blank" href="foo.html"> and window.open('foo.html', '_blank') since the first one does not require the window to be opened in the same process, while the other needs it (can be a followup).

Kevin, I need to go for tonight but if you have a new version of the patch I can update it tomorrow morning if I have some comments and land.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8480710 [details] [review]
Alternate approach, going the wrong direction

Same comment as the previous one.

Comment 12

[Kevin Grandon :kgrandon]

Comment on attachment 8479598 [details] [review]
Github pull request

Come on guys, we need to establish which direction to go in, and I feel like we can definitely land one of these first. The whole reason for having two patches here was to establish a direction to go in, and it seems like this might be the proper one?

Vivien - should we move forward with this approach?

Comment 13

[Kevin Grandon :kgrandon]

Also can we land this first and do the opener logic in a follow-up? We really need to close these core functionality pieces asap, as the FL deadline is tomorrow =/

Comment 14

[Kevin Grandon :kgrandon]

Attachment: Proposed fix by Alive

Comment 15

[Kevin Grandon :kgrandon]

Reviving the alternate approach for consideration. I'm fine with landing it then doing a follow-up for the opener thing.

Comment 16

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

Comment on attachment 8481020 [details] [review]
Proposed fix by Alive

AFAIK the bug is talking about the regression comes from bug 941035. let's fix vivien's concern in followup.

Comment 17

[Kevin Grandon :kgrandon]

Comment on attachment 8479598 [details] [review]
Github pull request

Thanks Alive. I'll land this and file a bug to track the window.opener and web compat issues.

Comment 18

[Kevin Grandon :kgrandon]

In master: https://github.com/mozilla-b2g/gaia/commit/c0fa9fe2f91ea76191d4e7c58db1dbf2993f76c8

Comment 19

[Kevin Grandon :kgrandon]

Some messed up HTML in that one.. Landed a follow-up: https://github.com/mozilla-b2g/gaia/commit/f087c45c328f16bf6f8d5a4d9452fa9a27aa6c7b