use openLinkIn in nsContextMenu ("Open Link in New Tab" shouldn't add tabs to popups)

VERIFIED FIXED in Firefox 4.0b9

Status

()

Firefox
Menus
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

({regression})

Trunk
Firefox 4.0b9
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 485502 [details] [diff] [review]
patch

Follow-up to bug 586234.

STR: Load attachment 464766 [details], open the popup, right click the link and choose "Open Link in New Tab".

The return values for openNewTabWith and openNewWindowWith have been added in bug 423833 for browser_bug423833.js. That test is disabled, but I enabled it to verify that it works with my modifications.
Attachment #485502 - Flags: review?(gavin.sharp)
Comment on attachment 485502 [details] [diff] [review]
patch

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>-  urlSecurityCheck(href, doc.nodePrincipal);
>-  openLinkIn(href, where, { fromContent: true,
>+  openLinkIn(href, where, { nodePrincipal: doc.nodePrincipal,

I don't really like the idea of moving the securityCheck call into openLinkIn and making it dependent on the nodePrincipal argument - it kind of obfuscates the fact that a security check is being done. Someone copying a openLinkIn call might not realize that the nodePrincipal argument is required for security, whereas copying a "urlSecurityCheck" call makes that more obvious. It's also somewhat confusing that the nodePrincipal argument affects the fromContent behavior. I would prefer keeping this pattern of calling urlSecurityCheck() explicitly before openLinkIn, where needed.

>diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js

>   openLinkInCurrent: function() {
>   openFrameInTab: function() {
>   openFrame: function() {

These probably all want to pass in charset too.

> function openNewTabWith(aURL, aDocument, aPostData, aEvent,

>-  // As in openNewWindowWith(), we want to pass the charset of the
>-  // current document over to a new tab. 
>-  var wintype = document.documentElement.getAttribute("windowtype");
>-  var originCharset;
>-  if (wintype == "navigator:browser")
>-    originCharset = window.content.document.characterSet;

This code was kind of bogus, but removing it will have an effect on callers. For example, newTabButtonObserver.onDrop in browser.js probably now wants to call openLinkIn and pass aEvent.dataTransfer.mozSourceNode.ownerDocument.characterSet (with a null check on mozSourceNode) as aCharset, and openFrameInTab wants to pass in doc.characterSet. Might be best to just leave this in if we plan to deprecate openNewTabWith anyways... Same comment applies to openNewWindowWith.

>-function openNewWindowWith(aURL, aDocument, aPostData, aAllowThirdPartyFixup,
>-                           aReferrer)

>-  var referrerURI = aDocument ? aDocument.documentURIObject : aReferrer;

You kept this prioritization for openNewTabWith, but didn't here - that seems wrong.

These methods have returned the tab/window since Firefox 3, so I'd rather not remove that. There's no real harm in extending that behavior to openLinkIn and passing it through here, is there?
Attachment #485502 - Flags: review?(gavin.sharp) → review-
blocking2.0: --- → ?
(Assignee)

Comment 2

7 years ago
(In reply to comment #1)
> These methods have returned the tab/window since Firefox 3, so I'd rather not
> remove that. There's no real harm in extending that behavior to openLinkIn and
> passing it through here, is there?

It would sometimes return a tab and sometimes a window, depending on whether a new window had to be opened. This seems to be asking for trouble.
I don't see that as particularly problematic...
(Assignee)

Comment 4

7 years ago
Created attachment 485823 [details] [diff] [review]
patch v2
Attachment #485502 - Attachment is obsolete: true
Attachment #485823 - Flags: review?(gavin.sharp)
(Assignee)

Comment 5

7 years ago
Created attachment 485825 [details] [diff] [review]
patch v2

forgot to remove fromContent in browser.js
Attachment #485823 - Attachment is obsolete: true
Attachment #485825 - Flags: review?(gavin.sharp)
Attachment #485823 - Flags: review?(gavin.sharp)
(Assignee)

Comment 6

7 years ago
(In reply to comment #3)
> I don't see that as particularly problematic...

The problem is that the caller may not realize it and assume the return value to always be a tab when where=="tab".

Nobody seems the have asked for the return values. They were added for a now-disabled test that doesn't really need them.
blocking2.0: ? → betaN+

Comment 7

7 years ago
There seems to be a coding error in openLinkIn. Hope it being fixed with this.

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#200

200   var w = getTopWin();
201   if ((where == "tab" || where == "tabshifted") &&
202       w.document.documentElement.getAttribute("chromehidden")) {
203     w = getTopWin(true);
204     aRelatedToCurrent = false;
205   }
206 
207   if (!w || where == "window") {

w is used before !w is checked.
Keywords: regression
Attachment #485825 - Flags: review?(gavin.sharp) → review+
I included a fix for that in my patch for bug 610203.
(Assignee)

Comment 9

7 years ago
http://hg.mozilla.org/mozilla-central/rev/c84a2abbc663
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b9
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b9pre) Gecko/20101227 Firefox/4.0b9pre ID:20101227030354
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Flags: in-litmus-
Duplicate of this bug: 621968
Depends on: 626148
Depends on: 626365

Comment 12

7 years ago
> Nobody seems the have asked for the return values. They were added for a now-disabled test that doesn't really need them.

I know I'm a bit late to the party, but a bug in Greasemonkey has been caused by this change.  Please advise where I should go if commenting on this bug is the wrong thing to be be doing.

In short, we provide a method GM_openInTab() which opens a tab, and we want to return a handle to the content window thus created (like what window.open would do in content).  Previously, for this, we relied on the return value of openNewTabWith, which I've now discovered has disappeared.

Any suggestions on how best to retain this feature?
(Assignee)

Comment 13

7 years ago
You can do what openNewTabWith did and call gBrowser.loadOneTab or gBrowser.addTab. However, then you have a problem with popups (see bug 641157 for some context). You can try to find a non-popup browser window, add the tab there, and pass through the returned tab. If no such window is around, you need to open a new window, but then GM_openInTab can't return the tab.

Comment 14

7 years ago
Comment #1 in Bug 626365 states that current behavior is by design and points to this bug for explanation. Unfortunately, I cannot follow the reasoning (not a programmer, work in QA/UX design).
Once the fix is in a release, will this reinstate FF3 behavior in regards to opening a new tab within a popup/secondary window? Only asking as this ends up to be a keep/toss decision in regards to FF4.
You need to log in before you can comment on or make changes to this bug.