1552227 - OTR, add fingerprint dialog: Improve typing experience

Status: RESOLVED FIXED

(Chat Core :: Security: OTR, defect)

Description

[Kai Engert [:KaiE:]]

The right-click menu for offline contacts will include an option to add the known fingerprint for a contact's OTR key.

The initial implementation will silently filter out all keystrokes that aren't hex characters.

Patrick thinks this approach might not be confusing, and has suggested to work on an improvement, he said:

"I would expected something like the box going red and an error appearing below it (immediately when an invalid letter is typed) saying that the data must be hexadecimal.

I did not suggest an alert box, especially not on every incorrect input. I do not think waiting for the user to press enter makes sense either if you can display something immediately. Maybe Alex has some ideas of how to make this UI better? This can be fixed in a follow-up."

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2019-05-27 11-22-58.png

We could use the occasion to start implementing the same paradigm that is getting introduced into the new Account Creation Dialog.
For input fields that need an helper and validation text, we can use the "info" icon with a descriptive tooltip, and when an error is detected we can swap the icon and permanently trigger the tooltip until the error is resolved.

I can quickly implement this if you like this workflow.
Thoughts?

Comment 2

[Patrick Cloke [:clokep]]

That's pretty much what I was going for, yeah. It seems reasonable to me.

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: otr-fingerprint.png

A little screenshot of the upcoming patch

Comment 4

[Alessandro Castellani [:aleca]]

Attachment: otr-fingerprint-error.png

Comment 5

[Alessandro Castellani [:aleca]]

Attachment: otr-add-fingerprint.patch

Here's the updated patch with the improved dialog.
I'd like Magnus to chime in as I've used the occasion to implement a base folder to handle shared assets. Since this style is very much identical to the new account hub and email wizard, and hopefully we'll keep reusing the same style and paradigm, I thought it would be good to set things up right off the bat.

Comment 6

[Kai Engert [:KaiE:]]

You're suggesting to show a "human fingerprint" image in this dialog.

I have seen the "human fingerprint" icon being shown on Android devices, in apps that allow logging in with the real human fingerprint, and on devices with a sensor for reading physical fingerprints.

In our context, the term fingerprint is used as a metaphor for something that is difficult to fake (like a fingerprint that used to be difficult to fake).

Aren't you worried that by showing an icon that depicts a human fingerprint, users might get confused and might believe we're talking about a real human finger?

Comment 7

[Kai Engert [:KaiE:]]

Maybe the screen should display a sentence that explains what the fingerprint metaphor means in our context, like:

"The term fingerprint is used as a metaphor for uniquely identifying information."

Comment 8

[Kai Engert [:KaiE:]]

On my Linux system, the buttons aren't visible (just a few top pixels). It seems the dialog doesn't resize to include all elements.

Comment 9

[Kai Engert [:KaiE:]]

Comment on attachment 9068277 [details] [diff] [review]
otr-add-fingerprint.patch

I'd like to suggest some enhancements.

>+otr-add-finger-description = If you know the 40 character long HEX fingerprint of your contact's OTR private key, enter it now.

You haven't touched this string, but I'd like to ask for two changes:
- please drop the word "private". It's sufficient, and more consistent, if we say "... of your contact's OTR key ..."
- I think the expression HEX isn't universally understood. It would be more correct to say "hexadecimal". On the other hand, it might be fine to completely remove that word, because in all situations I have ever encountered, fingerprints were always using hexadecimal strings, so the user shouldn't have to worry.

Also, when fingerprints are displayed on screen, it's common to mix in some whitespace.  

Instead of showing a string like ```D20F9171432BDCC8BFB8616EBE1FE79A8886F59F```,
the pidgin and coyim messengers, and also Thunderbird in OTR preferences, show it like ```D20F9171 432BDCC8 BFB8616E BE1FE79A 8886F59F```.

The reason is, reading/comparing 40 characters is difficult. It's much easier, if smaller groups of characters are shown, with space in between them.

As a result, users might try to enter the characters exactly like they see it displayed elsewhere.

