Closed Bug 1054215 Opened 6 years ago Closed 6 years ago
.open and <a target="_blank" > from a browser window should open a new browser window
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
I have a fix already for this in review in bug 1055761. Taking.
Assignee: nobody → kgrandon
Status: NEW → ASSIGNED
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!
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
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.
(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  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).  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?
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 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-
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?
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 =/
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+
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.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Some messed up HTML in that one.. Landed a follow-up: https://github.com/mozilla-b2g/gaia/commit/f087c45c328f16bf6f8d5a4d9452fa9a27aa6c7b
Target Milestone: --- → 2.1 S3 (29aug)
You need to log in before you can comment on or make changes to this bug.