We need to do a round of improvements in the Message Pane when dealing with the OpenPGP implementation.

This includes:

• Encrypted/Signed status
• Encrypted/Signed details
• Use SVG Photon icons
• Key discovery notification
• Import attached key (is this possible?)

Am I missing anything else?
Mock-ups coming soon.

Enigmail allows to set a custom column in the Message List to show if a message is encrypted/signed. While this is not exactly related to the Message Pane, I suggest it as an enhancement in this bug.

Thank you Andreas for the suggestion.
I'll open a dedicated bug to implement a dedicated Thread column.

Uploading the current screenshot to highlight the problem of the current implementation.

Other than using notification bars not properly styled, too often might happen that the user sees multiple notifications at same time.
If we add to this also a possible calendar notification, and attachment bottom bar, we end up with an extremely cramped and hard to read message pane.

The proposal is to defer all the OpenPGP notifications to a single alert/button, which will open a dedicated sidebar to allow the user to interact when needed, without stealing space from the message body.

Proposal

Relocate all the notifications inside a sliding panel that opens when the user clicks on the encryption status. (refer to bug 1624676 for the proposed style and iconography).
This sliding panel will replace the Message Security popup.

We can show a less intrusive counter on top of the encryption status button to reflect the "warnings" that have been triggered by this message.

Moving the notifications inside that panel allows us to not to worry about space and amount of warnings. We will also help the user to build the muscle memory of clicking on the encryption button whenever it needs to interact with it.

The grey bars are just placeholders for the content we currently show in the Message Security dialog.
We can also use the occasion to add a button that redirects to the Account Settings e2e page, or the Key Manager.

