340432 - plugin allowed sites dialog needs cancel button (also cookies, popups, etc)

Status: RESOLVED FIXED

(Firefox :: Settings UI, enhancement, P3)

Description

[Erik Purins]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4

When adding a new approved site from the 'protect your computer' bar->edit options button->allowed sites dialog, user cannot cancel accidental delete of originally approved update sites. There is no cancel button, and escape key does not exit the dialog. User can accidentally delete all the mozilla sites, without an in-dialog way to restore those settings.

When the allowed sites dialog is spawned this way, it needs a cancel option, and the tan 'change preferences' bar needs to stay in the window.

Reproducible: Always

Steps to Reproduce:
1. browse to unapproved site.
2. click install link.
3. click in tan 'change preferences' toolbar at the top (change allowed sites dialog opens)
4. delete any originally approved update site
5. user must now save prefs, with no cancel option. originally approved site is lost.

Actual Results:  
Originally approved site is lost.

Expected Results:  
Cancel button and escape key allow user to return to original prefs.
Tan 'change preferences' bar remains at top of window.

Comment 1

[Robert Strong (they/them - no direct email)]

Firefox -> Preferences since the dialog mentioned is for xpinstall whitelisting which is used by xpinstall to hand off xpi installs to both the plugin finder service and the extension manager.

Comment 2

[Daniel Veditz [:dveditz]]

All of the "Exception" dialogs work this way, on purpose so I'll flag this as an enhancement request rather than a bug. And I can confirm it as a reasonable request.

The dialogs have "close" buttons for when you're done editing things, but no "OMG what have I done? abort!" cancel button. This can be particularly bad when you've deleted a password or cookie from the wrong site, you can't simply "put it back" the way you can restore whitelisted addons sites.

Even the simpler permission-manager exception dialogs (popup blocker, addons whitelist, cookie/image blocked sites) would be tough to restore if you used the Remove All button. Remove All is rare enough and destructive enough that it's probably worth sticking a confirm dialog up there.

Adding "uiwanted" to see if the UI gurus agree this might be a usability problem, but even if they do adding some sort of transaction ability to these dialogs might not be trivial.

Comment 3

[Mike Connor [:mconnor]]

Would be good to do, not difficult, but not trivial.

Comment 4

[Viktor Stoitschev]

Hello,
I would like to fix this bug, however I'm not sure if I've understood it thouroughly. In the current version of Firefox (3.5.5) there is no allowed sites dialog as described above. Instead there is an "allowed sites for add-ons" option, where you can allow particular sites to install add-ons. There is also a "Remove all" button but no cancel button or warning for instance if you've removed something you didn't intend to. I would start fixing this issue if it correspodns to the given bug.

Comment 5

[Viktor Stoitschev]

Attachment: solved the bug for "remove all" button for permissions.xul and passwordManagerExceptions.xul

Following Tools>Options>Security there is the
allowed sites for add-ons "Exceptions" dialog, where particular sites can be allowed to install add-ons without a prompt. There is also a "Remove all" button which deletes all sites without confirmation. With this patch this issue is addressed by adding a new confirmation prompt, which pops up when clicking the "Remove all" button. The same issue exists with the "Exceptions- Saved passwords" dialog (Tools>Options>Security> Passwords> Save Passwords > Exceptions...) and is addressed in the same manner.

Comment 6

[Viktor Stoitschev]

Comment on attachment 422895 [details] [diff] [review]
solved the bug for "remove all" button for permissions.xul and passwordManagerExceptions.xul

diff --git a/browser/components/preferences/permissions.js b/browser/components/preferences/permissions.js
--- a/browser/components/preferences/permissions.js
+++ b/browser/components/preferences/permissions.js
@@ -284,16 +284,40 @@ var gPermissionManager = {
     for (var i = 0; i < removedPermissions.length; ++i) {
       var p = removedPermissions[i];
       this._pm.remove(p.host, p.type);
     }    
     document.getElementById("removePermission").disabled = !this._permissions.length;
     document.getElementById("removeAllPermissions").disabled = !this._permissions.length;
   },
   
