We need to add some extra actions to improve the usability of the OpenPGP Area we just implemented in bug 1627736.

These are the actions each key should show when the details dropdown is opened:

• Copy Public Key
• Send Public Key via email
• Export Key to file

General action external to any key:

• Reload key cache not necessary
• Only show valid keys discarded

For the Key Wizard I will implement the Key Import section:

• Import Public/Secret key from File
• Import key from URL
• Import key from Clipboard follow up on bug 1650235

For the Key Management Dialog, I will update the UI:

• Open as a SubDialog
• Add the AccountManager.css for visual consistency
• Remove the Menu Bar, integrate buttons specific for each key, and redirect the user to the Account Manager in case of importing/adding a new key. on hold as the Key manager is not an immediate priority and changes may occur

Does this look extremely busy?

Showing type is unnecessary. We're only offering key pairs for selection.

Are you trying to make the key manager completely unnecessary? That won't work, because this view here is only for secret keys, not for public keys, so you cannot see all keys here. Which means, you still need the key manager to view other keys. I'd keep anything that isn't related to "set my personal key" out of this area.

my feeling is:

change expiration - keep
type - remove
copy public key - remove
send public by email - remove
export key to file - it isn't clear what you'd do here, export public key for sharing, or exporting secret key as a backup? I'd remove that, and have that in key manager, only
delete key - remove
revoke key - may be, I'm undecided, but we don't want people to click that lightly
edit key - what key property will that edit? some editing is done from inside the "key properties" dialog already

Are you trying to make the key manager completely unnecessary?

Not at all.
As you said this area lists only the personal keys, and offering quick actions for those keys is important.
The user is looking at his keys, he wants to copy info, send a public key via email, etc, he should be able to do it directly from here, instead of asking the user to open the Key manager, look for his key, and discover a context menu to do those operations. That's not great.

change expiration - keep
type - remove

Wouldn't this info change in case of an external key created with GnuPG?

copy public key - remove
send public by email - remove
export key to file - it isn't clear what you'd do here, export public key for sharing, or exporting secret key as a backup? I'd remove that, and have that in key manager, only

I recommend to offer these options here for the reasons I listed above.
In terms of what's the purpose of "Exporting a key to file", we don't have to tell the user the reasons why he should/shouldn't do it. It's a useful option and we should offer it and make it easily discoverable.

delete key - remove

Why? We always ask for confirmation before proceeding, and again, we shouldn't hide these options behind multiple clicks if the user is already interacting with his key.

revoke key - may be, I'm undecided, but we don't want people to click that lightly

Same reasons as above.

edit key - what key property will that edit? some editing is done from inside the "key properties" dialog already

Sounds good.

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

type - remove

Wouldn't this info change in case of an external key created with GnuPG?

Even the key is stored/managed externally, it's still of type "personal key (secret+public)".
The storage location doesn't change its type.

copy public key - remove
send public by email - remove
export key to file - it isn't clear what you'd do here, export public key for sharing, or exporting secret key as a backup? I'd remove that, and have that in key manager, only

I recommend to offer these options here for the reasons I listed above.

Maybe you could have a "share public key" button, that pop ups a list of the ways to share it?
That pop up menu would offer "copy to clipboard, send by email, or save as file".

If you reduce the display to one share button, you have more space for a "backup" button, which I'd consider very important to have.

delete key - remove

Why?

It's a very rare event that you'd ever want to delete a personal key. If you've created it, you've probably used it. And if you've used it, it means you probably have some messages in your archive that are encrypted with it. And if you delete it, you'll no longer be able to read them.

I cannot offer a strong reason to remove the "delete" button. But the only scenario I can think of, if you have accidentally created an extra key, and you're certain that deleting it won't give you problems.

revoke key - may be, I'm undecided, but we don't want people to click that lightly

Same reasons as above.

ok, let's keep it here.

