932116 - Cannot copy certificate viewer text

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, enhancement)

Description

[Ralph Giles (:rillian)]

Field values in the Certificate Viewer dialog are not selectable, so I cannot for example copy the certificate fingerprint for comparison or use elsewhere.

We should make the field values selectable.

Comment 1

[Ralph Giles (:rillian)]

This is MacOS 10.8.5; Firefox 27.0a1 (2013-10-24).

Comment 2

[Garrett Robinson [:grobinson]]

This is true in Thunderbird 24.0 on Ubuntu 13.04 as well. Very annoying!

Comment 3

[Garrett Robinson [:grobinson]]

Attachment: WIP 1

Changes the id'ed fields that are filled in by addAttributeFromCert from XUL <labels> (which are not selectable, and AFAICT can never be made so) to readonly XUL <textboxes>, following the approach in browser/base/content/pageinfo/pageInfo.xul

Comment 4

[Garrett Robinson [:grobinson]]

Attachment: WIP 2

Hooked up styles so the field looks like it did before, but is now selectable and copy/pastable (using css from pageInfo.css to make the textboox look more like a label).

I now have the problem that the Cert Viewer window is not sized properly - some of the longer textboxes are cut off on the right side by the window. This can be fixed by resizing the window, but it is not-obvious and bad UX. It would be better to fit the Cert Viewer window to the longest value string.

Comment 5

[Garrett Robinson [:grobinson]]

Attachment: 3.patch

Latest patch resolves issue completely. I reverted to using <label>s, which minimizes change and avoids the text wrap problem from WIP 2. I was able to do this by setting -moz-user-select:text on the labels, thanks to a tip from paul on IRC.

Unfortuantely, it was not as simple as just adding that CSS, due to Bug 546387. Basically, <label>s with the value attribute set that have -moz-user-select:text raise a GTK error when clicked upon (and the text is not selectable, or copyable). This was worked around by setting label.textContent instead of the value attribute, in the manner of aboutDialog.{xul,js,css}.

This patch also cleans up some trailing whitespace in viewCertDetails.xul.

Comment 6

[Garrett Robinson [:grobinson]]

Here's a thread from dev.tech.xul that outlines the various means to this end (which mirrors my earlier conversation on #developers) [0]. There seems to be a trade-off between using <labels> and <textboxes>. <textbox> automatically generates context menus, which must be done manually for <label>. On the other hand, <label> causes its parent container to be sized to contain it, while <textbox> does not.

[0] https://groups.google.com/forum/#!topic/mozilla.dev.tech.xul/IWJTTZWb4yM

Comment 7

[Garrett Robinson [:grobinson]]

Attachment: 4 (labels)

Remove unnecessary closing tags from previous patch.

Comment 8

[Garrett Robinson [:grobinson]]

Attachment: 4-alt (textboxes)

An alternative approach that uses readonly textboxes with the default "plain" class instead of labels with a custom class that makes them selectable. Context menus are created automatically, but window sizing does not expand to contain the widest element, so the some elements are cut off on the right by the default window size. I am not sure of the best way to resolve this.

Comment 9

[Kai Engert (:KaiE:)]

Hi Garret, could you please attach screenshots?

Comment 10

[Garrett Robinson [:grobinson]]

Attachment: 4.png

The cert viewer window is automatically sized to fit all of the labels in the right column. Text is selectable, and can be copied with Ctrl-c. Right-clicking on a field does not bring up a context menu.

Comment 11

[Garrett Robinson [:grobinson]]

Attachment: 4-alt.png

The window is not automatically sized to fit the longest values, and cuts the longer ones off on the right. Text is selectable, and copyable with either Ctrl-c or the context menu that is brought up by right-clicking a field.

Comment 12

[Garrett Robinson [:grobinson]]

Attachment: 5.patch

After discussing the options with :keeler, we decided it was best to stick with textboxes as the context menus are an important usability improvement.

I improved that patch by experimenting to find a sensible minimum width for that column that doesn't cut off the SHA fingerprint, based on em so it will be correct no matter what text size is used.

I also made the values column "flex", so if there are any values longer than the SHA fingerprint, the user can resize the window and see them (previously, resizing the window would not change the width of the column).

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This bug should have a firefox/toolkit peer reviewer.

Comment 14

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

Comment on attachment 831957 [details] [diff] [review]
5.patch

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

This looks good to me. I don't think that we have plans to eventually remove this pane is a reason to not improve it in the meanwhile. Also, yes, a firefox/toolkit peer should also review this.

::: security/manager/pki/resources/content/viewCertDetails.js
@@ +155,5 @@
>    var node = document.getElementById(nodeName);
>    if (!value) {
>      value = bundle.getString('notPresent');
>    }
> +  node.setAttribute('value', value);

Technically this fixes a whitespace issue, but since we're not touching anything else in this file, I'm inclined to leave this change out for now.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e3967dd6303

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/2e3967dd6303