Camino fails to honor the pref for redirecting window.open links

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Tabbed Browsing
RESOLVED FIXED
11 years ago
7 years ago

People

(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: froodian (Ian Leue))

Tracking

({fixed1.8.1, testcase})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1, testcase

Details

Attachments

(2 attachments, 1 obsolete attachment)

Prior to 11 Aug, we redirected all window.open calls.  After 11 Aug, we don't redirect *any* window.open calls *even ones that don't specify any chrome restrictions or size*.

We *still* fail to follow the pref that exists to allow users to determine what they want to happen (aka the happy medium).

We should obey the pref and set it to 2 by default in all-camino.js

user_pref("browser.link.open_newwindow.restriction", 2);

0 = divert all window.open calls according to user's SWM prefs
1 = don't divert any window.open calls
2 = divert window.open according to user's SWM pref unless the window.open specifies size or features
define redirect. 

Are you saying that we now open new windows all the time regardless of the SWM setting? I tested that and it works fine.
i believe this is the relevant code to copy in Ff:

http://lxr.mozilla.org/seamonkey/source/xpfe/appshell/src/nsContentTreeOwner.cpp#799

I wish i knew what it was supposed to do or i'd just put it in.
Status: NEW → ASSIGNED
s/redirect/divert/ = send window.open calls to a new tab (or same tab) when SWM is enabled

Gah, I had a bad testcase for plain window.open; vanilla ones *are* diverted.  Sorry.

So this is just about honoring the pref (and setting it to 2 by default when people enable SWM).
Summary: Camino fails to honor the pref for redirecting window.open links, and doesn't redirect any window.open calls → Camino fails to honor the pref for redirecting window.open links
Created attachment 233428 [details]
working SWM testcase
(Assignee)

Updated

11 years ago
Keywords: testcase
(Assignee)

Comment 5

11 years ago
Created attachment 236104 [details] [diff] [review]
Partial Fix

This is a partial fix.  It makes us respect values of 0 and 2 for the pref.  I'm not sure how we'd go about fixing this entirely, since we'll actually have to detect window.open calls (instead of just looking for changed chrome or specified size/position like we do now).  I'll let the community/hwaara decide if this is acceptable (possibly with a followup bug).
Attachment #236104 - Flags: review?(hwaara)

Comment 6

11 years ago
Comment on attachment 236104 [details] [diff] [review]
Partial Fix

I don't really know how any our swm code works; I think it's better if pink reviews the change directly.
Attachment #236104 - Flags: review?(hwaara) → review?(mikepinkerton)
I'm not really sure we should do this. Setting the pref to anything other than 2 will most likely break web apps that assume this behavior. Sure, caveat emptor, but should we really give users enough rope to hang themselves if we can avoid it?
oh, also, I don't see |respectWindowOpenCallsWithSizeAndPosition| declared in any header. This should generate a warning, no?
Some users really want a true *single* window mode, where normal-clicking on any link works as $DEITY intended--opening that link in the same window.  Users who set the pref to 0 are also more likely to know/pick out links that will cause problems when the pref is set to 0 and know how to "fix"/prevent it.  

E.g., during the time from the point SWM landed until bug 338777 landed, 0 was our default behavior with SWM on, and I knew which sorts of links I could click on and which sorts I needed to disable SWM before clicking in order for things not to be broken.  It's fairly easy *for the sorts of people who would set this pref to 0* to figure out the sorts of links that are problematic, but supporting this pref gives them the flexibility to obtain the behavior they desire.  (It's a shame we didn't get all these pref behaviors for free...such is the life of embedding, I guess....)
(Assignee)

Comment 10

11 years ago
Created attachment 236516 [details] [diff] [review]
Patch

I agree with smokey - there is a group of users who'll want this.  This patch addresses the issue with |respectWindowOpenCallsWithSizeAndPosition| being improperly declared.
Assignee: nobody → stridey
Attachment #236104 - Attachment is obsolete: true
Attachment #236516 - Flags: review?(mikepinkerton)
Attachment #236104 - Flags: review?(mikepinkerton)
Since I'm one of the users that would use this regularly, I agree with Smokey and froodian as well. We should get this in.
Comment on attachment 236516 [details] [diff] [review]
Patch

sr=pink
Attachment #236516 - Flags: review?(mikepinkerton) → superreview+
(Assignee)

Comment 13

11 years ago
Checked in on trunk and 1.8branch
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.