284369 - UI for encrypted messages that can't be decrypted not dramantic.

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Robert Relyea]

When you read an encrypted message that cannot be displayed because if failed to
decrypt, you simply get a blank screen. Something a little more visual would by
nice (a gray background with appropriate text --- not "Undecryptable message"
like I tried to do;).

Comment 1

[Robert Relyea]

Attachment: How NOT to do this.

This patch is how I tried to fix the problem. NOTE: this patch does include
appropriate code to handle smart card insertion/removal. The code is dependent
on bug 284366.

This patch really isn't meant to be applied as is. I'm hoping our thunderbird
mail expert would know the correct way to handle this (including any
prerequisit i18n issues).

Comment 2

[Scott MacGregor]

There are several areas in the code where we try to set HTML on the message pane
(offline imap messages which we have downloaded, deleted newsgroup posts, etc.).
You can leverage the same code by calling:

nsIMsgWindow::displayHTMLInMessagePane
http://lxr.mozilla.org/mozilla/source/mailnews/base/public/nsIMsgWindow.idl#72

Although I missed this the first time, but that's what your patch is already
doing. It just needs tweaked to move the strings to msgReadSMIMEOverlay.properties.
 

Comment 3

[Robert Relyea]

Thanks scott,

How do I write without clobbering the header display, though?

bob

Comment 4

[Scott MacGregor]

oh. hmm.

What if you add a new parameter to DisplayHTMLInMessagePane which is a bool
which specifies whether to clear the message pane or not. 

That bool could then be checked before the call to clear the header pane:
http://lxr.mozilla.org/mozilla/source/mailnews/base/src/nsMsgWindow.cpp#510

Comment 5

[Robert Relyea]

So I should forget my patch with tempararily clears the
msgWindow.messagePanelController before calling
msgWindow.displayHTMLInMessagePane;).

// prevent the header from being cleared
aMessagePaneController = msgWindow.messagePaneController;
msgWindow.messagePaneController = null;
// insert our message
msgWindow.displayHTMLInMessagePane("Encrypted Message", "... html body.. ");

// restore the PaneController
msgWindow.messagePaneController = msgWindow.messagePaneController;

I wasn't sure how frozen the displayHTMLInMessagePane() was. I think the bool is
preferable, since I wasn't confident that msgWindow couldn't be referenced by C
code on another thread.

Comment 6

[Scott MacGregor]

none of these interfaces are frozen. We just need to change the interface ID in
nsIMsgWindow.idl when we change the argument.

Comment 7

[Robert Relyea]

Attachment: Secure client UI

This patch implements the UI as suggested by mscott.

Requesting reviews for both the code and the UI.

Comment 8

[Robert Relyea]

Attachment: This is not a patch for checking

This patch implements a possible future feature. It would be nice to link
directly back to the cert manager in this case so the user could fetch his
certs without digging through a lot of menu options.

They way this patch is implemented, however, does not feel like the correct way
to check it in to the tree.

Comment 9

[Scott MacGregor]

Comment on attachment 189125 [details] [diff] [review]
Secure client UI

1) can you make sure that gBundle or gBrandBundle aren't already getting
defined by other JS files that get loaded along with msgHdrViewSMIMEOverlay.js?
(i.e. is it defined in mailWindowOVerlay.js, or msgHdrViewOverlay.js)? I have a
feeling the brand bundle is already defined somewhere that you should be able
to re-use. 

2) 
+      var brand = gBrandBundle.getString("brandRealShortName");
+      //var brand = gBrandBundle.getString("brandShortName");

you can use brandShortName now. I finally fixed the bug that forced us to use
brandRealShortName

3) Is it over kill to make the html snippet load a chrome CSS file that has the
colors, padding and spacing CSS for the table? It may be, just throwing it out
there.

4) I think this part of the text is missing some language:
"The sender encrypted this message to you using of your digital certifictes"

Fix those minor things and I'll gladly r/sr.

Comment 10

[Robert Relyea]

Attachment: Incorportate mscott's comments.

gBrandBundle was opened by the main window. I included an optional
initialization if it wasn't initialized as paranoia. I changed the name of the
other bundle so it wouldn't conflict with other overlay names (none use
gBundle, but it seemed to generic to trust).

