Dialog shown when deleting many certificates is too big

RESOLVED FIXED in mozilla33

Status

Core Graveyard
Security: UI
RESOLVED FIXED
14 years ago
a year ago

People

(Reporter: thebug, Assigned: Cykesiopka)

Tracking

1.0 Branch
mozilla33
x86
Windows XP

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [kerh-coz])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

14 years ago
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 ;-)
(Reporter)

Comment 1

14 years ago
Created attachment 166017 [details]
Screenshot of the box

Comment 2

14 years ago
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

Comment 3

14 years ago
I agree.

This bug could be fixed by anybody with XUL ui experience.
Assigning to nobody.
Assignee: kaie → nobody

Comment 4

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

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

Updated

13 years ago
Component: Security: UI → Security: UI
Product: PSM → Core

Updated

13 years ago
Whiteboard: [kerh-coz]

Comment 5

13 years ago
*** Bug 321602 has been marked as a duplicate of this bug. ***
QA Contact: ui

Updated

10 years ago
Version: psm2.4 → 1.0 Branch
(Assignee)

Updated

4 years ago
Duplicate of this bug: 987094
(Assignee)

Updated

4 years ago
Duplicate of this bug: 139059
(Assignee)

Comment 8

4 years ago
Created attachment 8433051 [details] [diff] [review]
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...
Attachment #8433051 - Flags: feedback?(dkeeler)
(Assignee)

Comment 9

4 years ago
Created attachment 8433052 [details]
bug270016_v0 Screenshot.png

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+
(Assignee)

Comment 11

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

Comment 12

4 years ago
Created attachment 8444016 [details] [diff] [review]
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
Assignee: nobody → cykesiopka.bmo
Attachment #8433051 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8444016 - Flags: review?(dkeeler)
(Assignee)

Comment 13

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

Comment 15

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

Comment 16

4 years ago
Created attachment 8444996 [details] [diff] [review]
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
Attachment #8444016 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7e074262b062
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
QA Whiteboard: [good first verify]
(Assignee)

Updated

3 years ago
Duplicate of this bug: 400038
Product: Core → Core Graveyard
(Assignee)

Updated

a year ago
Duplicate of this bug: 711089
You need to log in before you can comment on or make changes to this bug.