Rework the popup permissions menu so that it behaves like the cookie and image permissions menu.

RESOLVED FIXED in seamonkey2.47

Status

SeaMonkey
Passwords & Permissions
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: lvm, Assigned: frg)

Tracking

Trunk
seamonkey2.47

SeaMonkey Tracking Flags

(seamonkey2.43 wontfix, seamonkey2.44 wontfix, seamonkey2.45 wontfix, seamonkey2.46 wontfix, seamonkey2.47 fixed)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:29.0) Gecko/20100101 Firefox/29.0 SeaMonkey/2.26.1 (Beta/Release)
Build ID: 20140612174402

Steps to reproduce:

prerequisites: popus are blocked (preferences-privacy and security-popup windows-block unrequested popup windows)

open any site
go to 'tools-popup manager-allow popups from this site' menu



Actual results:

nothing - exception allowing popus from this site is not created


Expected results:

a permission record allowing popus for the currently opened site should've been created

Actually the whole concept of popup manager has been destroyed with the introduction of the data manager: popup exception is just one of the many types of permission records - they cannot be viewed separately, so the 'tools-popup manager-manage popus' menu and 'preferences-privacy and 'preferences-privacy and security-popup windows-block unrequested popup windows-manage permissions' button both just open the data manager, you cannot filter just popups there. Data manager is evil and should be broken into parts.

Updated

3 years ago
Component: General → MailNews: Backend

Updated

3 years ago
Component: MailNews: Backend → Passwords & Permissions

Comment 1

2 years ago
My suspect: Popup Manager does not work at all (as stated in Description).

First Test: For <http://www.haustechnikdialog.de/> I added a popup exception via menu 'tools → PopUp Manager → Allow from this site. I opened data Manager: only known entries for password and cookies shown, no additional one for popup exception

Second Test:
With a newly created Profile with DE  SeaMonkey 2.0  (Windows NT 6.1; WOW64; rv:1.9.1.4)  Gecko/20091017  Build 20091017081335  (Classic Theme) on German WIN7 64bit, for <http://www.haustechnikdialog.de/> I added a popup exception via menu 'tools → PopUp Manager → Allow from this site. I opened data Manager: "haustechnikdialog.de" for exception shown. I opened data Manager: nothing shown for "haustechnikdialog.de"
I close SM 2.0 and open SeaMonkey German 2.39 final Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0 from official download area)  Gecko/20100101  Firefox/42.0  Build 20151103191810  (Classic Theme) on German WIN7 64bit for this profile.
(Assignee)

Comment 2

2 years ago
While this is a fairly old bug I would close it now as a duplicate of Bug 1188348. The popup exceptions work with the fixed Data Manager there. 

When you add a permission you need to first add it, then uncheck the default and change the value to something else. Otherwise it will not be saved. This was always the case with Data Manager and not exactly straightforward nor user friendly.

I now see that I need to change the patch again so that the scheme is added to the entry box when selecting "allow popups from this website". Currently only the host is added and you need to do this yourself.
(Assignee)

Comment 3

2 years ago
Created attachment 8714125 [details]
Reworked Popup Menu

I think the current Popup Manager menu s*cks. Unlike the cookie and image menu it takes you to Data Manager which I find counterproductive and unintuitive. I decided to make it just like the cookie and image menu. WIP patch is not ready yet. Need to enable and disable the new items based on the global popup settings pref.

If someone thinks this is a bad idea please tell me now.
Assignee: nobody → frgrahl
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 4

2 years ago
(In reply to Frank-Rainer Grahl from comment #3)
> If someone thinks this is a bad idea please tell me now.

I think that idea is plausible.
(Assignee)

Comment 5

2 years ago
Created attachment 8714960 [details] [diff] [review]
Reworked popup menu in Navigator patch

Patch to change the menu so that it behaves like the cookie and image permissions menu. The selective enabling of popups which were populated in navigator.js by popupBlockerMenuShowing can still be done in the notification box and didn't work for me anyway in the standard menu prior to the patch.

Tested with VS2015 Windows x64 build:

User agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:46.0) Gecko/20100101 Firefox/46.0 SeaMonkey/2.43a2
Attachment #8714960 - Flags: review?(neil)
(Assignee)

