(not reading, please use seth@sspitzer.org instead) Assignee
	Description • 20 years ago

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.

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 2 • 20 years ago

accepting, hoping for trunk and 1.7 final.

	Stephen Donner [:stephend] Not actively reading bugmail
	Comment 3 • 20 years ago

uber nit:

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

'every'

	Daniel Veditz [:dveditz]
	Comment 4 • 20 years ago

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

r=dveditz

	Mike Connor [:mconnor]
	Comment 5 • 20 years ago

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

	neil@parkwaycc.co.uk
	Comment 6 • 20 years ago

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

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 7 • 20 years ago

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

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 8 • 20 years ago

mike, I misunderstood your point.

we could skip do something like this:

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

	neil@parkwaycc.co.uk
	Comment 9 • 20 years ago

Saves fiddling around with around with timeouts and temporary preferences.

	Mike Connor [:mconnor]
	Comment 10 • 20 years ago

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)

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 11 • 20 years ago

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?

	Daniel Veditz [:dveditz]
	Comment 12 • 20 years ago

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

sr=dveditz works great
a=dveditz

	Daniel Veditz [:dveditz]
	Comment 13 • 20 years ago

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

obsoleted by Neil's patch

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 14 • 20 years ago

> obsoleted by Neil's patch

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

	Mike Connor [:mconnor]
	Comment 15 • 20 years ago

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

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 16 • 20 years ago

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.

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 17 • 20 years ago

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

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

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 18 • 20 years ago

fixed on both trunk and branch.

thanks again to neil, mconnor and dan.

	Tracy Walker [:tracy]
	Comment 19 • 20 years ago

verified