From my user perspective, this mock-up has too many buttons. As Kai proposed, put the "copy...", "send..." and "export..." buttons in one "Share Key" pop-up where now is the "Edit" button. The "Edit" button is not needed, because edits are done in the "Key Properties" dialog (maybe rename it to "Edit Key Properties"?).
In bug 1627736 you shared a mock-up (tab.png) where you have the information that the key comes from an external source in a pill right behind the key ID. If you implement this, you don't need the "Type" line to cover this information.

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

For the Key Wizard I will implement the Key Import section:

• Import Public/Secret key from File
• Import key from URL
• Import key from Clipboard

One comment on this: the latest daily update (TB 79.0a1 2020-06-26) requires the user to verify if a secret key is indeed his own personal key. Guide the user towards this information right after an import of a secret key.
"You just imported a secret key. You have to accept it as your personal key in order to use it. Want to do that now? (Otherwise do it later in the key manager)"

Thank you all for the insightful feedback.
Here's an updated screenshot of the working UI.

Since the possible actions may vary based on the key status, we can prevent the UI from moving around too much by grouping all those actions inside a popup menu.

When clicking on the More button, a menu will open with the following options:

• Copy Public Key
• Send Key via email
• separator
• Export key to file (I'd avoid using terms like "backup" as it might imply that Thunderbird offers a built-in backup option for it)
• separator
• Revoke key
• Delete key

Putting these actions inside that dropdown will help us prevent accidental clicks (even tho we always ask for confirmation before proceeding on any of those actions), clean up the UI, and organize all those actions that the user might need to use, but not on a daily basis.

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

• Export key to file (I'd avoid using terms like "backup" as it might imply that Thunderbird offers a built-in backup option for it)

We need to offer the user two different actions:

• export the public part of your personal key, only, intended for sharing it with other personal
• export the complete personal key (including the secret key), intended to backup on external media, to ensure you can restore it when necessary - to ensure you won't lose access to your archive of encrypted messages - or - to transfer it to a second computer you own, so you can read/decrypt received encrypted messages on both computers the user owns.

It's important to say what you are exporting, public or personal/secret. The personal/secret key must never be sent to another person. We have seen in the past that some users make this mistake. The UI for exporting should make it very clear what the user is exporting, and ideally even remind the user how to treat that file, to avoid fatal mistakes.

Indeed, thanks for the heads up.
I didn't include any further step as I see we have a dialog with this initial distinction when selecting the Export option in the current Key Manager.

I was planning to iterate upon that section later.

That dialog might not be easy to understand.
Maybe we could have something like this:
Export public key for sharing it with others.
Export secret key as a backup for yourself, to store it on external backup media.

For sure, that dialog will not be used.
I will explore the possibility of multiple menu items, or even a submenu to handle those exporting options

It could be better to have Export and Backup separate: "Export public key," and "Backup secret key"

This patch takes care of the following things:

• Improve the UI of the OpenPGP section
• Implement checkbox to show/hide expired keys
• Make expired keys unselectable
• Implement menupopup to copy, export, send, revoke, and delete key
• Reload key cache button

I'll work on the KeyWizard missing feature in a follow up patch to not make this one too big to review.

Also, I'll work on the improvements to the Key Manager in a dedicated bug.

Why do we need a reload button? Can't we do that automatically if/when needed?

There's something wrong with the "If you have an existing key..." - that text is now one one line and not wrapping (going outside of the dialog/hidden). Didn't check if it's this bug or not.

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

Why do we need a reload button? Can't we do that automatically if/when needed?

We have several data layers.
There's the raw key storage in RNP, which is a persistent store of OpenPGP keys.
Then we have in-memory data structures. keyRing.jsm loads the RNP keys, queries their various attributes, and keeps them in JS lists of objects and attributes.
This may be slow.

Most of the code tries to operate on the cached information.
If the underlying RNP keys are modified/deleted/imported, it's necessary to recreate the cache.

Currently there isn't any notifier/observer architecture for that data.
If UI 1 shows information about a specifc key, and a different UI 2 modifies the key, then UI 1 will not notice/update.

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

Why do we need a reload button? Can't we do that automatically if/when needed?

We have the following choices:
(a) if a change to keys is made in a different dialog (e.g. the openpgp key manager), then accept that the display doesn't refresh automatically. Require the user to close prefs and reopen them, to see the new changes
(b) identify all common scenarios for concurrent modifications, and implement cooperative notifications, to allow the prefs UI to learn about the requirement to update the displayed information
(c) introduce a clean mechanism, where all dialogs register modification observers for the currently shown information

Doing (c) is a lot of work.

Alessandro, did you ever run yourself into a scenario that made reloading the key cache necessary? In other words, do you have examples for (b) ? If yes, we could try to identify manual notifications for those.

Alessandro might have added this action, because it is already available as a menu command in the key manager.

Feedback on the patch:

You've decided to hide expired keys by default.

I see two problems with that.

First, what if the currently configured key has expired? The user won't see it. The user might not understand why their currently configured key isn't shown.

Suggestion:
Always show the currently configured key, even if the key has expired, and even if the checkbox is off.

Additional comment on the checkbox:

Currently is uses the text "Display invalid keys". I suggest to change that to "Display expired keys". There are additional reasons why a key might be invalid, for example, a key might be revoked. The query you are using will only give you "keys that are invalid because they are expired", not keys that are invalid for other reasons.

Another comment on "Reload Key Cache".

It's unfortunate that the button is so prominently shown.
Most users will have no idea what a key cache is, or why they would need to reload it.
It probably shouldn't be the first button.
You could make it the second button and rename it to: "Refresh list of keys".

(If preferences were simply a short-lived dialog (as it was in the past) then we wouldn't have this problem. Because if you close and open the preferences again, you'll see the updated data. But we can't go back to the past...)

Comment 18 and 19 are just general thoughts and responses to the discussion.

Comment 20 and 21 are suggestions for changes to the patch.

I've tested the following scenario:

• the currently configured key has expired in the meantim (you have configured it in the past at a time it was still valid)
• open e2e prefs

Displayed are options "None" and another test key I have.
No entry is currently selected.
The error console shows:

Uncaught (in promise) TypeError: document.querySelector(...) is null
highlightOpenPgpKey chrome://messenger/content/am-e2e.js:1069
reloadOpenPgpUI chrome://messenger/content/am-e2e.js:894

Alessandro, if you don't have an expired key yet for testing, you could create a new test key with an expiration of one day. Then you'll have an expired key tomorrow.

Alessandro, did you ever run yourself into a scenario that made reloading the key cache necessary? In other words, do you have examples for (b) ? If yes, we could try to identify manual notifications for those.

I did before, but I think I was able to cover all of the scenarios I encountered.
The key list reloads when accessing the page, and after a new key is created in the new Key Wizard.
I'm okay with removing that button and keep an eye on covering all the cases where a reload is needed (eg. expiration changed, key modified by another dialog, etc.)

Alessandro might have added this action, because it is already available as a menu command in the key manager.

That was my main reason for adding that button.

You've decided to hide expired keys by default.
I see two problems with that.

Sounds good, I'll set that pref as TRUE by default.

Suggestion:
Always show the currently configured key, even if the key has expired, and even if the checkbox is off.

That might be tricky as the hide/show is not done only in the UI but I'm actually loading the keys already filtered to avoid fetching data we don't need.
I have a possible solution, since the currently selected key is showed in the initial description, with a green checkmark, I can update that part of the UI with a warning icon/message and a quick button to change the expiration date.

Alessandro, if you don't have an expired key yet for testing, you could create a new test key with an expiration of one day. Then you'll have an expired key tomorrow.

I do and I missed this scenario, thanks for the heads up, I'll take care of it.

There's something wrong with the "If you have an existing key..." - that text is now one one line and not wrapping (going outside of the dialog/hidden). Didn't check if it's this bug or not.

I'm seeing this now for the first time, with or without this patch.
It must be a regression caused by something else.

Another idea is, if the currently selected key has expired, then automatically check the checkbox for showing expired keys.

I think the broken "Add Key" dialog was introduced with yesterday's daily release. Could it be related to the TB 80 branch?
It's not just the text, but the buttons are also missing.

The description XUL doesn't seem to properly wrap the text inside subdialogs anymore, pushing the content outside the visible area.

Could be from Fluent, but 1638822 comment 125

No, it comes from bug 1573678. Remove the lines https://searchfox.org/comm-central/rev/1ff29ab2c24eacd3f496aa3333b425d98c6ccaf5/mail/extensions/openpgp/content/ui/keyWizard.xhtml#22 and https://searchfox.org/comm-central/rev/1ff29ab2c24eacd3f496aa3333b425d98c6ccaf5/mail/extensions/openpgp/content/ui/keyWizard.xhtml#205 and it works again.

Thanks Richard, that worked.

Patch updated

Yeah I like having the More button instead so it's not so crammed.

Not sure I see the reason to have a "Display expired keys" checkbox. If there are expired keys seems better to just have them always listed.

Backup secret key doesn't work - ah, not implemented yet. Maybe it should just alert about that.

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

Yeah I like having the More button instead so it's not so crammed.

Nice, I like this solution as well.

Not sure I see the reason to have a "Display expired keys" checkbox. If there are expired keys seems better to just have them always listed.

I'm okay with not adding that.
Can you give your feedback on the requirements listed on bug 1647355? Those are partially related to this.

Backup secret key doesn't work - ah, not implemented yet. Maybe it should just alert about that.

I'll add an alert with non translatable strings for now.

Looks good for me. Let's see what the encryption expert say.

Thanks Richard for the ui-r+

Patch updated with:

• Do not implement the "Display expired keys"
• Alert the user of WIP backup feature
• Throw error on missing TEMP dir

Please change "Export Key to File" to "Export Public Key to File" - for consistency with the neighbouring commands, all of which clarify about which part of the key the action is about.

You might consider to move the separator. Have the first three items in one group - because all of them are actions on a public key.
And have the "backup" in a separate group.

Other than that, it looks good to me.

Patch updated

Pushed by alessandro@thunderbird.net:
https://hg.mozilla.org/comm-central/rev/6c2a85c521e2
Add missing actions to the OpenPGP wizard and section in the Account Manager. r=mkmelin, ui-r=paenglab

Thanks, the UI looks very clean. Just one small issue: the wrench icon of the "key properties" button doesn't change its color in dark mode.

I'll use this bug to let all together for the uplifts.

Fixed the icon colour and made the menu look better integrated to the in-content styling.

I notice I get "Your current configuration used Key ID 0xnull" when none of the keys available is in use.

Mh? So you have a key set to be used, but is not part of the list? Was it deleted?
I'm trying to simulate this scenario

I'll check... it's a testing account with no real usage.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/594d4c625b3f
Small style fixes for the OpenPGP section in the Account Manager. r=aleca DONTBUILD

My issue was that somehow the string "null" had got into the prefs. Don't know why.
Investigating this, found some inconsistencies in which function is responsible to set the string - it was often set twice. This patch makes things more clear.

Ah, I had fixed it in the html, but not in the js.

Updating the patch with the typo fix so we can land since Magnus is PTO.
Yay teamwork!

Ah, you're here :D
Sorry, I removed my patch.

Thunderbird 79.0b1:
https://hg.mozilla.org/releases/comm-beta/rev/754c7ad69ea9
https://hg.mozilla.org/releases/comm-beta/rev/90ac04e9d3f7
https://hg.mozilla.org/releases/comm-beta/rev/ea406f45aecb

Thunderbird 78.0.0 (build2):
https://hg.mozilla.org/releases/comm-esr78/rev/7d9b03748343
Thunderbird 78.0.0 (build2):
https://hg.mozilla.org/releases/comm-esr78/rev/ef019609987c
https://hg.mozilla.org/releases/comm-esr78/rev/6ced99ac67c0