I've removed most of the html formatting, but there may be a better way to do
what I want with css, I'd gladly take suggestings.

The rest of the comments were pretty straightforward to implement.

bob

Comment 11

[Robert Relyea]

Comment on attachment 190910 [details] [diff] [review]
Incorportate mscott's comments.

Hmmm I know I changed the brandShortName... but it's not in this patch... I'll
attach another!

Comment 12

[Robert Relyea]

Attachment: OK, really incorporate the comments.;).

OK, this patch does have all the changes. Unlike the comment I made, it does
not do the paranoia allocate of gBrandBundle, but depends on it being
initialized in messageWindow.js

Comment 13

[Scott MacGregor]

Comment on attachment 190914 [details] [diff] [review]
OK, really incorporate the comments.;).

You should be able to remove:

+    <stringbundle id="bundle_brand"
src="chrome://branding/locale/brand.properties"/>

Thanks for making those changes.

If you want to control the CSS of your message via CSS, you could do something
like making your HTML snippet include a CSS file. Something like
"chrome://messenger/skin/smime/msgSMIME****.css"

and that chrome file has CSS which styles your message however you want
including icons, colors, etc. If you do it that way you'll have to add some
class names to your html table so it can be styled. I'll let you decide if you
want to control the look of your error dialog via CSS.

Comment 14

[Scott MacGregor]

Robert, we do what I was suggesting to style the message body for e-mails
(things like vcards, etc.):

http://lxr.mozilla.org/mozilla/source/mailnews/mime/emitters/src/nsMimeHtmlEmitter.cpp#360

Comment 15

[u60234]

Small nit on the CantDecryptBody= strings:
... <li>If you are using a new new machine,...

Comment 16

[Robert Relyea]

Checking in mail/extensions/smime/content/msgHdrViewSMIMEOverlay.js;
/cvsroot/mozilla/mail/extensions/smime/content/msgHdrViewSMIMEOverlay.js,v  <--
 msgHdrViewSMIMEOverlay.js
new revision: 1.4; previous revision: 1.3
done
Checking in mail/locales/en-US/chrome/messenger-smime/msgReadSMIMEOverlay.proper
ties;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger-smime/msgReadSMIMEOverlay.p
roperties,v  <--  msgReadSMIMEOverlay.properties
new revision: 1.2; previous revision: 1.1
done
Checking in mailnews/base/public/nsIMsgWindow.idl;
/cvsroot/mozilla/mailnews/base/public/nsIMsgWindow.idl,v  <--  nsIMsgWindow.idl
new revision: 1.27; previous revision: 1.26
done
Checking in mailnews/base/src/nsMsgWindow.cpp;
/cvsroot/mozilla/mailnews/base/src/nsMsgWindow.cpp,v  <--  nsMsgWindow.cpp
new revision: 1.94; previous revision: 1.93
done
Checking in mailnews/base/util/nsMsgIncomingServer.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgIncomingServer.cpp,v  <--  nsMsgIncomin
gServer.cpp
new revision: 1.230; previous revision: 1.229
done
Checking in mailnews/extensions/smime/resources/content/msgHdrViewSMIMEOverlay.x
ul;
/cvsroot/mozilla/mailnews/extensions/smime/resources/content/msgHdrViewSMIMEOver
lay.xul,v  <--  msgHdrViewSMIMEOverlay.xul
new revision: 1.8; previous revision: 1.7
done
Checking in mailnews/extensions/smime/resources/locale/en-US/msgReadSMIMEOverlay
.properties;
/cvsroot/mozilla/mailnews/extensions/smime/resources/locale/en-US/msgReadSMIMEOv
erlay.properties,v  <--  msgReadSMIMEOverlay.properties
new revision: 1.2; previous revision: 1.1
done
Checking in mailnews/news/src/nsNNTPProtocol.cpp;
/cvsroot/mozilla/mailnews/news/src/nsNNTPProtocol.cpp,v  <--  nsNNTPProtocol.cpp

new revision: 1.373; previous revision: 1.372
done

Comment 17

[Kai Engert [:KaiE:]]

This caused regression bug 380744