Closed Bug 130337 Opened 22 years ago Closed 22 years ago

browser.block.target_new_window messes up Get New Themes link

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.2alpha

People

(Reporter: jonasj, Assigned: akkzilla)

References

Details

(Whiteboard: [Hixie-P0])

Attachments

(1 file)

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
I'd like to revisit this code anyway.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.1alpha
Blocks: 143424
Whiteboard: [Hixie-P0]
*** Bug 149503 has been marked as a duplicate of this bug. ***
Blah! missed 1.1. This is /really/ screwy.
Target Milestone: mozilla1.1alpha → mozilla1.2alpha
Re-adding bz and adam as possible reviewers, and for their opinions on which
schemes we should be checking.
Attached patch A fixSplinter Review
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 on attachment 95651 [details] [diff] [review]
A fix

r=jfrancis
Attachment #95651 - Flags: review+
Kin, could I get an sr for this small fix?  (About 5 lines.)
Comment on attachment 95651 [details] [diff] [review]
A fix

sr=kin@netscape.com
Attachment #95651 - Flags: superreview+
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
Component: Layout: HTML Frames → Layout: Images
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: