Closed Bug 244279 Opened 20 years ago Closed 20 years ago

Enabling popup blocking from About Popups fails to set the preference.

Categories

(SeaMonkey :: Preferences, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.7final

People

(Reporter: sspitzer, Assigned: sspitzer)

Details

(Keywords: verified1.7)

Attachments

(1 file, 1 obsolete file)

Enabling popup blocking from About Popups fails to set the preference.

thanks to trix for finding this.

If you attempt to block popups from the About Popups dialog, it displays an
UNSELECTED "Block unrequested popup windows" pref panel checkbox.

STEPS:
1. Make sure popups are not blocked by going to Preferences|Privacy &
Security|Popup Windows and deselect(if selected) "Block unrequested popup
windows" checkbox and click OK.
2. Now go to Tools|Popup Manager|About Popup Blocking.
3. Click Yes to attempt to enable popup blocking.

RESULTS:
The "Block unrequested popup windows" pref panel checkbox is UNSELECTED and pref
is not enabled.

NOTE:
If you click OK from the Popup Windows dialog at this point and return back,
you'll notice the preference was never enabled.  But in the Popup Windows pref
panel, if you click on another category(ie...Forms) and click back Popup
Windows, the pref is NOW set and enabled.  But because the average user will not
know to do this, this becomes a loss of browser functionality.  

also note, the way the current code is written, if you got to the pref ui this
way, and then unchecked the checkbox and switched back and forth between panels,
it would re-check the checkbox.

it is because we call init() each time the panel is shown, and this code gets
executed:

        if (!parent.window.opener.location) // opened from About Popups menuitem
          gPolicyCheckbox.checked = true;

I've got a fix, I'll attach.
Attached patch patch (obsolete) — Splinter Review
accepting, hoping for trunk and 1.7 final.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.7final
Attachment #149039 - Flags: superreview?(darin)
Attachment #149039 - Flags: review?(dveditz)
uber nit:

+          // (and not ever time the popup pref panel loads)

'every'
Comment on attachment 149039 [details] [diff] [review]
patch

r=dveditz
Attachment #149039 - Flags: review?(dveditz) → review+
Comment on attachment 149039 [details] [diff] [review]
patch

>-        if (!parent.window.opener.location) // opened from About Popups menuitem
>+        // if this pref panel was opened from About Popups menuitem
>+        // we want to act like the user checked the "enable popups" checkbox
>+        // but we only want to do that once, as the user could have unchecked
>+        // and switched between panels 
>+	setTimeout("setupPopupBlockingPrefUI()", 0);
>+      }

is there a reason to remove the opener check here?  if its not opened from the
About Popups menuitem (which isn't even available by default) we don't need to
be taking this codepath at all, so we're going through unnecessary hoops on
every panel load, instead of on the single case where it matters.

the rest looks good though
(In reply to comment #5)
>(From update of attachment 149039 [details] [diff] [review])
>the rest looks good though
Cough! Splutter!
> is there a reason to remove the opener check here? 

if we check the opener every time, it will be true even if we switch panels.
see the "also note" part of the initial bug description.
mike, I misunderstood your point.

we could skip do something like this:

if (!parent.window.opener.location)
  setTimeout("setupPopupBlockingPrefUI()", 0);
else
  setButtons();
Saves fiddling around with around with timeouts and temporary preferences.
Attachment #149069 - Flags: superreview?(sspitzer)
Attachment #149069 - Flags: review?(mconnor)
Comment on attachment 149069 [details] [diff] [review]
Use the prefwindow api

oh, that's way better, I didn't know about queuedTag (I should really read the
prefwindow API docs, assuming they exist)
Attachment #149069 - Flags: review?(mconnor) → review+
neil, thanks for the better approach.  

I'll test this out tonight, and then do the sr/a for 1.7 if all goes well.

here's a related issue that I'll spin off, but I figured I'm mention it while you were still thinking about 
pref bugs.

the pref window is not modal, so if you happen to have the pref window open when you do "about 
popups", I think the existing code will just focus the pref window, instead of switching panels.

see goPreferences() in http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/﷒0﷓

neil, do you think we should switch panels after focusing?
Comment on attachment 149069 [details] [diff] [review]
Use the prefwindow api

sr=dveditz works great
a=dveditz
Attachment #149069 - Flags: superreview?(sspitzer)
Attachment #149069 - Flags: superreview+
Attachment #149069 - Flags: approval1.7?
Comment on attachment 149039 [details] [diff] [review]
patch

obsoleted by Neil's patch
Attachment #149039 - Attachment is obsolete: true
Attachment #149039 - Flags: superreview?(darin)
> obsoleted by Neil's patch

it was my patch.  neil's was the good one.  
I'll test it too, just to make sure.
if that's a=dveditz shouldn't that be approval1.7+ instead?
tested it, and it works great.  thanks again, neil.

I've landed the fix on the 1.7 branch.

Checking in resources/content/pref-popups.xul;
/cvsroot/mozilla/extensions/cookie/resources/content/pref-popups.xul,v  <--  pre
f-popups.xul
new revision: 1.8.16.1; previous revision: 1.8
done

now I'll go test and land on the trunk.
Comment on attachment 149069 [details] [diff] [review]
Use the prefwindow api

a=dveditz,sspitzer (...and indirectly asa, from a comment in another bug)
Attachment #149069 - Flags: approval1.7? → approval1.7+
fixed on both trunk and branch.

thanks again to neil, mconnor and dan.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Keywords: fixed1.7
verified
Status: RESOLVED → VERIFIED
Keywords: fixed1.7verified1.7
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: