Last Comment Bug 442150 - "Backup all" button not enabled after cert import
: "Backup all" button not enabled after cert import
Status: RESOLVED FIXED
[psm-easy]
:
Product: Core Graveyard
Classification: Graveyard
Component: Security: UI (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Kai Engert (:kaie)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-06-26 16:04 PDT by Marcia Knous [:marcia - use ni]
Modified: 2016-09-27 13:03 PDT (History)
3 users (show)
mozillamarcia.knous: in‑litmus+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Screenshot of issue using Firefox 3 (45.17 KB, image/png)
2008-06-26 16:04 PDT, Marcia Knous [:marcia - use ni]
no flags Details
Screenshot of cert dialog after initial certificate install (43.29 KB, image/png)
2008-07-02 11:33 PDT, Marcia Knous [:marcia - use ni]
no flags Details
Patch v1 (1.49 KB, patch)
2008-07-08 06:02 PDT, Kai Engert (:kaie)
bugzilla: review+
Details | Diff | Splinter Review
Patch v2 (1.86 KB, patch)
2008-07-08 07:04 PDT, Kai Engert (:kaie)
bugzilla: review+
Details | Diff | Splinter Review
Patch v3 (2.29 KB, patch)
2008-07-08 14:58 PDT, Kai Engert (:kaie)
no flags Details | Diff | Splinter Review
Patch v4 (2.26 KB, patch)
2010-06-22 11:32 PDT, Kai Engert (:kaie)
gavin.sharp: review+
Details | Diff | Splinter Review

Description Marcia Knous [:marcia - use ni] 2008-06-26 16:04:00 PDT
Created attachment 327031 [details]
Screenshot of issue using Firefox 3

Seen while testing  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9) Gecko/2008053008 Firefox/3.0.

STR:
1. Install multiple certs in Firefox.
2. Observe the attached screenshot. I would expect the "Backup all" button to be available.

The one note I wanted to make is I think the first cert is expired. I don't know if that makes any difference.
Comment 1 Johnathan Nightingale [:johnath] 2008-06-27 07:35:39 PDT
I'm not seeing this on my machine ("Backup all..." is enabled) with three valid certs... MXR suggests this line is to blame, one way or another:

http://mxr.mozilla.org/mozilla/source/security/manager/pki/resources/content/certManager.js#108