Updated

2 years ago
Blocks: 1240738

Comment 6

a year ago
Comment on attachment 8714960 [details] [diff] [review]
Reworked popup menu in Navigator patch

I only skimmed the patch, but it looks like you changed the behavior on Mac:

-  if (!/Mac/.test(navigator.platform))
-    popupBlockerMenuShowing(aEvent);

The 'Show popup' menuitems should not be generated on Mac. See bug 359597 for more info. The workaround for bug 359597 (don't generate the menuitems) was http://hg.mozilla.org/comm-central/rev/e31023bb0084 and then Ian moved the check to navigator.js in http://hg.mozilla.org/comm-central/rev/7c0c838f516d
Flags: needinfo?(frgrahl)
(Assignee)

Comment 7

a year ago
Stefan,

the popups use the same logic as cookies and images with the patch and the test was imho no longer valid or needed. If it works for images and cookies on mac it should work here too.

If you are able to compile suite it would be great if you could check it out.

FRG
Flags: needinfo?(frgrahl)

Comment 8

a year ago
> the popups use the same logic as cookies and images with the patch and the
> test was imho no longer valid or needed. If it works for images and cookies
> on mac it should work here too.

I'm sorry, but it won't. Not until bug 331635 is fixed (which bug 359597 depends on). It's even less chance of it getting fixed nowadays because embedding Gecko is history. To make a long story short: The Mac menu is native and that means that you can't unblock popups from it (Gecko thinks unblocking is done by an untrusted source).

Comment 9

a year ago
Aha, now I see what you do. You actually remove the feature of showing individual (blocked) popups from the Tools menu. That can then only be done by clicking the Preferences button in the notification bar. I think that should work on Mac.

Updated

a year ago
Summary: popup exceptions are not added → Rework the popup permissions menu so that it behaves like the cookie and image permissions menu.

Updated

a year ago
Duplicate of this bug: 1247383
(Assignee)

Comment 11

a year ago
Comment on attachment 8714960 [details] [diff] [review]
Reworked popup menu in Navigator patch

I think Neil is currently not available. Ian,
can you review this?
Attachment #8714960 - Flags: review?(neil) → review?(iann_bugzilla)

Comment 12

a year ago
With the patch, are the individual popups still available from the item in the Status Bar? (Using Windows)

Comment 13

a year ago
(When right clicking on it.)
(Assignee)

Comment 14

a year ago
Created attachment 8742106 [details]
Clipboard.jpg

No but they are still easily accessible from the notification bar.

Updated

a year ago
OS: Windows 7 → All
Hardware: x86 → All
Version: SeaMonkey 2.26 Branch → Trunk

Updated

a year ago
Status: NEW → ASSIGNED

Comment 15

a year ago
Comment on attachment 8714960 [details] [diff] [review]
Reworked popup menu in Navigator patch

>-    <!-- Items are generated, see popupBlockerMenuShowing() -->
>+    <menupopup id="popupBlockerMenu"/>
What now populates this menupopup, if anything?
The only thing that I can see that uses it is the popupIcon in the statusbarpanel. I think that should be looked at as part of this bug.

>+++ b/suite/browser/navigatorOverlay.xul

>-            <menuitem id="AllowPopups"
>+           <menuitem id="popup_deny"
Nit: indentation.

f=me for the moment.
Attachment #8714960 - Flags: review?(iann_bugzilla) → feedback+
(Assignee)

Comment 16

a year ago
Created attachment 8742898 [details] [diff] [review]
1055954-popupmanage-V2.patch

Version 2 patch.

I think this one is much better. Added scheme when going to Data Manager and populated the status bar popup menu again instead of removing it. I didn' take into account that you can hide the notification bar and if so you wouldn't be able to manage the individual popups.
Attachment #8714960 - Attachment is obsolete: true
Attachment #8742898 - Flags: review?(iann_bugzilla)

Comment 17

a year ago
Comment on attachment 8742898 [details] [diff] [review]
1055954-popupmanage-V2.patch

>+++ b/suite/browser/navigator.js

>+function hostUrl()
> {
>+  var url = "";
>   try {
>+    url = getBrowser().currentURI.scheme + "://" + getBrowser().currentURI.hostPort;
>   } catch (e) {}
>+  return url;
> }

>+++ b/suite/common/utilityOverlay.js
>+function hostUrl()
>+{
>+  var url = "";
>+  try {
>+    url = getBrowser().currentURI.scheme + "://" + getBrowser().currentURI.hostPort;
>+  } catch (e) {}
>+  return url;
>+}
>+

Is this needed in both files? Would it still work in just utilityOverlay.js?
Flags: needinfo?(frgrahl)
(Assignee)

Comment 18

a year ago
Created attachment 8752480 [details] [diff] [review]
1055954-popupmanage-V3.patch

Thanks for pointing it out. Seems to pickup the function in utilityOverlay fine so here is patch V3.
Attachment #8742898 - Attachment is obsolete: true
Attachment #8742898 - Flags: review?(iann_bugzilla)
Flags: needinfo?(frgrahl)
Attachment #8752480 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

a year ago
Attachment #8752480 - Flags: review?(philip.chee)

Comment 19

a year ago
Comment on attachment 8752480 [details] [diff] [review]
1055954-popupmanage-V3.patch

> +  var allowBlocking = Services.prefs.getBoolPref("dom.disable_open_during_load");
> +
> +  if (allowBlocking) {
> +    document.getElementById("popup_deny").removeAttribute("disabled");
> +    document.getElementById("popup_default").removeAttribute("disabled");
> +    document.getElementById("popup_allow").removeAttribute("disabled");
> +  }
> +  else {
> +    document.getElementById("popup_deny").setAttribute("disabled", "true");
> +    document.getElementById("popup_default").setAttribute("disabled", "true");
> +    document.getElementById("popup_allow").setAttribute("disabled", "true");

Looking at the surrounding code for hints I think you can do:

var items = aEvent.target.getElementsByAttribute("name", "popup");
for (let item of items) {
  if (allowBlocking)
    item.removeAttribute("disabled");
  else
    item.setAttribute("disabled", "true");

>          <menu id="menu_cookieManager"
>                label="&cookieCookieManager.label;"
>                accesskey="&cookieCookieManager.accesskey;"
> -              oncommand="if (event.target.id) CookieImageAction(event.target);
> -                         else toDataManager('|cookies');">
> +              oncommand="if (event.target.id) CookieImagePopupAction(event.target);
Perhaps if (event.target.id.startsWith("cookie_");

> +                         else toDataManager(hostUrl() + '|cookies');">
....
>             <menuitem label="&cookieDisplayCookiesCmd.label;"
>                       accesskey="&cookieDisplayCookiesCmd.accesskey;"/>
Move the toDataManager(...) call to here.
Add an event.stopPropagation() e.g.
oncommand="toDataManager(hostUrl() + '|cookies'); event.stopPropagation();"
This menuitem should maybe have an id="menuitem_cookieDisplay"

>          <menu id="menu_imageManager"
>                label="&cookieImageManager.label;"
>                accesskey="&cookieImageManager.accesskey;"
>                oncommand="if (event.target.id == 'menuitem_cookieDisplay')
> -                           toDataManager('|permissions');
> +                           toDataManager(hostUrl() + '|permissions|add|image');
>                           else
> -                           CookieImageAction(event.target);">
Perhaps if (event.target.id.startsWith("image_");
> +                           CookieImagePopupAction(event.target);">
....
>             <menuitem id="menuitem_cookieDisplay"
>                       label="&cookieDisplayImagesCmd.label;"
>                       accesskey="&cookieDisplayImagesCmd.accesskey;"/>

Move the toDataManager(...) call to id="menuitem_cookieDisplay"
Add an event.stopPropagation() e.g.
oncommand="toDataManager(hostUrl() + '|permissions|add|image'); event.stopPropagation();"
> id="menuitem_cookieDisplay"
And shouldn't this be id=menuitem_imageDisplay"

> -              label="&cookiePopupManager.label;"
> -              accesskey="&cookiePopupManager.accesskey;"
> -              oncommand="popupBlockerMenuCommand(event.target);">
> +              label="&cookiePopupsManager.label;"
> +              accesskey="&cookiePopupsManager.accesskey;"
These are not cookies any more so please shorten the entities to e.g. &popupManager.label;

> +              oncommand="if (event.target.id == 'ManagePopups')
> +                           toDataManager(hostUrl() + '|permissions|add|popup');
Move this back to ManagePopups and add event.stopPropagation();

> +                         else
> +                           CookieImagePopupAction(event.target);">

>            <menupopup id="menupopup_checkForVisibility"
> -                     onpopupshowing="CheckForVisibility(event);"
> +                     onpopupshowing="CheckPermissionsMenu('popup', this);
> +                                      CheckForVisibility(event);"

Just make CheckForVisibility() call CheckPermissionsMenu() with the right arguments.

> +<!ENTITY cookiePopupsMessageTitle.label "Popups Permissions Changed">
> +<!ENTITY cookieManagePopups.label "Manage Popups">
> +<!ENTITY cookieManagePopups.accesskey "M">
Just drop the cookie prefixes.

> +<!ENTITY cookiePopupsMessageTitle.label "Popups Permissions Changed">
popupMessageTitle. to be consistent with the naming conventions.
but *popups* for the rest.
Attachment #8752480 - Flags: review?(philip.chee)
Attachment #8752480 - Flags: review?(iann_bugzilla)
Attachment #8752480 - Flags: review-
(Assignee)

Comment 20

a year ago
Created attachment 8760088 [details] [diff] [review]
1055954-popupmanage-V4.patch

I hope I have taken care of all issues raised. 

The individual management of popup windows in the status and notification bar is currently broken due to bug 1273685 which changed the way blocked popups are reported. This needs to be ported in a separate bug.

Tested the match on c-a and it worked fine.
Attachment #8752480 - Attachment is obsolete: true
Attachment #8760088 - Flags: review?(philip.chee)

Comment 21

a year ago
Comment on attachment 8760088 [details] [diff] [review]
1055954-popupmanage-V4.patch

r=me a=me with the following fixed:

>            <menupopup id="menupopup_checkForVisibility"
> -                     onpopupshowing="CheckForVisibility(event);"
> +                     onpopupshowing="CheckForVisibility(event,this);"
Space after comma: (event, this);

> -function StatusbarViewPopupManager()
I prefer to keep this function even though it's now a one liner call from the status bar.
Attachment #8760088 - Flags: review?(philip.chee) → review+
(Assignee)

Comment 22

a year ago
Created attachment 8763405 [details] [diff] [review]
1055954-popupmanage-V5.patch

Patch with latest suggestions incorporated.

Retested using local VS2015 x64 en-US build:

>> User agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 >> Firefox/50.0 SeaMonkey/2.47a1
>> Build identifier: 20160619230911

r+ and a+ from Philip Chee carried forward.
Attachment #8760088 - Attachment is obsolete: true
Attachment #8763405 - Flags: review+
(Assignee)

Comment 23

a year ago
Done:

https://hg.mozilla.org/comm-central/rev/96d2d9e01a35
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-seamonkey2.43: --- → wontfix
status-seamonkey2.44: --- → wontfix
status-seamonkey2.45: --- → wontfix
status-seamonkey2.46: --- → wontfix
status-seamonkey2.47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.47
You need to log in before you can comment on or make changes to this bug.