Closed Bug 270016 Opened 17 years ago Closed 7 years ago

Dialog shown when deleting many certificates is too big

Categories

(Core Graveyard :: Security: UI, defect)

1.0 Branch
x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla33

People

(Reporter: aleisterm, Assigned: Cykesiopka)

References

Details

(Whiteboard: [kerh-coz])

Attachments

(3 files, 2 obsolete files)

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 ;-)
Attached image Screenshot of the box
Confirming with Moz 1.8a4 on WinXP.  And moving to PSM.
Assignee: firefox → kaie
Status: UNCONFIRMED → NEW
Component: Preferences → Client Library
Ever confirmed: true
Product: Firefox → PSM
QA Contact: mconnor
Summary: Manage Certificates (I am using the German Version, I hope this is the correct translation) - Authority - Box is too big → Dialog shown when deleting many certificates is too big
Version: unspecified → 2.4
I agree.

This bug could be fixed by anybody with XUL ui experience.
Assigning to nobody.
Assignee: kaie → nobody
Question: Does keyboard key "Cancel" do the right thing?

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

Product: PSM → Core
Whiteboard: [kerh-coz]
*** Bug 321602 has been marked as a duplicate of this bug. ***
QA Contact: ui
Version: psm2.4 → 1.0 Branch
Duplicate of this bug: 987094
Duplicate of this bug: 139059
Attached patch bug270016_v0.patch (obsolete) — Splinter Review
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...
Attachment #8433051 - Flags: feedback?(dkeeler)
Screenshot of v0 on Lubuntu 14.04 and Windows 7.
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)
Attachment #8433051 - Flags: feedback?(dkeeler) → feedback+
(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.
Attached patch bug270016_v1.patch (obsolete) — Splinter Review
+ 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
Assignee: nobody → cykesiopka.bmo
Attachment #8433051 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8444016 - Flags: review?(dkeeler)
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 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.
Attachment #8444016 - Flags: review?(dkeeler) → review-
(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.
This is v1 minus the selection bits, and therefore has r+ (See Comment 14).

https://tbpl.mozilla.org/?tree=Try&rev=730581e465de
Attachment #8444016 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7e074262b062
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
QA Whiteboard: [good first verify]
Duplicate of this bug: 400038
Product: Core → Core Graveyard
Duplicate of this bug: 711089
You need to log in before you can comment on or make changes to this bug.