I think we should do one of the following:
- either allow the user to enter any combination of spaces into the dialog, and ignore all whitespace when doing the verification
- or tell the user in an onscreen message that whitespace should be omitted when entering the fingerprint.


>+otr-add-finger-tooltip = No special character allowed, only letters (A to F) and numbers (0 to 9)
>+otr-add-finger-tooltip-error = Invalid character detected

Maybe not everyone detects the tooltip? How about simplifying and combining the hint with the information from the tooltip?
=> "Invalid characters detected. Valid are: abcdef0123456789"

Then everything would be visible on screen immediately.

(Add a comment in .ftl to not translate "abcdef0123456789".)

You could silently allow spaces, without mentioning they are allowed.

(Nit: maybe rename "otr-add-finger-tooltip-error" to "otr-add-finger-popup-error", because it's a popup, not tooltip.)

Comment 10

[Patrick Cloke [:clokep]]

(In reply to Kai Engert (:kaie:) from comment #9)

I think we should do one of the following:

• either allow the user to enter any combination of spaces into the dialog,
  and ignore all whitespace when doing the verification
• or tell the user in an onscreen message that whitespace should be omitted
  when entering the fingerprint.

Please strip spaces automatically.

Comment 11

[Patrick Cloke [:clokep]]

Florian mentioned to me on IRC that he had some feedback on this.

Comment 12

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2019-05-30 10-04-46.png

Comment 13

[Alessandro Castellani [:aleca]]

Attachment: otr-add-fingerprint.patch

Patch updated.

• I'm automatically stripping away all the blank spaces before validating and counting, easier for the user to paste or type the 40 char string.
• I re-positioned the elements to reduce the dialog size and remove extra strings.

(Nit: maybe rename "otr-add-finger-tooltip-error" to "otr-add-finger-popup-error", because it's a popup, not tooltip.)

I'd suggest we keep those as -tooltip- because the markup element is actually a <tooltip>. The method that triggers them in JS is openPopup()

Comment 14

[Kai Engert [:KaiE:]]

Comment on attachment 9068747 [details] [diff] [review]
otr-add-fingerprint.patch

Thanks, I like the key icon better. You're still including the fingerprint icon in the patch, should this be removed? (Or do you anticipate we'll need it later?)

The input field behavior seems very good to me. The dialog size is adjusted correctly for me.

The cancel button says "skip". Looking at bug 1518185 attachment 9035039 [details] it was set to "skip" in the previous code already. But I don't understand why.  Maybe we should use the standard "cancel" instead.

You are adding many new CSS files. You have 12 files {linux,osx,windows}/base/{colors,icons,inputs,tooltips} that each contain only a single @import line. And there aren't differences between platforms. Could you avoid adding these 12 files, and change otrFingerprintDialog.css to include the "shared" files directly? Or do you consider it very likely that you'll soon need individual platform specific code for other purposes?

>   l10nHelper,
> } = ChromeUtils.import("resource:///modules/imXPCOMUtils.jsm");
>-const {OTR} = ChromeUtils.import("resource:///modules/OTR.jsm");
>+const { OTR } = ChromeUtils.import("resource:///modules/OTR.jsm");

unnecessary change?


>diff --git a/mail/themes/osx/jar.mn b/mail/themes/osx/jar.mn
>--- a/mail/themes/osx/jar.mn
>+++ b/mail/themes/osx/jar.mn
>@@ -259,6 +259,12 @@
>   skin/classic/editor/icons/img-align-left.gif                   (editor/img-align-left.gif)
>   skin/classic/editor/icons/img-align-middle.gif                 (editor/img-align-middle.gif)
>   skin/classic/editor/icons/img-align-right.gif                  (editor/img-align-right.gif)
>-  skin/classic/editor/icons/img-align-top.gif                    (editor/img-align-top.gif)
>+  skin/classic/editor/icons/img-align-top.gif                    (edaitor/img-align-top.gif)

Typo in an unrelated line.

Comment 15

[Alessandro Castellani [:aleca]]

(In reply to Kai Engert (:kaie:) from comment #14)

Thanks, I like the key icon better. You're still including the fingerprint
icon in the patch, should this be removed? (Or do you anticipate we'll need
it later?)

It slipped away, sorry, I'll remove it.

The cancel button says "skip". Looking at bug 1518185 attachment 9035039 [details]
[details] it was set to "skip" in the previous code already. But I don't
understand why. Maybe we should use the standard "cancel" instead.

I agree with replacing it with "Cancel".

You are adding many new CSS files. You have 12 files
{linux,osx,windows}/base/{colors,icons,inputs,tooltips} ... Or do you consider it very likely
that you'll soon need individual platform specific code for other purposes?

This structure will be used by the new Account hub with some per-platform CSS variations. But you're right in suggesting to use only the shared as those variations won't be used soon. I got carried away in preparing files for the future.

I'll push an updated patch soon.

Comment 16

[Alessandro Castellani [:aleca]]

Attachment: otr-add-fingerprint.patch

Comment 17

[Alessandro Castellani [:aleca]]

Attachment: otr-add-fingerprint.patch

Sorry, I forgot to update the "Cancel" button string.

Comment 18

[Kai Engert [:KaiE:]]

Comment on attachment 9069035 [details] [diff] [review]
otr-add-fingerprint.patch

You can simply remove the line
    .buttonlabelcancel = Cancel
and it should use the same string from the default (one string less for localizers)

r+

Comment 19

[Alessandro Castellani [:aleca]]

Attachment: otr-add-fingerprint.patch

Right on.
Waiting for feedback from Magnus and Florian before marking it for check-in.

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 9069052 [details] [diff] [review]
otr-add-fingerprint.patch

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

::: chat/content/otr-add-fingerprint.js
@@ +34,5 @@
>  
>    oninput(e) {
> +    let hex = e.value.replace(/\s/g, "");
> +
> +    if ((/[^0-9a-fA-F]/gi).test(hex)) {

I think spaces should be allowed, and then you can trim them later.
It would actually be nice to use the spaces on input also: when the input changes, add the space where "needed" so that it shows as expected during input too whether the user inputs the spaces or not. That way it's clear to a user whether he is supposed to input space separators or not.

Maybe we should also just drop input (while entering) that is not HEX.

::: chat/modules/OTRUI.jsm
@@ +164,5 @@
>          let contact = target.contact;
>          let args = OTRUI.contactWrapper(contact);
>          args.wrappedJSObject = args;
> +        let features =
> +          "chrome,modal,centerscreen,resizable=no,minimizable=no";

why this change, the original looks < 80ch

Comment 21

[Kai Engert [:KaiE:]]

(In reply to Magnus Melin [:mkmelin] from comment #20)

Maybe we should also just drop input (while entering) that is not HEX.

Magnus, this is how the original implementation behaved, but Patrick didn't like it.

Comment 22

[Magnus Melin [:mkmelin]]

Ok, I was just thinking about the error message. It's pretty obscure. I'd imagine not even 1% of users could tell you what HEX is. Any non-HEX is not going to be accepted anyway, so why not just drop those typos?

Comment 23

[Magnus Melin [:mkmelin]]

Comment on attachment 9069052 [details] [diff] [review]
otr-add-fingerprint.patch

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

I think it would could be nice/preferable to also use standard form validation, like

<html:input id="fingerprint" name="fingerprint" required="required" minlength="40" pattern="[^0-9a-fA-F ]/" title="Fingerprint in HEX format (only letters ABCDEF and numbers allowed)" placeholder="Hexadecimal fingerprint of the OTR key for ${contact.name}" />

I'm not sure how it all fits into this bug. I'll just mention some issues with the dialog:
* the dialog title is super specific. Would usually be something like jut "Add Fingerprint"
* the text "If you know".... seems superflous, maybe we can remove it, and rely on the placeholder
* the context menu item includes the contact name, which seems odd or at least unusual

I do think the info signs etc. look very nice (the invalid message - the title - is not so nice though, and probably not workable as it is now, since it would be too long for some localizations), but I'd like to preferably avoid much custom code for showing the error messages, and rely on browser in-built functionality as much as possible. I think it should be fine to have the ok button always active, and on ok, the validation message pops up if you hadn't filled it all. 
We should also work with toolkit to improve how these things are handled if they do not fit our needs.

Comment 24

[Florian Quèze [:florian]]

(In reply to Patrick Cloke [:clokep] from comment #11)

Florian mentioned to me on IRC that he had some feedback on this.

I don't really remember, I think I mostly said that this looks fine if it's triggered by a user action (eg. a click on a button in the UI) but should never be put in the user's face as a reaction to something received on the network.

Comment 25

[Patrick Cloke [:clokep]]

(In reply to Magnus Melin [:mkmelin] from comment #22)

Ok, I was just thinking about the error message. It's pretty obscure. I'd imagine not even 1% of users could tell you what HEX is. Any non-HEX is not going to be accepted anyway, so why not just drop those typos?

Because it is a confusing user experience to just drop erroneous input arbitrarily. Spaces are not errors, they're expected in situations, but unnecessary for the calculation. I don't feel super strongly about this though.

Comment 26

[Alessandro Castellani [:aleca]]

(In reply to Magnus Melin [:mkmelin] from comment #20)

I think spaces should be allowed, and then you can trim them later.

White spaces are allowed in the field. The trim happens only to properly count the 40 cha for validation, without affecting what the user wrote.

It would actually be nice to use the spaces on input also: when the input
changes, add the space where "needed" so that it shows as expected during
input too whether the user inputs the spaces or not. That way it's clear to
a user whether he is supposed to input space separators or not.

Nice, I like it. I can implement it oninput and onblur and add a blank space once ever 10 cha.

Maybe we should also just drop input (while entering) that is not HEX.

I think we should avoid it.
Usually, preventing specific characters and drop them oninput is fine when handling short strings or number values.
In this case, we're dealing with a 40 char long string, and we can't assume the user will properly type it on the first try. Here's a couple of case scenarios:

1. The user manually types the fingerprint from a piece of paper and looks at the keyboard while doing it, consequentially not noticing that some characters are not accepted.
2. The user paste the fingerprint directly in the field and we strip away non hex characters. At that point it'll be hard to identify which character was removed, and even notice at first glance if the fingerprint is not 39 or 38 characters instead of 40.

    let features =
      "chrome,modal,centerscreen,resizable=no,minimizable=no";

why this change, the original looks < 80ch

Sorry, that's a leftover for when I was defining width and height.

I do think the info signs etc. look very nice (the invalid message - the title - is not so nice though, and probably not workable as it is now, since it would be too long for some localizations), but I'd like to preferably avoid much custom code for showing the error messages, and rely on browser in-built functionality as much as possible.

I agree in general with this approach, but there are some cases like this one where the browser built-in functionality is not enough, causing a bad experience, and risking to confuse the user.

I think it should be fine to have the ok button always active, and on ok, the validation message pops up if you hadn't filled it all.

I disagree with this. We shouldn't allow users to go down a path if that path is not available. That's extra work for us and a misleading and frustrating workflow for the user. Imagine if the user inputs 39 characters instead of 40, keeps clicking on OK, and we keep returning an error message. We're forcing the user to visually count the characters trying to identify where he missed one.

Enabling the OK button only when the input is valid removes that blocking experience. I think we should leverage a "smart" and adaptable tooltip to guide the user in properly typing a complex and long string.

We should also work with toolkit to improve how these things are handled if they do not fit our needs.

Totally agree with this, but in the meantime we should not accept a not so great user experience with the promise of some future improvements.

Thoughts?

Comment 27

[Kai Engert [:KaiE:]]

(In reply to Alessandro Castellani (:aleca) from comment #26)

Nice, I like it. I can implement it oninput and onblur and add a blank space once ever 10 cha.

Please make that "every 8 characters", that's what's used in other places.

Comment 28

[Alessandro Castellani [:aleca]]

Attachment: otr-add-fingerprint.patch

Here's an updated patch with all the suggestions and updates.

• The dialog's title is a simple "Add Fingerprint"
• The title inside the dialog has the contact's email to remind the user which contact he's adding the fingerprint to.
• The description is shorter and more direct.
• The input field has both title and placeholder text

Here's the recap of what happens on typing

• A blank space is added once every 8 characters
• Text is validated at every input, triggering an error message once an invalid character is detected.
• The tooltip with the error message remains visible until the error has been fixed.
• The OK button is disabled until the fingerprint is valid and 40 character in total, excluding blank spaces.

Comment 29

[Magnus Melin [:mkmelin]]

(In reply to Alessandro Castellani (:aleca) from comment #26)

In this case, we're dealing with a 40 char long string, and we can't assume
the user will properly type it on the first try. Here's a couple of case
scenarios:

1. The user manually types the fingerprint from a piece of paper and looks
   at the keyboard while doing it, consequentially not noticing that some
   characters are not accepted.

The current approach doesn't help if he's not looking, since it doesn't way what char. In any case, for both versions, the input is invalid and will need to be re-entered or examined.

2. The user paste the fingerprint directly in the field and we strip away
   non hex characters. At that point it'll be hard to identify which character
   was removed, and even notice at first glance if the fingerprint is not 39 or
   38 characters instead of 40.

If pasting something invalid, that's not something we can help with. He'll have to write a new valid input in there.
For length validation, you'd get that automatically with proper message for html5 input type validation.

I think it should be fine to have the ok button always active, and on ok, the validation message pops up if you hadn't filled it all.

I disagree with this. We shouldn't allow users to go down a path if that
path is not available. That's extra work for us and a misleading and
frustrating workflow for the user. Imagine if the user inputs 39 characters
instead of 40, keeps clicking on OK, and we keep returning an error message.
We're forcing the user to visually count the characters trying to identify
where he missed one.

Well how is he to know why the button can't be clicked? He might very well give up on the dialog since there is no error message shown anywhere in the UI and no way to go ahead. If you let him click the button, then an explanation is easily shown.

Comment 30

[Magnus Melin [:mkmelin]]

Comment on attachment 9069436 [details] [diff] [review]
otr-add-fingerprint.patch

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

I get this: (and title is undefined)
console.warn: "[fluent] Missing translations in en-US: otr-add-finger-dialog-title."

I think the label next to the input should maybe have a colon(?)

The text in the dialog is still repetative, and odd. Like
  Add aleca-otr-test@....'s OTR Fingerprint
  Enter the 40 character long hexadecimal fingerprint of your contact's OTR key.
In that there is already the contact name, and then we say "your contact" - which seems odd.

Maybe we can drop the Enter.... text. And have a placeholder for the fingerprint. Something like "The 40 character long OTR key fingerprint"

It doesn't seem to do the adding/removing of space on each keyup (if ever?). 

Please add minlength=40 and maxlength=44 (40 + 4 spaces each 8 chars)

Comment 31

[Alessandro Castellani [:aleca]]

Attachment: fingerprint-normal.png

Thanks for the feedback and review, here's the updated dialog with some additional changes.

For the dialog description, I merged together title and description to avoid the repetition and the odd feeling you pointed out. With this long description the dialog feels more balanced and plays well with the left icon.

I updated the placeholder, and added a : for the input label.

I'd like to try to convince you in keeping the OK button disabled until the input is valid.
You raised a good point about the user not knowing why the button is disabled, that's why I implemented a character count which updates at every input. That visual hint should remove any confusion and help the user in reviewing the length of the string. What do you think?

Also, and I'm sorry if this is a stupid question, isn't the HTML input validation triggered only on form submit? We're not submitting a form here, so I'm not sure if that would work.

If you still think the button should always be enabled, I will do that and take care of the validation and error messages.

More screenshots for the different states and a patch are coming.

Comment 32

[Alessandro Castellani [:aleca]]

Attachment: fingerprint-error.png

Comment 33

[Alessandro Castellani [:aleca]]

Attachment: fingerprint-complete.png

Comment 34

[Alessandro Castellani [:aleca]]

Attachment: otr-add-fingerprint.patch

Comment 35

[Kai Engert [:KaiE:]]

FYI, after last night's OTR commits: If you edit your patch file, and remove
type="application/javascript"
the patch applies.

Comment 36

[Kai Engert [:KaiE:]]

Comment on attachment 9069721 [details] [diff] [review]
otr-add-fingerprint.patch

>+otr-add-finger-input =
>+    .title = HEX format (only letters ABCDEF and numbers allowed)

Is this ever shown? If not, remove.
If yes, please change HEX to hexadecimal.


You have three places that say 
  "Only letters ABCDEF and numbers allowed"

Should we make it a full sentence and add "are"?
  "Only letters ABCDEF and numbers are allowed"


To avoid multiple translation of the same text, you can self-reference text in ftl files like this:

otr-text-only-hex = Only letters ABCDEF and numbers allowed

+otr-add-finger-tooltip = { otr-text-only-hex }
+otr-add-finger-tooltip-error = Invalid character. { otr-text-only-hex }
+otr-add-finger-input =
+    .title = HEX format ({ otr-text-only-hex })
+    .placeholder = The 40 character long OTR key fingerprint


Other than this detail, the patch and dialog behavior looks good to me.
r+

Comment 37

[Alessandro Castellani [:aleca]]

Attachment: otr-add-fingerprint.patch

Thanks Kai for the feedback, I updated the patch.
Let's wait for the green light from Magnus

Comment 38

[Magnus Melin [:mkmelin]]

Comment on attachment 9070092 [details] [diff] [review]
otr-add-fingerprint.patch

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

::: chat/content/otr-add-fingerprint.xul
@@ +40,5 @@
> +             class="label-box"
> +             control="fingerprint"/>
> +      <hbox class="input-control" align="center" flex="1">
> +        <html:input id="fingerprint"
> +                    name="fingerprint"

name used for anything?

@@ +63,5 @@
> +      <tooltip id="fingerError"
> +               position="before_end"
> +               class="tooltip-warning">
> +        <description data-l10n-id="otr-add-finger-tooltip-error"/>
> +      </tooltip>

I thought a lot on this, but I do think we want to just use standard html5 form validation.
Like I wrote in bug 1552227, the info is difficult for accessibility and localization. 

I don't care much if you enable or disable the ok button until the full length is reached.

I didn't check now, but I think the basic forms of validation work (so you get the red borders). 

If you think it's needed, you can use custom validation message too for when there's something wrong. https://developer.mozilla.org/en-US/docs/Learn/HTML/Forms/Form_validation#Customized_error_messages.
Hmm, I notice bug 1562198 now... :/

Comment 39

[Alessandro Castellani [:aleca]]

Attachment: otr-add-fingerprint.patch

Here's an updated patch.

Like I wrote in bug 1551133, the HTML5 form validation doesn't currently work in Thunderbird as expected. No error messages are being visualized and the validation needs to be triggered in JS in order to make sense, otherwise it renders the field invalid at first keystroke, or when you dismiss the dialog.

The current patch uses some built-in HTML5 attributes for validation, alongside JS error handling.
The icons now use tooltiptext which is accessible.
The error is visualized underneath the input field if a wrong character is typed.

All the style is self contained into a single CSS, and the UX has been greatly improved thanks to the word counter.

We could revisit this once the HTML5 native form validation features are usable.
(Screenshot to follow)

Comment 40

[Alessandro Castellani [:aleca]]

Attachment: otr-fingerprint-3steps

Comment 41

[Kai Engert [:KaiE:]]

Comment on attachment 9076127 [details] [diff] [review]
otr-add-fingerprint.patch

I'd like to remove myself from reviewing the UI fine tuning.

Comment 42

[Alessandro Castellani [:aleca]]

Sorry Kai, I won't request further UI reviews for this patch.
Magnus, this one doesn't need rebasing as it applies normally to the current trunk.

Comment 43

[Magnus Melin [:mkmelin]]

Comment on attachment 9076127 [details] [diff] [review]
otr-add-fingerprint.patch

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

I've made the below changes, will submit a patch.

::: chat/content/otr-add-fingerprint.js
@@ +41,3 @@
>    },
>  
>    oninput(e) {

e seems like an unusual variable name here, since it's not the event (but presumingly e for element). Maybe name it input?

@@ +43,5 @@
>    oninput(e) {
> +    let hex = e.value.replace(/\s/g, "");
> +
> +    if ((/[^0-9a-fA-F]/gi).test(hex)) {
> +      this.fingerValid = false;

we don't need this either, we can just check input.validity.valid

@@ +48,5 @@
> +      this.fingerInfo.hidden = true;
> +      this.keyCount.hidden = true;
> +      this.fingerWarning.hidden = false;
> +      this.fingerError.hidden = false;
> +      e.setAttribute("error", "true");

no need to set this, just let the build in validation handle it with :invalid

@@ +73,3 @@
>    },
>  
>    add(e) {

the e is not used, and looks like the whole function should be inlined

::: chat/content/otr-add-fingerprint.xul
@@ +40,5 @@
> +             class="label-box"
> +             control="fingerprint"/>
> +      <hbox class="input-control" align="center" flex="1">
> +        <html:input type="text"
> +                    id="fingerprint"

id first please

@@ +45,5 @@
> +                    data-l10n-id="otr-add-finger-input"
> +                    class="input-field"
> +                    oninput="otrAddFinger.oninput(this);"
> +                    onblur="otrAddFinger.onblur(this);"
> +                    minlength="40"

44, due to the spaces (otherwise the element is not properly :invalid when it should be)

::: chat/content/otr/add-finger.ftl
@@ +14,3 @@
>  
> +otr-text-only-hex = Only letters ABCDEF and numbers are allowed
> +otr-add-finger-tooltip = { otr-text-only-hex }

I think the information icon should be removed. It's not very useful but only adds a chore for the user to go looking. It's not like they should be inputting something they figure out by themselves, but should be copy pasting a string from somewhere else. Oh, the paste context menu is missing.

@@ +14,4 @@
>  
> +otr-text-only-hex = Only letters ABCDEF and numbers are allowed
> +otr-add-finger-tooltip = { otr-text-only-hex }
> +otr-add-finger-tooltip-error = Invalid character. { otr-text-only-hex }

Invalid character entered.

Comment 44

[Magnus Melin [:mkmelin]]

Attachment: otr-add-fingerprint.patch

Comment 45

[Alessandro Castellani [:aleca]]

Comment on attachment 9079263 [details] [diff] [review]
otr-add-fingerprint.patch

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

Perfect, thanks for updating the patch.

Comment 46

[Alessandro Castellani [:aleca]]

Kai, does this need to be uplifted to beta or esr68?

Comment 47

[Kai Engert [:KaiE:]]

Comment on attachment 9079263 [details] [diff] [review]
otr-add-fingerprint.patch

Yes, all OTR improvements should go to esr68
(As long as they don't cause risk for other chat functionality.)

Comment 48

[Jorg K (CEST = GMT+2)]

Comment on attachment 9079263 [details] [diff] [review]
otr-add-fingerprint.patch

We're back to normal here, beta and ESR. I guess I'll ship this without a ride on the beta train first, right?

Comment 49

[Alessandro Castellani [:aleca]]

Yes, please.
This patch has mostly minor UX and UI changes, not affecting any chat functionality.

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4a0cb4b952fa
OTR: improve UX of 'Add Finderprint' dialog. r=mkmelin,kaie

Comment 51

[Jorg K (CEST = GMT+2)]

TB 69 beta:
https://hg.mozilla.org/releases/comm-beta/rev/fa74c9e9b21e6cdce902e8ab0cb2fb6d57479a3a

Comment 52

[Jorg K (CEST = GMT+2)]

TB 68.0 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/2671a7fcbaeffd2bd80aa3a5af6260768a5e831b