Closed Bug 244279 Opened 20 years ago Closed 20 years ago
Enabling popup blocking from About Popups fails to set the preference
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.
accepting, hoping for trunk and 1.7 final.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.7final
20 years ago
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.
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
Comment on attachment 149039 [details] [diff] [review] patch obsoleted by Neil's patch
> 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: 22.214.171.124; 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
You need to log in before you can comment on or make changes to this bug.