Closed Bug 1054215 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S3 (29aug)

People

(Reporter: benfrancis, Assigned: kgrandon)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files, 1 obsolete file)

46 bytes, text/x-github-pull-request
Details | Review
46 bytes, text/x-github-pull-request
alive
: review+
Details | Review
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
Blocks: 941270
No longer blocks: browser-chrome-mvp
I have a fix already for this in review in bug 1055761. Taking.
Assignee: nobody → kgrandon
Status: NEW → ASSIGNED
Attached file Github pull request
This is a different issue than bug 1055761. Here is a test case, but working on a fix now.
Comment on attachment 8479598 [details] [review]
Github pull request

Hey guys - could anyone give this a review? Thanks!
Attachment #8479598 - Flags: review?(etienne)
Attachment #8479598 - Flags: review?(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
Attachment #8479598 - Flags: review?(etienne)
Attachment #8479598 - Flags: review?(alive)
Attachment #8479598 - Flags: review-
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.
Flags: needinfo?(alive)
Blocks: 1055713
(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
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?
No longer blocks: 1055713
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!
Attachment #8479598 - Flags: review?(etienne)
Attachment #8479598 - Flags: review?(21)
Attachment #8480710 - Flags: review?(etienne)
Attachment #8480710 - Flags: review?(21)
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.
Attachment #8479598 - Flags: review?(21) → review-
Comment on attachment 8480710 [details] [review]
Alternate approach, going the wrong direction

Same comment as the previous one.
Attachment #8480710 - Flags: review?(21) → review-
Attachment #8479598 - Flags: review?(etienne)
Attachment #8480710 - Attachment is obsolete: true
Attachment #8480710 - Flags: review?(etienne)
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?
Attachment #8479598 - Flags: review-
Attachment #8479598 - Flags: feedback?(alive)
Attachment #8479598 - Flags: feedback?(21)
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 =/
Attached file Proposed fix by Alive
Attachment #8481020 - Flags: review?(alive)
Reviving the alternate approach for consideration. I'm fine with landing it then doing a follow-up for the opener thing.
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.
Attachment #8481020 - Flags: review?(alive) → review+
Flags: needinfo?(alive)
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.
Attachment #8479598 - Flags: feedback?(alive)
Attachment #8479598 - Flags: feedback?(21)
In master: https://github.com/mozilla-b2g/gaia/commit/c0fa9fe2f91ea76191d4e7c58db1dbf2993f76c8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
See Also: → 1060191
Some messed up HTML in that one.. Landed a follow-up: https://github.com/mozilla-b2g/gaia/commit/f087c45c328f16bf6f8d5a4d9452fa9a27aa6c7b
Whiteboard: [systemsfe]
Target Milestone: --- → 2.1 S3 (29aug)
Depends on: 1095653
You need to log in before you can comment on or make changes to this bug.