It sort of seems like, in your case, the treeview is reporting 0 rows even though there are clearly rows present?  Kai knows this code better than I do.
Comment 2 Johnathan Nightingale [:johnath] 2008-06-27 07:37:06 PDT
It would be helpful to know if anything shows up in your js error console when you open this dialog (assuming "javascript.options.showInConsole" is set to true).
Comment 3 Marcia Knous [:marcia - use ni] 2008-06-27 11:37:35 PDT
Oddly enough, when I started running my machine today and check the cert panel, the "Backup All" button is now enabled. I did restart the browser several times. I will resolve this as WFM for now but I will keep an eye out if it happens again.
Comment 4 Marcia Knous [:marcia - use ni] 2008-06-27 15:46:03 PDT
https://litmus.mozilla.org/show_test.cgi?id=4208 should cover this scenario if you have multiple certs installed.
Comment 5 Eddy Nigg (StartCom) 2008-06-27 19:35:49 PDT
The screen shot shows that you have selected a specific certificate. In that case the "Backup All' button is disabled. Deselect any of the selected certificates (or simply don't select any in your "Your Certificates" store) and 'Backup All" will be back. 
Comment 6 Kai Engert (:kaie) 2008-06-30 16:25:07 PDT
Thanks Eddy.
Maybe backup-all should always be enabled, even if you have selected something.
Comment 7 Marcia Knous [:marcia - use ni] 2008-06-30 16:40:37 PDT
I agree with Kai in Comment 6 - it makes more sense to have it enabled even if something is selected.
Comment 8 Eddy Nigg (StartCom) 2008-06-30 19:51:00 PDT
(In reply to comment #6)
> Maybe backup-all should always be enabled, even if you have selected something.
> 

Not sure. If one hits "Backup All" unintentionally, it could lead to some unwanted surprises maybe.

Comment 9 Kai Engert (:kaie) 2008-07-01 01:55:52 PDT
Johnathan, what's your opinion? See comment 6 and following.
Comment 10 Johnathan Nightingale [:johnath] 2008-07-02 06:24:55 PDT
(In reply to comment #9)
> Johnathan, what's your opinion? See comment 6 and following.

I think my opinion is contingent on whether Eddy's diagnosis is correct.  I suspected there might have been multi-select foo at work here when I saw Marcia's report, but I tried multi-selecting (on Mac) and the Backup All button remained.  I tried selecting multiple certs, multiple of the CA meta-entries, and combinations, and in all cases, the Backup All button remained.  What am I missing here?

I do think that "backup all" should always backup _all_, and that subset-backups can be supported through multi-selecting and clicking the "backup..." button.  But I'm not sure that's even the bug under discussion until we can confirm that that is the cause.  :)

Comment 11 Marcia Knous [:marcia - use ni] 2008-07-02 11:33:32 PDT
Created attachment 327834 [details]
Screenshot of cert dialog after initial certificate install

I think I see what may be going on. While testing, if I install multiple certs and don't select any, the "Backup All" is not enabled after the initial install. As soon as I switch tabs away from "Your Certificates" and then go back to the tab, then the Backup All button is enabled. So that may be the bug that I am seeing, and may explain why when I came back after a restart the button was enabled.
Comment 12 Johnathan Nightingale [:johnath] 2008-07-02 11:39:58 PDT
Yep, that's entirely plausible!  Kai, I bet that after installing the cert, LoadCerts isn't being called again to reset the button enablement?

I think we should leave the semantic meaning of "Backup All..." as-is, but we should fix the stale-disabled-state bug here.  Would you agree?
Comment 13 Kai Engert (:kaie) 2008-07-08 05:34:48 PDT
(In reply to comment #12)
> I think we should leave the semantic meaning of "Backup All..." as-is, but we
> should fix the stale-disabled-state bug here.  Would you agree?

Yes, I agree
Comment 14 Kai Engert (:kaie) 2008-07-08 05:55:56 PDT
(In reply to comment #12)
> Kai, I bet that after installing the cert,
> LoadCerts isn't being called again to reset the button enablement?

LoadCerts is a very expensive function call, which can take many seconds to complete.

We should not call it after importing.

We should create a separate function that does the button enabling etc. and call it after importing.
Comment 15 Kai Engert (:kaie) 2008-07-08 06:02:58 PDT
Created attachment 328472 [details] [diff] [review]
Patch v1
Comment 16 Johnathan Nightingale [:johnath] 2008-07-08 06:53:46 PDT
Comment on attachment 328472 [details] [diff] [review]
Patch v1

>diff --git a/security/manager/pki/resources/content/certManager.js b/security/manager/pki/resources/content/certManager.js
>+  enableBackupAllButton();
>+}
>+
>+function enableBackupAllButton()
>+{
>   var rowCnt = userTreeView.rowCount;
>   var enableBackupAllButton=document.getElementById('mine_backupAllButton');
>   if(rowCnt < 1) {
>     enableBackupAllButton.setAttribute("disabled",true);
>   } else  {
>     enableBackupAllButton.setAttribute("enabled",true);

I know this is the code as-is, but I find the variable name "enableBackupAllButton" confusing, since it refers to a button element, not a boolean about what to do WITH that button.  If you feel like it, changing this to just be "backupAllButton" would seem preferable.

Looks good to me - do we want to add a test here to make sure this doesn't happen again?  I don't honestly know what PSM's unit-testing expectations are, if any?
Comment 17 Kai Engert (:kaie) 2008-07-08 07:04:21 PDT
Created attachment 328478 [details] [diff] [review]
Patch v2

I like your proposal for an additional reason, because my v1 patch would introduce an ambiguity between function and variable names.
Comment 18 Johnathan Nightingale [:johnath] 2008-07-08 07:07:34 PDT
Comment on attachment 328478 [details] [diff] [review]
Patch v2

I haven't tested this patch, but it looks good.
Comment 19 Kai Engert (:kaie) 2008-07-08 14:57:36 PDT
(In reply to comment #18)
> I haven't tested this patch, but it looks good.


Sorry, I should have tested before submitting.
My testing has now shown the patch does not work.

I think it's because "enabled" and "disabled" are separate attributes.

When entering the dialog with zero certs, 
it sets disabled=true

After importing a cert,
it sets enabled=true

But that doesn't enable the button...


I've changed the code to always modify attribute "disabled", setting it to either true or false. That makes the patch work.


During testing I thought of one more scenario: When deleting the last cert the backup-all button should go to disabled.
I'll add a call to the new function whenever we delete a personal cert.
Comment 20 Kai Engert (:kaie) 2008-07-08 14:58:48 PDT
Created attachment 328571 [details] [diff] [review]
Patch v3
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-07-08 16:27:25 PDT
.removeAttribute("disabled") is a more reliable way of enabling something (many bindings only check for the presence of the attribute, ignoring its value).
Comment 22 Johnathan Nightingale [:johnath] 2008-07-09 05:45:48 PDT
(In reply to comment #20)
> Created an attachment (id=328571) [details]
> Patch v3
> 

What Gavin said.  :)  (And I should have caught.  :(  Bad johnath, no cookie.)
Comment 23 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-07-09 13:11:38 PDT
Actually, you should just use the property "button.disabled = true/false", which is even more reliable!
Comment 24 Johnathan Nightingale [:johnath] 2008-10-23 08:46:44 PDT
Comment on attachment 328571 [details] [diff] [review]
Patch v3

Gavin's comments here are right on - let's do this as reliably as possible.
Comment 25 Kai Engert (:kaie) 2010-06-22 11:32:19 PDT
Created attachment 453118 [details] [diff] [review]
Patch v4

So instead of 

  if(rowCnt < 1) {
    backupAllButton.setAttribute("disabled",true);
  } else  {
    backupAllButton.setAttribute("disabled",false);
  }


you want this

  if(rowCnt < 1) {
    backupAllButton.disabled = true;
  } else  {
    backupAllButton.disabled = false;
  }

right?

Patch updated
Comment 26 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-11 17:34:58 PDT
Comment on attachment 453118 [details] [diff] [review]
Patch v4

>diff --git a/security/manager/pki/resources/content/certManager.js b/security/manager/pki/resources/content/certManager.js

>+function enableBackupAllButton()

>   if(rowCnt < 1) {
>+    backupAllButton.disabled = true;
>   } else  {
>+    backupAllButton.disabled = false;
>   }

This could just be:

backupAllButton.disabled = rowCnt < 1;

Kind of silly that this took me so long to get to, sorry about that. This UI seems kind of broken in general (collapsable treeview means the buttons only work when child rows are selected, etc.). We should probably fix it at some point!
Comment 27 Kai Engert (:kaie) 2011-07-20 08:07:28 PDT
(In reply to comment #26)
> backupAllButton.disabled = rowCnt < 1;

fixed with Gavin's change proposal added.

http://hg.mozilla.org/mozilla-central/rev/ff199a2f3ab4

Note You need to log in before you can comment on or make changes to this bug.