+  
+  deleteAllSites: function()
+  {
+		
+		var brandBundle = Components.classes[ "@mozilla.org/intl/stringbundle;1" ]
+                             .getService( Components.interfaces.nsIStringBundleService )
+                               .createBundle("chrome://browser/locale/preferences/preferences.properties");
+		var remPermTitle = brandBundle.GetStringFromName("removeAllPermissionsTitle");
+		var remPermPrompt = brandBundle.GetStringFromName("removeAllPermissionsPrompt");
+		
+		var prompts = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
+							.getService(Components.interfaces.nsIPromptService);
+	
+		var result = prompts.confirm(null, remPermTitle, remPermPrompt);
+
+		if (result) {
+		gPermissionManager.onAllPermissionsDeleted();
+		}
+		else {
+		//do nothing
+		}	
+  },
+  
+  
   onAllPermissionsDeleted: function ()
   {
     if (!this._view.rowCount)
       return;
     var removedPermissions = [];
     gTreeUtils.deleteAll(this._tree, this._view, this._permissions, removedPermissions);
     for (var i = 0; i < removedPermissions.length; ++i) {
       var p = removedPermissions[i];
diff --git a/browser/components/preferences/permissions.xul b/browser/components/preferences/permissions.xul
--- a/browser/components/preferences/permissions.xul
+++ b/browser/components/preferences/permissions.xul
@@ -96,21 +96,22 @@
     </tree>
   </vbox>
   <hbox align="end">
     <hbox class="actionButtons" flex="1">
       <button id="removePermission" disabled="true"
               accesskey="&removepermission.accesskey;"
               icon="remove" label="&removepermission.label;"
               oncommand="gPermissionManager.onPermissionDeleted();"/>
-      <button id="removeAllPermissions"
+		<button id="removeAllPermissions"
               icon="clear" label="&removeallpermissions.label;"
               accesskey="&removeallpermissions.accesskey;" 
-              oncommand="gPermissionManager.onAllPermissionsDeleted();"/>
-      <spacer flex="1"/>
+              oncommand="gPermissionManager.deleteAllSites()"/>
+		<spacer flex="1"/>
+
 #ifndef XP_MACOSX
       <button oncommand="close();" icon="close"
               label="&button.close.label;" accesskey="&button.close.accesskey;"/>
 #endif
     </hbox>
     <resizer dir="bottomend"/>
   </hbox>
 </window>
diff --git a/browser/locales/en-US/chrome/browser/preferences/preferences.properties b/browser/locales/en-US/chrome/browser/preferences/preferences.properties
--- a/browser/locales/en-US/chrome/browser/preferences/preferences.properties
+++ b/browser/locales/en-US/chrome/browser/preferences/preferences.properties
@@ -16,16 +16,18 @@ cookiepermissionstitle=Exceptions - Cook
 addonspermissionstext=You can specify which web sites are allowed to install add-ons. Type the exact address of the site you want to allow and then click Allow.
 addons_permissions_title=Allowed Sites - Add-ons Installation
 popuppermissionstext=You can specify which web sites are allowed to open pop-up windows. Type the exact address of the site you want to allow and then click Allow.
 popuppermissionstitle=Allowed Sites - Pop-ups
 imagepermissionstext=You can specify which web sites are allowed to load images. Type the exact address of the site you want to manage and then click Block or Allow.
 imagepermissionstitle=Exceptions - Images
 invalidURI=Please enter a valid hostname
 invalidURITitle=Invalid Hostname Entered
+removeAllPermissionsTitle=Confirm
+removeAllPermissionsPrompt=Are you sure you wish to delete all allowed web sites?
 
 #### Master Password
 
 pw_change2empty_in_fips_mode=You are currently in FIPS mode. FIPS requires a non-empty Master Password.
 pw_change_failed_title=Password Change Failed
 
 #### Fonts
 
diff --git a/toolkit/components/passwordmgr/content/passwordManagerExceptions.js b/toolkit/components/passwordmgr/content/passwordManagerExceptions.js
--- a/toolkit/components/passwordmgr/content/passwordManagerExceptions.js
+++ b/toolkit/components/passwordmgr/content/passwordManagerExceptions.js
@@ -108,16 +108,38 @@ function DeleteReject() {
 
 function DeleteAllRejects() {
   DeleteAllFromTree(rejectsTree, rejectsTreeView,
                         rejects, deletedRejects,
                         "removeReject", "removeAllRejects");
   FinalizeRejectDeletions();
 }
 
+function ConfirmDeleteAllRejects(){
+		var brandBundle = Components.classes[ "@mozilla.org/intl/stringbundle;1" ]
+                             .getService( Components.interfaces.nsIStringBundleService )
+                               .createBundle("chrome://passwordmgr/locale/passwordmgr.properties");
+
+		
+		var remRejectTitle = brandBundle.GetStringFromName("removeAllRejectsTitle");
+		var remRejectPrompt = brandBundle.GetStringFromName("removeAllRejectsPrompt");
+		
+
+		var prompts = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(Components.interfaces.nsIPromptService);
+	
+		var result = prompts.confirm(null, remRejectTitle , remRejectPrompt);
+
+		if (result) {
+		DeleteAllRejects();
+		}
+		else {
+		//do nothing
+		}
+}
+
 function FinalizeRejectDeletions() {
   for (var r=0; r<deletedRejects.length; r++) {
     passwordmanager.setLoginSavingEnabled(deletedRejects[r].host, true);
   }
   deletedRejects.length = 0;
 }
 
 function HandleRejectKeyPress(e) {
diff --git a/toolkit/components/passwordmgr/content/passwordManagerExceptions.xul b/toolkit/components/passwordmgr/content/passwordManagerExceptions.xul
--- a/toolkit/components/passwordmgr/content/passwordManagerExceptions.xul
+++ b/toolkit/components/passwordmgr/content/passwordManagerExceptions.xul
@@ -71,13 +71,13 @@
         <separator class="thin"/>
         <hbox>
           <button id="removeReject" disabled="true" icon="remove"
                   accesskey="&remove.accesskey;"
                   label="&remove.label;" oncommand="DeleteReject();"/>
           <button id="removeAllRejects"
                   accesskey="&removeall.accesskey;"
                   icon="clear" label="&removeall.label;"
-                  oncommand="DeleteAllRejects();"/>
+                  oncommand="ConfirmDeleteAllRejects();"/>
         </hbox>
     </vbox>
   </prefpane>
 </prefwindow>
diff --git a/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties b/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties
--- a/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties
+++ b/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties
@@ -63,10 +63,12 @@ notifyBarDontChangeButtonAccessKey = D
 userSelectText = Please confirm which user you are changing the password for
 hidePasswords=Hide Passwords
 hidePasswordsAccessKey=P
 showPasswords=Show Passwords
 showPasswordsAccessKey=P
 noMasterPasswordPrompt=Are you sure you wish to show your passwords?
 removeAllPasswordsPrompt=Are you sure you wish to remove all passwords?
 removeAllPasswordsTitle=Remove all passwords
+removeAllRejectsPrompt=Are you sure you wish to remove all web sites?
+removeAllRejectsTitle=Confirm
 loginsSpielAll=Passwords for the following sites are stored on your computer:
 loginsSpielFiltered=The following passwords match your search:

Comment 7

[Viktor Stoitschev]

Attachment: solved the bug for "remove all" button for permissions.xul and passwordManagerExceptions.xul

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 422953 [details] [diff] [review]
solved the bug for "remove all" button for permissions.xul and passwordManagerExceptions.xul

First off, thanks for taking a stab at this.

I don't think adding a confirmation dialog is the right approach here.  It's the simplest approach, but the right answer, IMO, is to make the prefwindow not be instant apply, and perform the actions on clicking OK (which, ideally, would be changed to "Apply Changes").

On the patch itself, the files use 2 space indents, you're using tabs.  Also, there's no need to have empty else statements... you really want to look at existing code style and copy that.

Comment 9

[Liz Henry (:lizzard)]

mconnor, is this still relevant? I couldn't figure out where this option/feature is, if it is still as described.   Should it stay a good first bug?  Would you like to sign up to mentor it?

Comment 10

[Melissa O'Connor [:melissa]]

Changing to mconnor instead of moconnor.

Comment 11

[Mike Connor [:mconnor]]

* the Exceptions button for Addons under Security.
* Under Privacy: use custom settings, then Exceptions for cookies
* The Exceptions dialog for popups under Content

It's a shared dialog, just takes a different permissions arg.  It is still as described.

I could mentor, but I'd suggest someone like jaws/mconley/mattn instead as I'm not currently working on desktop Firefox directly.

Comment 12

[Mike Lopez]

Attachment: bug-340432.patch

First attempt at patch.

Comment 13

[Mike Lopez]

Attachment: bug-340432.patch

Had some failing tests that needed to be updated based on my changes. I had to hack up the test code a lot because it was assumed the preferences window would stay open between the running of several tests which is not true any more.

Comment 14

[Mike Lopez]

Attachment: bug-340432.patch

Small test cleanup.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Thanks for the patch!

Let's confirm with a member of the UX team that we actually want the behavior suggested in comment 8.

In the mean time, I'll flag a couple of code reviewers who are more active than mconnor to provide some feedback on the patch.

One thing I noticed right away: instead of modifying code that references services via Components.classes, we should move them to use Services.jsm (https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Services.jsm) while we're at it. For example, in this patch, after you've imported Services.jsm, you can use Services.perms instead of _pm, and Services.obs instead of "os".

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8541980 [details] [diff] [review]
bug-340432.patch

Review of attachment 8541980 [details] [diff] [review]:
-----------------------------------------------------------------

General notes:
* This is great - much prefer this solution. Undo is almost always better than confirming.
* What Gavin said about using Services.jsm
* Plenty of trailing whitespace in these changes, and the file in general, which would be nice to get rid of

::: browser/components/preferences/permissions.js
@@ +121,3 @@
>      }
> +    else {
> +      if (!capabilityExists) {

Nit: else if

@@ +155,5 @@
> +    // permission manager.
> +    let isNewPermission = false;
> +    for (let i = 0; i < this._permissionsToAdd.length; ++i) {
> +      if (this._permissionsToAdd[i].host == aPermission.host) {
> +        this._permissionsToAdd.splice(i, 1);

This makes me think _permissionsToAdd/_permissionsToDelete should be a Map, using the host as a key.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map

@@ +172,5 @@
> +    // Re-do the sort, if the status changed from Block to Allow
> +    // or vice versa, since if we're sorted on status, we may no
> +    // longer be in order. 
> +    if (this._lastPermissionSortColumn.id == "statusCol") {
> +        gTreeUtils.sort(this._tree, this._view, this._permissions,

Feels like this should be split out to it's own resort() method.

@@ +262,5 @@
>    },
>    
>    uninit: function ()
>    {
> +    if (!this._observerRemoved) {

Why is this needed now when it wasn't before?

@@ +371,5 @@
> +                              .getService(Components.interfaces.nsIIOService);
> +
> +    this.uninit();
> +
> +    for (let i = 0; i < this._permissionsToAdd.length; ++i) {

Prefer for-of loops for new code, as it's far less verbose:

  for (let permissionParams of this._permissionsToAdd) {
    ...
  }

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of

::: browser/locales/en-US/chrome/browser/preferences/permissions.dtd
@@ +22,5 @@
>  <!ENTITY windowClose.key              "w">
>  
> +<!ENTITY button.cancel.label          "Cancel">
> +<!ENTITY button.cancel.accesskey      "C">
> +<!ENTITY button.ok.label              "Ok">

"OK"/"Cancel" pairs are a bit ambiguous, and don't clearly communicate what will happen when pressing one of the buttons. So I think this string should be "Save changes" or "Apply changes" - to make it clear that this button is an action, and that the changes have not been made yet. ("Cancel" is fine as-is, as it's given context by the other button.)

Comment 17

[Mike Lopez]

Attachment: bug-340432.patch

Made requested changes.

The _observerRemoved property is needed because I remove the observer when I'm about to write out the adds/deletes that way we don't observe our own changes we are making. The observer is also attempted to be removed as part of the window's onunload event. So if I don't check to see if the observer has been removed, I get an error saying the observer could not be found etc. since it was already removed. Please let me know if there is a better way to handle this situation.

Comment 18

[Mike Lopez]

Attachment: bug-340432.patch

Missed some semicolons

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8543667 [details] [diff] [review]
bug-340432.patch

Review of attachment 8543667 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/en-US/chrome/browser/preferences/permissions.dtd
@@ +23,5 @@
>  
> +<!ENTITY button.cancel.label          "Cancel">
> +<!ENTITY button.cancel.accesskey      "C">
> +<!ENTITY button.ok.label              "Save Changes">
> +<!ENTITY button.ok.accesskey          "s">

Capital "S" for the accesskey - so it underlines the first "S", rather than the "s" at the end.

Comment 20

[Mike Lopez]

Attachment: bug-340432.patch

Made requested changes.

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8544222 [details] [diff] [review]
bug-340432.patch

Review of attachment 8544222 [details] [diff] [review]:
-----------------------------------------------------------------

Just waiting on Philipp to give some feedback now.

Comment 22

[Mike Lopez]

Hey is there someone else that can take a look at this?

Comment 23

[:Gavin Sharp [email: [e-mail]]]

I poked Philipp on IRC, he's taking a look.

Comment 24

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Looks good to me. The only thing I'm concerned about is how the cancel and save buttons are visually at the same level as the remove/remove all buttons. Perhaps we can move the cancel/save buttons to their own row.

Comment 25

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: multiple rows mockup

Quick mockup of what I was thinking of

Comment 26

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8544222 [details] [diff] [review]
bug-340432.patch

Lets get this fixed up with respect to comment 24 and attachment 8560212 [details].

Comment 27

[Mike Lopez]

Attachment: bug-340432.patch

The buttons are now on visually different rows as requested.

Comment 28

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8563855 [details] [diff] [review]
bug-340432.patch

Review of attachment 8563855 [details] [diff] [review]:
-----------------------------------------------------------------

(Utterly swamped, redirecting most bugs ATM)

Comment 29

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8563855 [details] [diff] [review]
bug-340432.patch

Hoping Steven can help with review here. Sorry to keep redirecting you,  l0p3s222.

Comment 30

[Steven MacLeod [:smacleod]]

Comment on attachment 8563855 [details] [diff] [review]
bug-340432.patch

Review of attachment 8563855 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. Big improvement :D

There are just a few trailing whitespace introductions left to clear up. I'll give an r+ on a patch with those cleared up.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fa061196f03

Michael, you might want to consider applying for Level 1 Commit Acess [1][2]. This would allow you to push to try and
test your own changes.

[1] https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/
[2] https://www.mozilla.org/en-US/about/governance/policies/commit/

::: browser/components/preferences/tests/browser_cookies_exceptions.js
@@ +11,2 @@
>  
> +  tests: 

nit: trailing whitespace.

@@ +11,4 @@
>  
> +  tests: 
> +    [
> +      { 

nit: trailing whitespace.

@@ +168,5 @@
> +              os.removeObserver(permObserver, "perm-changed");
> +
> +              if (testRunner.tests[testRunner._currentTest].cleanup) {
> +                testRunner.tests[testRunner._currentTest].cleanup();
> +              }        

nit: trailing whitespace.

@@ +180,5 @@
> +          let os = Cc["@mozilla.org/observer-service;1"]
> +                     .getService(Ci.nsIObserverService);
> +
> +          os.addObserver(permObserver, "perm-changed", false);
> +          

nit: trailing whitespace.

Comment 31

[Mike Lopez]

Attachment: bug-340432.patch

Cleaned up white space.

Thanks for pushing to try for me!

Comment 32

[Steven MacLeod [:smacleod]]

Comment on attachment 8568276 [details] [diff] [review]
bug-340432.patch

Review of attachment 8568276 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thanks!

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/722f9df4cc1c

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/722f9df4cc1c

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

Just seeing this now as we're going through bugs that should get uplifted to firefox 38 for in-content preferences. We can't uplift this bug because of the string changes, so we'll let it ride with 39.