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

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, defect)

Description

[(not reading, please use [e-mail] instead)]

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.

Comment 1

[(not reading, please use [e-mail] instead)]

Attachment: patch

Comment 2

[(not reading, please use [e-mail] instead)]

accepting, hoping for trunk and 1.7 final.

Comment 3

[Stephen Donner [:stephend] Not actively reading bugmail]

uber nit:

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

'every'

Comment 4

[Daniel Veditz [:dveditz]]

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

r=dveditz

Comment 5

[Mike Connor [:mconnor]]

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

Comment 6

[[e-mail]]

(In reply to comment #5)
>(From update of attachment 149039 [details] [diff] [review])
>the rest looks good though
Cough! Splutter!

Comment 7

[(not reading, please use [e-mail] instead)]

> 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.

Comment 8

[(not reading, please use [e-mail] instead)]

mike, I misunderstood your point.

we could skip do something like this:

if (!parent.window.opener.location)
  setTimeout("setupPopupBlockingPrefUI()", 0);
else
  setButtons();

Comment 9

[[e-mail]]

Attachment: Use the prefwindow api

Saves fiddling around with around with timeouts and temporary preferences.

Comment 10

[Mike Connor [: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)

Comment 11

[(not reading, please use [e-mail] instead)]

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 12

[Daniel Veditz [:dveditz]]

Comment on attachment 149069 [details] [diff] [review]
Use the prefwindow api

sr=dveditz works great
a=dveditz

Comment 13

[Daniel Veditz [:dveditz]]

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

obsoleted by Neil's patch

Comment 14

[(not reading, please use [e-mail] instead)]

> obsoleted by Neil's patch

it was my patch.  neil's was the good one.  
I'll test it too, just to make sure.

Comment 15

[Mike Connor [:mconnor]]

if that's a=dveditz shouldn't that be approval1.7+ instead?

Comment 16

[(not reading, please use [e-mail] 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 17

[(not reading, please use [e-mail] instead)]

Comment on attachment 149069 [details] [diff] [review]
Use the prefwindow api

a=dveditz,sspitzer (...and indirectly asa, from a comment in another bug)

Comment 18

[(not reading, please use [e-mail] instead)]

fixed on both trunk and branch.

thanks again to neil, mconnor and dan.

Comment 19

[Tracy Walker [:tracy]]

verified