1647039 - [OpenPGP] Improve the UI and UX of the Message Pane

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, enhancement, P1)

Description

[Alessandro Castellani [:aleca]]

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.

Comment 1

[Andreas]

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.

Comment 2

[Andreas]

Attachment: Message list_Enigmail.png

Comment 3

[Alessandro Castellani [:aleca]]

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

Comment 4

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2020-07-13 14-21-34.png

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.

Comment 5

[Alessandro Castellani [:aleca]]

Attachment: Message Pane Alerts.png

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.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9164906 [details]
Message Pane Alerts.png

Looks pretty good. I think it would be better to have it pop out a s sidebar and not on top of the other content though.

Comment 7

[Richard Marti (:Paenglab)]

Comment on attachment 9164906 [details]
Message Pane Alerts.png

Looks good. What do you plan to show below the two info boxes? Certificate information?

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.

Comment 8

[Kai Engert (:KaiE:)]

(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.

Comment 9

[Alessandro Castellani [:aleca]]

Attachment: openPgpPanel.gif

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.

Comment 10

[Andreas]

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.

Comment 11

[Richard Marti (:Paenglab)]

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.

Comment 12

[Alessandro Castellani [:aleca]]

Attachment: 1647039-openpgp-message-pane.diff

Comment 13

[Richard Marti (:Paenglab)]

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

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

This breaks stand-alone message windows, no header pane is shown and I get the error: Uncaught TypeError: can't access property "classList", securityPane is null.

::: mail/base/content/msgSecurityPane.inc.xhtml
@@ +4,5 @@
> +                  <html:div style="position: relative;">
> +                    <html:div id="securityPane">
> +                      <html:header class="message-security-header">
> +                        <html:h3>&status.label; <html:span id="techLabel"></html:span></html:h3>
> +                        <html:button class="toolbarbutton-1 close-crypto-button"

Instead of `toolbarbutton-1` set `close-icon` and remove the button-close-icon class on the child image. Additionally set on #close-crypto-button a `border-style: none;`. With this you don't need to re-invent the wheel for the close icon which isn't complete and doesn't work properly on dark theme.

Comment 14

[Magnus Melin [:mkmelin]]

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

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

I'm not sure we want to have the "counter" bubble. It's a bit messy, and also creates a chore for the user.
I agree with Richard, an arrow popup could be nicer, so it's connected to the tech. I do think this works fairly nicely too.

::: mail/extensions/openpgp/content/ui/enigmailMessengerOverlay.js
@@ +387,5 @@
>      EnigmailLog.DEBUG("enigmailMessengerOverlay.js: messageCleanup\n");
>  
>      let element = document.getElementById("brokenExchangeBox");
>      if (element) {
> +      element.setAttribute("collapsed", true);

`.hidden = true;` doesn't work?
(along with hidden attribute in the markup)

::: mailnews/extensions/smime/msgReadSMIMEOverlay.js
@@ +10,5 @@
>  var { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
>  
> +var EnigmailFuncs = ChromeUtils.import(
> +  "chrome://openpgp/content/modules/funcs.jsm"
> +).EnigmailFuncs;

var { EnigmailFuncs } = hromeUtils.import("chrome://openpgp/content/modules/funcs.jsm");

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

Comment 15

[Magnus Melin [:mkmelin]]

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

Comment 16

[Alessandro Castellani [:aleca]]

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.

Comment 17

[Magnus Melin [:mkmelin]]

(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.

Comment 18

[Alessandro Castellani [:aleca]]

Attachment: 1647039-openpgp-message-pane.diff

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 19

[Richard Marti (:Paenglab)]

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.

Comment 20

[Alessandro Castellani [:aleca]]

Attachment: 1647039-arrowpopup.diff

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

Comment 21

[Richard Marti (:Paenglab)]

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.

Comment 22

[Magnus Melin [:mkmelin]]

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

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

I like it. Not sure about the all white background - it could be the grayish one like with the other other patch.

::: mailnews/extensions/smime/msgReadSMIMEOverlay.js
@@ +73,5 @@
> +    let panel = document.getElementById("openPgpSecurityPanel");
> +    panel.openPopup(
> +      document.getElementById("encryptionTechBtn"),
> +      "after_end",
> +      -14,

It may be opening in a slightly wrong location (by some pixels)

Comment 23

[Alessandro Castellani [:aleca]]

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

Comment 24

[Alessandro Castellani [:aleca]]

Attachment: 1647039-openpgp-message-pane.diff

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

Comment 25

[Magnus Melin [:mkmelin]]

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

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

Please update S/MIME to also show info the same way. 

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.

Besides that, looks pretty good.

::: mail/base/content/msgSecurityPane.inc.xhtml
@@ +1,4 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +                  <panel id="openPgpSecurityPanel" type="arrow" orient="vertical"

I think we can drop the excessive indention. 2 spaces should be enough

@@ +5,5 @@
> +                         onpopuphidden="hideMessageReadSecurityInfo();"
> +                         position="bottomcenter topright"
> +                         tabindex="0">
> +                      <html:header class="message-security-header">
> +                        <html:h3>&status.label; <html:span id="techLabel"></html:span></html:h3>

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

Comment 26

[Richard Marti (:Paenglab)]

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

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

Looks good, thanks!

Yes, the S/MIME info should also open in the arrow panel.

::: mail/themes/shared/mail/messageHeader.css
@@ +373,5 @@
>  }
>  
> +#openPgpSecurityPanel > .panel-arrowcontent {
> +  background-color: var(--toolbar-bgcolor) !important;
> +  color: var(--lwt-text-color);

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

@@ +441,5 @@
>  html|button.crypto-button {
>    display: flex;
>    flex-wrap: nowrap;
>    align-items: center;
> +  margin-block: 9px 3px;

This makes the header grow. Do we need 9px? What about 6px?

@@ +494,5 @@
> +  width: 34rem;
> +}
> +
> +#openPgpSecurityPanel > .panel-arrowcontent {
> +  background: -moz-Dialog !important;

Like above, this doesn't apply. Thankfully, because this would break theming.

Comment 27

[Alessandro Castellani [:aleca]]

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.

Comment 28

[Alessandro Castellani [:aleca]]

Attachment: 1647039-openpgp-message-pane.diff

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.

Comment 29

[Magnus Melin [:mkmelin]]

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

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

I don't see anything for an email with just an Autocrypt header.  But I notice now that's also without this patch. I do see the notification to import for an email that has Autocrypt AND the key attached. I'm pretty sure we used to allow importing keys from Autocrypt header (only) at some point? Did bug 1631198 regress somehow?

I'm not sure what you meant, but Autocrypt is a OpenPGP only thing, so there is no "technology confusion". For S/MIME, signed mails always include the key internally so there is really no need for something like Autocrypt there.

Comment 30

[Magnus Melin [:mkmelin]]

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.

Comment 31

[Magnus Melin [:mkmelin]]

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

Comment 32

[Alessandro Castellani [:aleca]]

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.

Comment 33

[Alessandro Castellani [:aleca]]

Attachment: 1647039-openpgp-message-pane.diff

Comment 34

[Magnus Melin [:mkmelin]]

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

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

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

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

Comment 35

[Alessandro Castellani [:aleca]]

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?

Comment 36

[Richard Marti (:Paenglab)]

Attachment: two-scrollbars.png

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.

Comment 37

[Alessandro Castellani [:aleca]]

Ah, good catch, thanks.

Comment 38

[Alessandro Castellani [:aleca]]

Attachment: 1647039-openpgp-message-pane.diff

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 39

[Richard Marti (:Paenglab)]

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

Great, no scrollbars any more.

Comment 40

[Magnus Melin [:mkmelin]]

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

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

Looks good, but unfortunately i found one thing that does not work: View | Security info
Since the security info no longer opens a dialog, but the bubble, the way it works is slightly strange in general. But I think we can leave that for now, possibly remove that menu item in the future.
However, for a "normal" message with no security properties using that menu will very briefly pop up the bubble but (I think) instantly close it again.

::: mail/base/content/msgSecurityPane.inc.xhtml
@@ +5,5 @@
> +         onpopuphidden="hideMessageReadSecurityInfo();"
> +         position="bottomcenter topright"
> +         tabindex="0">
> +    <html:header class="message-security-header">
> +      <html:h3>&status.label; - <html:span id="techLabel"></html:span></html:h3>

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

Comment 41

[Alessandro Castellani [:aleca]]

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.

Comment 42

[Alessandro Castellani [:aleca]]

Attachment: 1647039-openpgp-message-pane.diff

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

Comment 43

[Alessandro Castellani [:aleca]]

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

Comment 44

[Alessandro Castellani [:aleca]]

Attachment: 1647039-openpgp-message-pane.diff

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

Comment 45

[Alessandro Castellani [:aleca]]

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.

Comment 46

[Alessandro Castellani [:aleca]]

Attachment: 1647039-openpgp-message-pane.diff

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

Comment 47

[Alessandro Castellani [:aleca]]

Attachment: 1647039-openpgp-message-pane.diff[LANDED ON C-C]

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.

Comment 48

[Magnus Melin [:mkmelin]]

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

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

Looks good, thx! r=mkmelin

Comment 49

[Pulsebot]

Pushed by [e-mail]:
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

Comment 50

[Andreas]

Attachment: Dark mode with key not available

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?

Comment 51

[Alessandro Castellani [:aleca]]

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 52

[Alessandro Castellani [:aleca]]

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 53

[Alessandro Castellani [:aleca]]

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 54

[Kai Engert (:KaiE:)]

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.

Comment 55

[Kai Engert (:KaiE:)]

Attachment: merge for esr78 v1

Initial merge attempt.

Comment 56

[Kai Engert (:KaiE:)]

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.

Comment 57

[Kai Engert (:KaiE:)]

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?

Comment 58

[Kai Engert (:KaiE:)]

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.

Comment 59

[Kai Engert (:KaiE:)]

(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 60

[Kai Engert (:KaiE:)]

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

removing approval requests.

Comment 61

[Alessandro Castellani [:aleca]]

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.

Comment 62

[Kai Engert (:KaiE:)]

(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.

Comment 63

[Kai Engert (:KaiE:)]

(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.

Comment 64

[Kai Engert (:KaiE:)]

Attachment: 1647039-for-esr78-part1-after-1662279.patch

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.)

Comment 65

[Alessandro Castellani [:aleca]]

Attachment: 1647039-openpgp-message-pane-PART2.diff

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.

Comment 66

[Alessandro Castellani [:aleca]]

Attachment: 1647039-openpgp-message-pane-PART2.diff

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

Comment 67

[Magnus Melin [:mkmelin]]

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

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

Would be good to also add tests for this.

::: mailnews/extensions/smime/msgReadSMIMEOverlay.js
@@ +162,5 @@
>      case Ci.nsICMSMessageErrors.VERIFY_UNTRUSTED:
>        sigInfoLabel = "SIInvalidLabel";
>        sigInfoHeader = "SIInvalidHeader";
>        sigInfo = "SIUntrustedCA";
> +      sigClass = "mismatch";

I don't think mismatch is the right change here. I think "notok" would be more appropriate.

Comment 68

[Alessandro Castellani [:aleca]]

Would be good to also add tests for this.

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

Comment 69

[Alessandro Castellani [:aleca]]

Attachment: 1647039-openpgp-message-pane-PART2.diff[LANDED ON C-C]

Comment 70

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2c48e0d3e354
Follow-up: Fix wrong notifications in message security popup. r=mkmelin

Comment 71

[Alessandro Castellani [:aleca]]

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 72

[Alessandro Castellani [:aleca]]

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 73

[Wayne Mery (:wsmwk)]

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 74

[Alessandro Castellani [:aleca]]

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 75

[Alessandro Castellani [:aleca]]

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 76

[Alessandro Castellani [:aleca]]

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.

Comment 77

[Alessandro Castellani [:aleca]]

Attachment: 1647039-openpgp-message-pane-78.diff

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.

Comment 78

[Alessandro Castellani [:aleca]]

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.

Comment 79

[Alessandro Castellani [:aleca]]

Attachment: 1647039-openpgp-message-pane-PART3.diff

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

Comment 80

[Alessandro Castellani [:aleca]]

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 81

[Wayne Mery (:wsmwk)]

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

[Triage Comment]
Approved for esr78

Comment 82

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bd69ba13b037
Follow up: Missing CSS declaration in message security popup. r=aleca

Comment 83

[Rob Lemley [:rjl]]

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

Comment 85

[Christian Riechers]

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.

Comment 86

[Alessandro Castellani [:aleca]]

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?

Comment 87

[Christian Riechers]

(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.

Comment 88

[Christian Riechers]

Attachment: decrypt.png

Comment 89

[Alessandro Castellani [:aleca]]

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.

Comment 90

[Christian Riechers]

(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.

Comment 91

[Alessandro Castellani [:aleca]]

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.

Comment 92

[Christian Riechers]

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

Comment 93

[Christian Riechers]

Attachment: decrypt_failure.png

Comment 94

[Magnus Melin [:mkmelin]]

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.

Comment 95

[Alessandro Castellani [:aleca]]

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?

Comment 96

[Magnus Melin [:mkmelin]]

(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.

Comment 97

[Alessandro Castellani [:aleca]]

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.

Comment 98

[Christian Riechers]

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