browser.block.target_new_window messes up Get New Themes link

RESOLVED FIXED in mozilla1.2alpha

Status

()

Core
Layout: HTML Frames
RESOLVED FIXED
17 years ago
16 years ago

People

(Reporter: Jonas Jørgensen, Assigned: Akkana Peck)

Tracking

Trunk
mozilla1.2alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Hixie-P0])

Attachments

(1 attachment)

(Reporter)

Description

17 years ago
Trunk build 2002-03-11-03, Win2000:

If the browser.block.target_new_window pref is turned on, the Get New Themes
link in Preferences|Appearance|Themes will open in the Preferences window
instead of in a new window.

To reproduce:
* Open Preferences and go to Advanced|Scripts & Windows.
* Disable "Open a link in a new window".
* Click OK.
* Open Preferences again, this time go to Appearance|Themes.
* Click the "Get New Themes" link.

Actual results:
* A page from XULPlanet.com is loaded inside the Preferences dialog.

Expected results:
* The page is loaded in a new window.

I guess the way to fix this will be to always allow target="anything" for
chrome:// URLs.

Boris: I hope you don't mind me assigning this to you -- you did such a nice,
quick job in bug 126003, so it seems like a very natural thing to do. :) But
feel free to give this away, of course.
Punting to Akkana. I have pretty much no time to work on Mozilla stuff till
after 1.0....
Assignee: bzbarsky → akkana
(Assignee)

Comment 2

17 years ago
I'd like to revisit this code anyway.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.1alpha
Whiteboard: [Hixie-P0]

Comment 3

16 years ago
*** Bug 149503 has been marked as a duplicate of this bug. ***

Comment 4

16 years ago
Blah! missed 1.1. This is /really/ screwy.
Target Milestone: mozilla1.1alpha → mozilla1.2alpha
(Assignee)

Comment 5

16 years ago
Re-adding bz and adam as possible reviewers, and for their opinions on which
schemes we should be checking.
(Assignee)

Comment 6

16 years ago
Created attachment 95651 [details] [diff] [review]
A fix

Here's a fix.  In addition to checking whether the new url will be chrome or
resource, we also check whether the URI of the current docshell is chrome. 
Should we also check for resource: ?  Can a docshell's URI be resource:, and
for what sort of window would that occur?

In the chrome case, we just ignore mDisallowPopupWindows and go ahead and pop
up a new window.  Personally I'd rather have it open the url in the
last-accessed browser window, but I'm not sure how to get that (get it somehow
from the nsIDocShellTreeItem weak ref mParent?) or whether the prefs dialog
modality might cause that window to be unusable on nonunix platforms.  So I
took the easy way out.

The normal case (click on a target=new link in a browser) still works -- the
docshell's uri is the uri of the page, not of the browser chrome (whew).

The patch also includes an initializer-ordering warning fix.  After this,
nsDocShell.cpp still generates one warning, but it looks like that might be gcc
being overly picky as usual:
nsDocShell.cpp: In method `nsresult nsDocShell::InternalLoad (nsIURI *, 
nsIURI *, nsISupports *, int, const PRUnichar *, nsIInputStream *, 
nsIInputStream *, unsigned int, nsISHEntry *, int, nsIDocShell **, 
nsIRequest **)':
nsDocShell.cpp:4671: warning: enumeral mismatch in conditional 
expression: `nsIContentPolicy::{anonymous enum}' vs 
`nsIContentPolicy::{anonymous enum}'

Comment 7

16 years ago
Comment on attachment 95651 [details] [diff] [review]
A fix

r=jfrancis
Attachment #95651 - Flags: review+
(Assignee)

Comment 8

16 years ago
Kin, could I get an sr for this small fix?  (About 5 lines.)

Comment 9

16 years ago
Comment on attachment 95651 [details] [diff] [review]
A fix

sr=kin@netscape.com
Attachment #95651 - Flags: superreview+
(Assignee)

Comment 10

16 years ago
Fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.