(In reply to Richard Marti (:Paenglab) from comment #7)

I'm not sure we should show both info boxes, as for the user both end in
importing the key. It should be combined with only one button.

Please don't require us to optimize the combination of buttons at this time. Please accept that we might show two notifications at the same time. We have multiple different kinds of notifications, and it's difficult to reorganize them all into a combined notification workflow. Also, different buttons trigger different actions. Import will import what's contained inside the message, Discover will search online.

Just a quick update to show you the progress with this bug.
It's taking me longer than expected due to a lot of code decoupling from the various dialogs used, and the ability to make that section easily reusable between OpenPGP and S/MIME.

The styling is incomplete, as you can see the content doesn't look great, but all the important functionalities have been ported and the section behaves as expected, now it's time to improve the UI and add the inline notifications inside that panel.

I can also implement the ability to refresh the icons and panel content in case the signature acceptance changes.

I think a sliding panel is the way to go compared to a sidebar (like the contacts sidebar in the compose), as the user doesn't need to interact with both sections at once, and it allows us to implement some nice responsiveness in case the message window is narrow.

To be honest, it looks strange to click on a button and then a panel opens on top of this button. Actually, I would prefer if the panel opens below the button just like the hamburger menu ≡ does.
I agree that this panel can partially hide the mail content when it is open.

And how about using an arrow popup like we use to edit an address in the from/to addresses star in the header. This would the info directly connect to the button.

Oh, the "View signer key" button feels misaligned now in this new inline view.

This breaks stand-alone message windows

Ah, I missed that window, sorry. I fixed it.

I'm not sure we want to have the "counter" bubble. It's a bit messy, and also creates a chore for the user.

The intention was to remove all the multiple notification bars a user might see in a single message, but still maintaining a bit of a "heads up" for the user to highlight that an alert is present.
I'd be okay with removing it, but are we sure all those warnings can be simply moved inside the new pane without any UI warning to the user?
Do you think the iconography is good enough for that purpose?

I agree with Richard, an arrow popup could be nicer.

I'll create a variant patch with the arrow popup so we can see both in action and pick which one feels more natural.
I'm worried that the arrow popup might not fit our needs since there's a lot of content we need to show.

I think "common" message security function needs to live somewhere other than under extensions/smime

Indeed, a deep and careful folder structure update will be necessary once these leftover bugs have landed.

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

I'd be okay with removing it, but are we sure all those warnings can be simply moved inside the new pane without any UI warning to the user?
Do you think the iconography is good enough for that purpose?

It's basically just informational notices (like import, discover etc.), so I think it should be fine. In any case, showing the number wouldn't make much difference.

This patch maintains the sliding panel approach.

I removed the notification counter, put the "View Signature" button inline, fixed the close button, and replaced the collapsed attributes with hidden.

I'll work on a patch variation, to be added on top of this, to have the panel as an arrow popup.

Comment on attachment 9175932 [details] [diff] [review]
1647039-openpgp-message-pane.diff

Looks good except on tall windows there where is a big blank area below the information in the panel.

I'm interested how it looks with an arrow panel.

Here's the variation with the arrow popup panel, patch to be applied on top of the previous one.

Comment on attachment 9176116 [details] [diff] [review]
1647039-arrowpopup.diff

I like it more than the pane solution.

I'm not sure if we want to let the arrow panel open when we click one of the buttons inside of it.

Sounds good.
I think I like it too as an arrow popup panel.
I'll merge the two patches and fix the reported nits.

Patch updated.
Richard, I'm not able to change the background colour of the panel content, any hints on that?

Please update S/MIME to also show info the same way.
Yes, the S/MIME info should also open in the arrow panel.

I was planning to do that in a follow up bug to not make this one too big, but that's okay, I'll update also the S/MIME section.

Maybe we should add an "-" in between. (This should really be done differently altogether...)

Indeed, that approach is pretty hacky but if we want this to land on 78 I can't touch any of those strings.

Richard:

This rule doesn't apply. Needed? I don't think.

I was trying to change the default colors of the popup panel, like I wrote in comment 24: "Richard, I'm not able to change the background colour of the panel content, any hints on that?".

I won't touch that and leave it as it is.

All right, this was a doozie.

For an unsigned email with only an Autocrypt header, there's no way to see/access that now to import that key - because the OpenPGP button is not shown.

Since we might stumble upon messages with no encryption technology used, we can't show the encryption button with an inaccurate label, so I'm leveraging the built-in notification system only for this case.

Please, be sure to test any possible scenario and combination between the two types of encryption, and let me know if I missed something as handling the same UI for both OpenPGP and S/MIME was a bit tricky.

Sorry, I already had the key of that user - that's why the Autocrypt import didn't show.

That said, we shouldn't do the (fairly disruptive) inline notification for an Autocrypt header. Only add the OpenPGP button and make import accessible from there.

(On a side note, surprising that importing from Autocrypt headers calls importAttachedSenderKey - would assume that is attached keys.)

Sorry, I already had the key of that user - that's why the Autocrypt import didn't show.

Yeah, I had the same thing and I was confused for a second.

I'm not sure what you meant, but Autocrypt is a OpenPGP only thing, so there is no "technology confusion".

Ah, I didn't know that, thanks for the explanation. Indeed, showing the OpenPGP button will make sense.

The "bubble" that opens is usually far too small so I get scrollbars.

I could set a min-height for the content, but that might be risky in some cases as we should let the popup panel adapt in height based on screen real estate.

If opening in the standalone message window, something is off with the styling: horizontal scrollbars and some of the text pushed out of view.

I can't reproduce this issue, sorry. Would you be able to share a screenshot?
Richard, can you reproduce this issue?

On Windows I have only a vertical scrollbar with all needed content visible. Only the encryption key isn't visible.

But now I found an old S/MIME mail which shows also a horizontal scrollbar, probably because of the long issuer name "StartCom Class 1 Primary Intermediate Client CA". See screenshot.

On Linux I have a mail with not imported key. In the tab I have also only one scrollbar but in stand-alone window also a horizontal scrollbar. Thanks to the not imported key and no light blue background I see why: you haven't imported inlineNotification.css in messageWindow.xhtml.

Ah, good catch, thanks.

All right, this should do it.
In order to allow labels and descriptions to wrap text naturally, I had to use textContent instead of value.
I also fixed an issue when multiple keys are detected in the message and listed in the panel, and also set a max height with scrollable overflow for the list of keys, just in case the container grows too tall.

Comment on attachment 9176963 [details] [diff] [review]
1647039-openpgp-message-pane.diff

Great, no scrollbars any more.

Since the security info no longer opens a dialog, but the bubble, the way it works is slightly strange in general.

Ah damn. Funny enough, this would not be an issue if we had stayed with the sliding panel :D

I think we can leave that for now, possibly remove that menu item in the future.

I think we should remove it right now.
That menu item is pretty useless since we shouldn't show security info if a message doesn't have neither encryption nor signature.
It's a common UX approach to not show actionable items to represent a default state. In our case "default" is non encrypted and non signed.

We should also remove it since that's not really discoverable and we moved the focal point to the message itself with a dynamic button.

for non-secure messges, the separator will have to be dealt with since techLabel is not set.

I can manually add that separator when we populate the techLabel since the only strings we add are OpenPGP and S/MIME and those are not translated.

I fixed the issue of the separator in the title and removed the View Message Security menuitem since it doesn't makes sense having it with this implementation.

I also launched a try-run to be sure I didn't break anything with those removals: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a4bb552ebbe8e9966bde00577430ec1a138041ab

Uh, the try run is busted due to a duplicate msgCompSecurityInfo.css.
So strange, I didn't touch the dupes file.
Ah, maybe is the msgReadSecurityInfo.css removal that is still listed in the suite/ dupes

Let's give it another try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=71749f86bf132c557bdcc4e36694e08171530b79

Mh, I'm a bit stuck on this.
No errors are reported when I locally run ./mach package.
It's strange that the error log reports that the msgCompSecurityInfo.css file is a duplicate as that file is listed in the suite/installer/allowed-dupes.mn file.

All right, I should have fixed everything by removing unnecessary dupes.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a038cc07ec3cf662f8c6f09f3efb142a5057de40

Maybe it would be better if I actually did add the newly created file.
And another try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4a7db4ce41c663d64be536de505b96b9bf3d95e6

BTW, ./mach package always runs locally and never returns an error for me.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/c8edbf65b666
[OpenPGP] Improve the UI and UX of the notification in the Message Pane. r=mkmelin, ui-r=Paenglab

Looks very good.
Just one thing: the pink warning that no decryption key is available is hard to read in dark mode because of poor contrast. Would it be possible to use a darker background color in dark mode or keep the font black in this bubble?

Welp, that's definitely a mistake, that should be dark red and have AAA readability contrast.
I suspect this issue affects all those inline notifications.
I'll fix it.

Comment on attachment 9177539 [details] [diff] [review]
1647039-openpgp-message-pane.diff[LANDED ON C-C]

[Approval Request Comment]
Regression caused by (bug #): -
User impact if declined: Not a great UI or experience when viewing encryption information due to the usage of dialogs popup and a hard to read list of included keys.
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low as it mostly tackles UI updates and doesn't touch any OpenPGP core functionality.

Comment on attachment 9177539 [details] [diff] [review]
1647039-openpgp-message-pane.diff[LANDED ON C-C]

[Approval Request Comment]
Regression caused by (bug #): -
User impact if declined: Not a great UI or experience when viewing encryption information due to the usage of dialogs popup and a hard to read list of included keys.
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low as it mostly tackles UI updates and doesn't touch any OpenPGP core functionality.

The patch doesn't apply to comm-esr78 as is. A manually merged version of the patch will be required.

I think this is not yet ready to ask for esr78 approval.

Initial merge attempt.

I'm seeing some regressions. I'll add separate comments. I think these need to get address, prior to uplifting.

When reading an S/MIME message that cannot be decrypted I get:
Uncaught TypeError: label is null
loadSmimeMessageSecurityInfo chrome://messenger-smime/content/msgReadSMIMEOverlay.js:256
showMessageReadSecurityInfo chrome://messenger-smime/content/msgReadSMIMEOverlay.js:83
onclick chrome://messenger/content/messenger.xhtml:1

Element encryptionHeader no longer exists.

One of the features that distinguishes us from other OpenPGP implementations, we show a warning if a message contains a new, unexpected key (you already have accepted a correspondent's key, but a new different key arriving).

In this scenario, prior to this patch, we'd show a notification bar with string ID openpgp-be-careful-new-key

I'm seeing this with a message that triggers multiple notifications, "cannot decrypt", "claims to contain sender's key".
Do you limit the number of notifications you show?

I have a message that is sent by myself, signed with my own key, encrypted to myself.

The old info dialog showed "good sig, signed key id, button view signer key - is encrypted, your decryption key", view your decryption key". That's all.

However, the new implementation additionaly says at the bottom "message was encrypted to the owners of the following keys". No additional keys are shown, only this message. The message should not be shown, because the message wasn't encrypted to additional keys.

(In reply to Kai Engert (:KaiE:) from comment #57)

One of the features that distinguishes us from other OpenPGP implementations, we show a warning if a message contains a new, unexpected key (you already have accepted a correspondent's key, but a new different key arriving).

In this scenario, prior to this patch, we'd show a notification bar with string ID openpgp-be-careful-new-key

I'm seeing this with a message that triggers multiple notifications, "cannot decrypt", "claims to contain sender's key".
Do you limit the number of notifications you show?

I found another message that doesn't encryption, and there's no red warning.

The message openpgp-be-careful-new-key is missing in that scenario, too, so it seems the new code generally omits this warning.

[edit: clarification: I don't complain that red warning is missing. In this second scenario, it was correct that no red warning was there. The be-careful message is missing in both scenarios, regardless of a red warning shown or not shown.]

Comment on attachment 9177539 [details] [diff] [review]
1647039-openpgp-message-pane.diff[LANDED ON C-C]

removing approval requests.

Sorry for the regressions introduced.
I'll compare this trunk with 78 and upload a follow up patch.

These are the things that need to be fixed:

• S/MIME message, Comment 56.
• Missing openpgp-be-careful-new-key notification, Comment 57.
• Incorrect "Addition keys" message, Comment 58.
• Missing red warning when unable to decrypt a message, Comment 59.

Did you notice any other inconsistency?

Do you limit the number of notifications you show?

No, all the notifications should be there without limit, and they shouldn't exclude each other.

The patch doesn't apply to comm-esr78 as is. A manually merged version of the patch will be required.

I'll create a 78 variation once everything works as expected.

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

I'll create a 78 variation once everything works as expected.

For comm-central you need an incremental patch anyway.
For esr78, we could then land the merged patch I've provided, also plus your incremental patch.

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

• Missing red warning when unable to decrypt a message, Comment 59.

No. Comment 59 is NOT an additional problem. I simply mentioned it to correct my theory from comment 57 (the warning is missing in both scenarios, with or without other red warning present).

Did you notice any other inconsistency?

Not yet. I'm running esr78 as my primary client, I can pick up your current patch in my local build to give it more testing.

This is a merged version of the c-c commit from comment 49 that applies on comm-esr78.
The patch is based on top of patches from bug bug 1665497 and bug 1662279 which I expect to land prior to this one.
(There is a small overlap.)

Pinging also Kai on this review in case he gets to it.
This patch fixes the reported issues:

• Fix the usage of an incorrect signature icon in the security pane.
• Fix spacing issue in the encryption button.
• Fix error when unable to decrypt S/MIME message: comment 59.
• Fix missing openpgp-be-careful-new-key notification: comment 57 and comment 59.
• Fix incorrect "Addition keys" message: comment 58.

Regarding the "Additional Keys" section, when I send an encrypted and signed email to myself in 78, I get the additional keys section with an "unknown" key.
With my patch, the key listed is correctly recognized as my own.

I missed the collapsing of the new label item when the popup is hidden.

Would be good to also add tests for this.

Indeed, I'll create a follow up bug for this.

Pushed by thunderbird@calypsoblue.org:
https://hg.mozilla.org/comm-central/rev/2c48e0d3e354
Follow-up: Fix wrong notifications in message security popup. r=mkmelin

Comment on attachment 9177539 [details] [diff] [review]
1647039-openpgp-message-pane.diff[LANDED ON C-C]

[Approval Request Comment]
Regression caused by (bug #): -
User impact if declined: Not a great UI or experience when viewing encryption information due to the usage of dialogs popup and a hard to read list of included keys.
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low as it mostly tackles UI updates and doesn't touch any OpenPGP core functionality.

Comment on attachment 9180741 [details] [diff] [review]
1647039-openpgp-message-pane-PART2.diff[LANDED ON C-C]

[Approval Request Comment]
Follow up to the previous patch to fix some inconsistencies.

Comment on attachment 9180741 [details] [diff] [review]
1647039-openpgp-message-pane-PART2.diff[LANDED ON C-C]

[Triage Comment]
This missed making it to 82.0b3. But if you want this in 72.4.0 please request approval today so it makes the cut. (it looks safe to me)

Comment on attachment 9177539 [details] [diff] [review]
1647039-openpgp-message-pane.diff[LANDED ON C-C]

Removing comm-beta approval since it missed 82b3.

Requesting direct uplift to 78.4

[Approval Request Comment]
Regression caused by (bug #): -
User impact if declined: Not a great UI or experience when viewing encryption information due to the usage of a dialog popup and a hard to read list of included keys.
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low as it mostly tackles UI updates and doesn't touch any OpenPGP core functionality.

Comment on attachment 9180741 [details] [diff] [review]
1647039-openpgp-message-pane-PART2.diff[LANDED ON C-C]

[Approval Request Comment]
Same as above. These patches should land together

Comment on attachment 9177539 [details] [diff] [review]
1647039-openpgp-message-pane.diff[LANDED ON C-C]

Cancelling these approval requests since these patches don't apply cleanly on 78.
I'm gonna create a 78 variation with the two patches merged into one.

Variation for 78 created with the two previous patches merged.

[Approval Request Comment]
Regression caused by (bug #): -
User impact if declined: Not a great UI or experience when viewing encryption information due to the usage of a dialog popup and a hard to read list of included keys.
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low as it mostly tackles UI updates and doesn't touch any OpenPGP core functionality.

While creating the 78 variation, I noticed a super tiny error with a missing CSS declaration for the SMIME signature style.
I fixed it in the 78 variation, but I'm uploading a tiny patch for trunk.

A very silly mistake I didn't notice in PART 2.

Comment on attachment 9181851 [details] [diff] [review]
1647039-openpgp-message-pane-PART3.diff

I'm self approving this patch since we need something to push after the recent m-c push, and this is a super minor CSS mistake that it's good to be merged.

Comment on attachment 9181845 [details] [diff] [review]
1647039-openpgp-message-pane-78.diff

[Triage Comment]
Approved for esr78

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/bd69ba13b037
Follow up: Missing CSS declaration in message security popup. r=aleca

Thunderbird 78.4.0:
https://hg.mozilla.org/releases/comm-esr78/rev/eaab6105af71

I believe this bug caused a serious regression in TB 78.4.0 as well as in 83.0 beta1.
That is, inline PGP encrypted messages now fail to decrypt. The gray bar with the 'Decrypt' button at the top right corner doesn't show up, and hence the message does not decrypt at all. This still worked in TB 78.3.3, and 82.0 beta3. See also bug 1649030.
The behavior above applies to the scenario were the sender did sign the message, the sig is valid, and the sender's public key is present in the receivers OpenPGP Key Manager.
There are some other (undesired) effects when the senders public key isn't available in the receivers OpenPGP Key Manager, i.e. either non-existent, or expired.
So in short, this bug causes more problems then it fixes. Unless there are other related recent changes changes in TB 78.4.0 which may have caused this , this bug should be backed out of TB 78.4.0 ASAP.
Re-opening.

Do you have a message you could forward to me that triggers this problem?
Do you see an "OpenPGP" label in the top right corner of the message header?

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

Do you have a message you could forward to me that triggers this problem?

I sent you an email.

Do you see an "OpenPGP" label in the top right corner of the message header?

Upon opening the message, the "OpenPGP" label in the top right corner of the message header shows up as long as the sender's key is not present in the recipients OpenPGP Key Manager.
If this is the intended behavior, then to me, this feels inferior to the previous behavior, where the "Decrypt" button showed up instead at the top right corner.
So the message could be decrypted with a single click before, now it needs at least two clicks. And an "OpenPGP" button is somewhat less intuitive than a one-click "Decrypt" button.

Only after pressing the "OpenPGP" button, the "Decrypt" button is revealed. Upon pressing "Decrypt", this leads to a temporary inconsistent status:
The message status symbols at the top right indicate an encrypted and signed message. However, the pop up says 'no signature', and 'not encrypted' - see screenshot attached.
One needs to press the "OpenPGP" button a second time to clear this up. That's pretty confusing.

So the message could be decrypted with a single click before, now it needs at least two clicks. And an "OpenPGP" button is somewhat less intuitive than a one-click "Decrypt" button.
Only after pressing the "OpenPGP" button, the "Decrypt" button is revealed.

We decided to move all the notifications inside the encryption security popup to avoid cases in which users might end up with 3 or 4 different notification bars.

I see a couple of potential issues and improvements here.
First, a discoverability issue for the Decrypt action, which we could consider moving outside the popup panel.
Second, the necessity of reloading the message security information after the user clicks Decrypt.

These are all UX issues due to a different paradigm implemented here, but nothing was broken or made unusable by this bug, so I'm gonna close it again and create a follow up bug to track improvements.

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

These are all UX issues due to a different paradigm implemented here, but nothing was broken or made unusable by this bug, so I'm gonna close > it again and create a follow up bug to track improvements.

That isn't correct. As explained in comment #85, an inline PGP encrypted message fails to decrypt (due to the lack of a "Decrypt" button) once the sender's key has been imported. There is no "OpenPGP" button either at this stage. This needs to be fixed.
Re-opening again.

There is no "OpenPGP" button either at this stage.

I can't reproduce this issue.
After I import the sender's key, the "OpenPGP" button in the message header is present for me, and I can interact with it.

That may be the case initially after importing the key. But not after closing and re-opening the message with the key already present.

So in the standalone message window? I noticed for an .eml file opened from file the button is missing but it's not something new.

Indeed, the fact that the encryption technology button (OpenPGP, S/MIME) doesn't appear in the message header is an old issue, not regressed by this bug. Also in the previous version the encryption icons weren't visible in this scenario.

The problem introduced by my patch is that now the Decrypt bar is inside the Message Security Pane, not anymore printed as a full bar notification, so without the technology button visible, users can't access the decrypt button.

I proposed a couple of solutions in bug 1672851.
Which solution do you think we should pursue? Fixing the missing showing of the button + message pane, or moving the Decrypt action to an external notification bar?

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

Indeed, the fact that the encryption technology button (OpenPGP, S/MIME) doesn't appear in the message header is an old issue,

But only for .emls, not when opening mails from a mail folder in a new window. It's unclear how tricky it is to fix that. Possibly more than it should...

I guess decrypt should indeed instead be in the a notification bar on top, if needed. Like blocked remote content.

I guess decrypt should indeed instead be in the a notification bar on top, if needed. Like blocked remote content.

Sounds good to me, and I think it makes sense for easy discoverability.
I'll work on this on bug 1672851.

Christian, does this sound good? Let's close this bug and continue on the other bug since we're not gonna do a backup, and uploading a follow up patch here will only complicate things for uplifting.

Sounds good to me too. Closing this bug - finally.