"Backup all" button not enabled after cert import

RESOLVED FIXED

Status

Core Graveyard
Security: UI
RESOLVED FIXED
9 years ago
7 months ago

People

(Reporter: marcia, Assigned: kaie)

Tracking

Trunk
x86
Mac OS X
Bug Flags:
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [psm-easy])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

9 years ago
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.
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.
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).
(Reporter)

Comment 3

9 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-litmus?
Resolution: --- → WORKSFORME
(Reporter)

Comment 4

9 years ago
https://litmus.mozilla.org/show_test.cgi?id=4208 should cover this scenario if you have multiple certs installed.
Flags: in-litmus? → in-litmus+

Comment 5

9 years ago
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. 
(Assignee)

Comment 6

9 years ago
Thanks Eddy.
Maybe backup-all should always be enabled, even if you have selected something.
(Reporter)

Comment 7

9 years ago
I agree with Kai in Comment 6 - it makes more sense to have it enabled even if something is selected.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Summary: "Backup all" is greyed out when I have multiple certs installed → "Backup all" is greyed out when I have a cert selected

Comment 8

9 years ago
(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.

(Assignee)

Comment 9

9 years ago
Johnathan, what's your opinion? See comment 6 and following.
(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.  :)

(Reporter)

Comment 11

9 years ago
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.
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?
Summary: "Backup all" is greyed out when I have a cert selected → "Backup all" button not enabled after cert import
(Assignee)

Comment 13

9 years ago
(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
(Assignee)

Comment 14

9 years ago
(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.
(Assignee)

Comment 15

9 years ago
Created attachment 328472 [details] [diff] [review]
Patch v1
Attachment #328472 - Flags: review?(johnath)
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?
Attachment #328472 - Flags: review?(johnath) → review+
(Assignee)

Comment 17

9 years ago
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.
Attachment #328472 - Attachment is obsolete: true
Attachment #328478 - Flags: review?(johnath)
Comment on attachment 328478 [details] [diff] [review]
Patch v2

I haven't tested this patch, but it looks good.
Attachment #328478 - Flags: review?(johnath) → review+
(Assignee)

Comment 19

9 years ago
(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.
(Assignee)

Comment 20

9 years ago
Created attachment 328571 [details] [diff] [review]
Patch v3
Attachment #328478 - Attachment is obsolete: true
Attachment #328571 - Flags: review?(johnath)
.removeAttribute("disabled") is a more reliable way of enabling something (many bindings only check for the presence of the attribute, ignoring its value).
(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.)
Actually, you should just use the property "button.disabled = true/false", which is even more reliable!
Comment on attachment 328571 [details] [diff] [review]
Patch v3

Gavin's comments here are right on - let's do this as reliably as possible.
Attachment #328571 - Flags: review?(johnath)
(Assignee)

Comment 25

7 years ago
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
Attachment #328571 - Attachment is obsolete: true
Attachment #453118 - Flags: review?(gavin.sharp)
(Assignee)

Updated

7 years ago
Whiteboard: [psm-easy]
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!
Attachment #453118 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 27

6 years ago
(In reply to comment #26)
> backupAllButton.disabled = rowCnt < 1;

fixed with Gavin's change proposal added.

http://hg.mozilla.org/mozilla-central/rev/ff199a2f3ab4
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago6 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.