270016 - Dialog shown when deleting many certificates is too big

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[thebug]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.7.5) Gecko/20041108 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.7.5) Gecko/20041108 Firefox/1.0

When I open
Preferences - Certificates - Manage Certificates (I am using the German Version,
I hope this is the correct translation) - Authority
and mark everything and click on "Delete" a box opens that is too big to be
displayed on the screen, since it shows all the authorities I want to delete.
I don't even see the "Ok" or "Yes" button to click on, and I am using a 21 inch
screen.
A smaller box with a scrollbar would be better.
Please mail me back if you shouldn't understand what I mean.
I even can give you a call to describe it.

Please continue this great project and thank you for all your work!

Reproducible: Always
Steps to Reproduce:
1. Click on Preferences
2. Click on Certificates
3. Manage Certificates (I am using the German Version, please ask me if I shall
find out how this button is called exactly in English)
4. Authority
5. Select all
6. Click on Delete

Actual Results:  
A box popped up that asked me if I want to delete all the authorities. But I did
not even see the the Ok or Yes button

Expected Results:  
A smaller box with a scrollbar

Please contact me if you should not understand what I mean.
I would even be happy to give you a call (whoever will read this ;-)

Comment 1

[thebug]

Attachment: Screenshot of the box

Comment 2

[Josh Birnbaum]

Confirming with Moz 1.8a4 on WinXP.  And moving to PSM.

Comment 3

[Kai Engert (:KaiE:)]

I agree.

This bug could be fixed by anybody with XUL ui experience.
Assigning to nobody.

Comment 4

[Kai Engert (:KaiE:)]

Question: Does keyboard key "Cancel" do the right thing?

If not, this bug should be changed to "severity major".

Comment 5

[Josh Birnbaum]

*** Bug 321602 has been marked as a duplicate of this bug. ***

Comment 8

[:Cykesiopka]

Attachment: bug270016_v0.patch

This patch puts the CAs/Servers etc inside a scrollable list.

A few notes:
1. I chose <richlistbox> because:
 - I feel it's better to have the list of CAs etc be visually distinct from the surrounding labels
 - <listbox> and flex don't mix well

2. The list is now horizontally scrollable as well
 - 35em comfortably contains by default the longest entry I could find (see the screenshot I'm going to attach)

3. The list currently allows single selection. I'm worried this might mislead people into thinking they're only deleting the selected entry, but maybe I'm overthinking this...

Comment 9

[:Cykesiopka]

Attachment: bug270016_v0 Screenshot.png

Screenshot of v0 on Lubuntu 14.04 and Windows 7.

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8433051 [details] [diff] [review]
bug270016_v0.patch

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

This looks like a separate issue, but why is the name on that second entry blank?
About the selection issue - I assume you're talking about entries in the richlistbox you added? I would see if you can make them not selectable.
In general, it's great that you're working on these bugs, but I would be wary of putting a lot of effort into this when it's so fundamentally unusable that it needs to be replaced completely. That said, I would definitely appreciate your contributions to the new UI (unfortunately, it has yet to be designed - I'm working on making that happen).

::: security/manager/pki/resources/content/deletecert.js
@@ +63,5 @@
>    
>    setText("confirm",confirm);
>  
>    var box=document.getElementById("certlist");
> +  var listItem, label;

nit: declare these where they're used (i.e. in the loop)

Comment 11

[:Cykesiopka]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #10)
> This looks like a separate issue, but why is the name on that second entry
> blank?

I think it's because that cert (and I suspect the others for which this occurs) don't include a commonName, but in certManager.js commonName is used unconditionally:

438   for (t=0; t<numcerts; t++) 
439   {
440     var tree_item = selected_tree_items[t];
441     var c = tree_item.cert;
442     if (!c) {
443       params.SetString(t+1, tree_item.hostPort);
444     }
445     else {
446       params.SetString(t+1, c.commonName);
447     }
448 
449   }

> About the selection issue - I assume you're talking about entries in the
> richlistbox you added? I would see if you can make them not selectable.

Yes. I'll see what I can do.

> In general, it's great that you're working on these bugs, but I would be
> wary of putting a lot of effort into this when it's so fundamentally
> unusable that it needs to be replaced completely.

Yeah, I don't have any plans to work on bugs for the current cert UI past this one, so it should be fine.

> That said, I would
> definitely appreciate your contributions to the new UI (unfortunately, it
> has yet to be designed - I'm working on making that happen).

I'll be glad to help when the time comes.

Comment 12

[:Cykesiopka]

Attachment: bug270016_v1.patch

+ Make richlistitems "not selectable" (I couldn't find a way to actually do this, so this is the least hacky solution I could find to make the items look non-selectable)
+ Fix nit from Comment 10

Comment 13

[:Cykesiopka]

Comment on attachment 8444016 [details] [diff] [review]
bug270016_v1.patch

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

::: security/manager/pki/resources/content/deletecert.js
@@ +59,5 @@
>    }
>    var confirReference = document.getElementById('confirm');
>    var impactReference = document.getElementById('impact');
>    document.title = title;
>    

Since I'm here, I'll remove this whitespace as well in the next patch...

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8444016 [details] [diff] [review]
bug270016_v1.patch

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

r- for now for the selection issue. If you want to not worry about it and take out that bit, r+.

::: security/manager/pki/resources/content/deletecert.js
@@ +67,5 @@
>    for(var x=0;x<numberOfCerts;x++)
>    {
> +    var listItem = document.createElement("richlistitem");
> +    listItem.setAttribute("style",
> +      "background-color: -moz-Field; color: -moz-FieldText;");

This is clever, but I think it's a bit fragile. We should probably figure out how to disable selection for real or just not worry about it. I don't think it would be the most confusing thing about the certificate manager UI.

Comment 15

[:Cykesiopka]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #14)
> r- for now for the selection issue. If you want to not worry about it and
> take out that bit, r+.
> 
> ::: security/manager/pki/resources/content/deletecert.js
> @@ +67,5 @@
> >    for(var x=0;x<numberOfCerts;x++)
> >    {
> > +    var listItem = document.createElement("richlistitem");
> > +    listItem.setAttribute("style",
> > +      "background-color: -moz-Field; color: -moz-FieldText;");
> 
> This is clever, but I think it's a bit fragile. We should probably figure
> out how to disable selection for real or just not worry about it. I don't
> think it would be the most confusing thing about the certificate manager UI.

Fair points. I still haven't found a way to truly disable selection without disabling the listbox entirely, but I don't really want to spend any more time on this... I will take out the selection part.

Comment 16

[:Cykesiopka]

Attachment: bug270016_v2.patch

This is v1 minus the selection bits, and therefore has r+ (See Comment 14).

https://tbpl.mozilla.org/?tree=Try&rev=730581e465de

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e074262b062

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7e074262b062