129100 - Implement the UI for the mail compose window

Status: VERIFIED FIXED

(MailNews Core :: Security: S/MIME, defect, P1)

Description

[Kai Engert (:KaiE:)]

See http://www.mozilla.org/mailnews/specs/security/

This bug tracks implementation of the "mummy" button.
Icons will be replaced when they are available.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: Suggested Implementation

Please note that the images that I have prepared are not very good looking.
However, they are of the correct size, and the toolbar buttons looks correctly
(besides the face that will change as the icons from bug 104502 get ready).

Comment 2

[Kai Engert (:KaiE:)]

Stephane or David, can you please review?

Comment 3

[Kai Engert (:KaiE:)]

Attachment: Updated patch

This should now have all the other UI logic that is required for the message
compose window, as required by the spec.

- three different states for the send button
- three different states for the new security button.
- the status bar icons (clickable)
- the dropdown menu on the security button
- changed the text "always encrypt" to "require encryption" as mentioned in the
spec


(The logic, that will detect whether the message is currently in state "crypto
possible" or "crypto not possible" will not be implemented with this bug. This
also applies to enabling/disabling the send button, when sending is not
possible.)

Comment 4

[Kai Engert (:KaiE:)]

I notice all my changes are below mailnews/ and themes/.

Scott, can you please review? Thanks.


(Please note that I have been unable to make the toolbar work correctly, by
using only overlays. Only if I add securityButton directly inside
messengercompose.xul, the button appears at the correct location - otherwise it
appears even behind the throbber icon.)

Comment 5

[Kai Engert (:KaiE:)]

Comment on attachment 72720 [details] [diff] [review]
Updated patch

Sorry for the spam, I take my request for review back.

First, I have to make sure, the security button will only appear, if crypto is
compiled in.

Comment 6

[Kai Engert (:KaiE:)]

Attachment: New patch that uses overlays nicely

Comment 7

[Kai Engert (:KaiE:)]

I've come to the opinion that it does not make sense to post intermediate
patches or request reviews half way.

Comment 8

[Stephane Saux]

*** Bug 123990 has been marked as a duplicate of this bug. ***

Comment 9

[Kai Engert (:KaiE:)]

Attachment: Huge patch

Comment 10

[Kai Engert (:KaiE:)]

*** Bug 115335 has been marked as a duplicate of this bug. ***

Comment 11

[Kai Engert (:KaiE:)]

*** Bug 117763 has been marked as a duplicate of this bug. ***

Comment 12

[Kai Engert (:KaiE:)]

*** Bug 119996 has been marked as a duplicate of this bug. ***

Comment 13

[Kai Engert (:KaiE:)]

Javi, the latest patch contains changes to files in mozilla/security. Can you
please review them?

The changes are:
- cert picker no longer supports dynamic setting of the info and prompt labels,
it's not required. I removed those to make the dialog look closer to what has
been specified in the UI spec.
- cert picker for cert configuration no longer has the first entry preselected,
but the one that is given as a parameter. This is used to preselect the cert
that is currently configured.
- I implemented the new method nsIX509Cert::verifyForUsage, that verifies the
requested usage only and returns the verification result code. This was produced
by cloning code from other methods in nsNSSCertificate.cpp, so it should be correct.

Comment 14

[Javier Delgadillo]

security changes look good.

r=javi

Comment 15

[Kai Engert (:KaiE:)]

Attachment: Updated patch

Code in security/manager did not change, so r=javi does still apply.

The UI strings in this patch have r=cotter

Comment 16

[Kai Engert (:KaiE:)]

The latest patch has an enhancement: The message security info window, when
composing a message, will now show whether it is possible to send the message or
not.
Signed: Yes  => Signing is enabled and certificates are configured
Signed: No => Signing is disabled
Signed: Not possible => User requested to sign, but no certs have been configured.

Encrypted: Yes => Valid certs for all recipients are available
Encrypted: No => Encryption is disabled
Encrypted: Not possible => There is no valid cert for at least one recipient, or
there are no recipients specified at all.

Comment 17

[Kai Engert (:KaiE:)]

Attachment: screen shot, classic, composer with new toolbar and status bar icons

Comment 18

[Kai Engert (:KaiE:)]

Attachment: screen shot, modern, composer with new toolbar and status bar icons

Comment 19

[Kai Engert (:KaiE:)]

Attachment: screen shot, classic, mail window, icons in header area and status bar

Comment 20

[Kai Engert (:KaiE:)]

Attachment: screen shot, modern, mail window, icons in header area and status bar

Comment 21

[Kai Engert (:KaiE:)]

Attachment: screen shot, message security window when composing a message

Comment 22

[Kai Engert (:KaiE:)]

Attachment: Updated Patch

In order to be consistent with the other mail windows, which already show a
"security info" command in the view menu, I added that command to the composer
window's view menu, too.

But by doing so, I realized that this menu command is already contained below 
another menu command, "Options/Security".

With this patch, I have removed that command from the Options/Security menu.

I think it makes sense for the following reasons:
- the user expects only changeable items inside the options menu.
- the message security dialog does not allow any options to be changed, 
  it only displays information.
- moving the command to "View/Message Security Info" makes all mail
  window's menus consistent

However, the dropdown menu that opens when the security toolbar button gets
clicked, still shows that menu command.

Comment 23

[Kai Engert (:KaiE:)]

Attachment: Adding help buttons / removing unnecessary cancel buttons

John suggested, we don't need Cancel buttons in the security info dialogs.
Sean suggested to add help buttons to the security info dialogs.

Comment 24

[Scott MacGregor]

Comment on attachment 74967 [details] [diff] [review]
Adding help buttons / removing unnecessary cancel buttons

1) In nsISMimeJSHelper.idl, you'll want to change the license file so that you
are  the listed contributor instead of me. The copyright in that file says 1998
to. Should be 2002.

2) now that we cache one compose window and recycle it for subsequent windows,
does that expose any holes with us showing stale security info? Just asking as
it could be an interesting test case to make sure we have working.

3) Doesn't the following mean we'll always send an encrypted message or am I
misreading the JS assignment? 
+      var encryptionPolicy =
gCurrentIdentity.getIntAttribute("encryptionpolicy");
+      // 0 == never, 1 == if possible, 2 == always Encrypt.
+      gSMFields.requireEncryptMessage = encryptionPolicy == 2;

5) In msgCompSecurityInfo, it looks like we use a tree widget to represent the
security info. I believe tree is going away. Joe Hewitt has been converting all
tree widgets to either outliners or list boxes. I bet we'll need to touch base
with him to coordinate potential landing conflicts. See "phaseOutXULTrees" on
the mozilla landing plan: 
http://komodo.mozilla.org/planning/branches.cgi
for more details.

The only thing that worried me was the secure compose stuff in
MsgComposeCommands.js. I wonder if we can abstract the security stuff that went
in there a little bit. I need to sleep on that part and maybe talk to JF about
it in the morning. 

Nice work laying all this out Kai. Code looked really good.

Comment 25

[Kai Engert (:KaiE:)]

> 1) In nsISMimeJSHelper.idl, you'll want to change the license file so that you
> are  the listed contributor instead of me. The copyright in that file says 1998
> to. Should be 2002.

Done, changed it with a MPL license.


> 2) now that we cache one compose window and recycle it for subsequent windows,
> does that expose any holes with us showing stale security info? Just asking as
> it could be an interesting test case to make sure we have working.

We rely on the the gComposeRecyclingListener in MsgComposeCommands.js to be
called. I rely that both onClose and onReopen are called and calls are forwarded
to the security overlay.

For enhanced safety, I now added another deletion of the security info to the
onReopen function, just before we try to create a new instance, which ensures
cleared info when new object creation goes wrong.

I'm also relying that either gMsgCompose.compFields stays around between compose
window recycles, or, that on new compFields creation it is assured that
securityInfo will be null, which I assume it is by default.


> 3) Doesn't the following mean we'll always send an encrypted message or am I
> misreading the JS assignment? 
> +      var encryptionPolicy =
> gCurrentIdentity.getIntAttribute("encryptionpolicy");
> +      // 0 == never, 1 == if possible, 2 == always Encrypt.
> +      gSMFields.requireEncryptMessage = encryptionPolicy == 2;

Hmm, maybe that line is not obvious enough.
My understanding is, first the comparison will be evaluated, and the boolean
result will be assigned. So, only if encryptionPolicy is set to "2", we'll
require encrpytion, all other cases will not use encryption.
Value "2" is set by the preferences dialog. I wanted to leave that value for
now, until we implement the "if possible" encryption in a future version.


> 5) In msgCompSecurityInfo, it looks like we use a tree widget to represent the
> security info. I believe tree is going away. Joe Hewitt has been converting all
> tree widgets to either outliners or list boxes. I bet we'll need to touch base
> with him to coordinate potential landing conflicts. See "phaseOutXULTrees" on
> the mozilla landing plan: 
> http://komodo.mozilla.org/planning/branches.cgi
> for more details.

Hmm. I asked around a bit and found postings on the newsgroups. Looks like
<tree>s must be replaced with either <listbox> or <outliner>. I liked <listbox>
better.

The fact, that currently not a single xul or js file is using <listcell> is not
very encouraging.

However, I changed my code to use <listbox> and all the various suggested new
tags, and it seems to work.


> The only thing that worried me was the secure compose stuff in
> MsgComposeCommands.js. I wonder if we can abstract the security stuff that went
> in there a little bit. I need to sleep on that part and maybe talk to JF about
> it in the morning. 

If in the future, multiple encryption engines were used, the code in
MsgComposeCommands can just be extended to store a list of registered secure
compose overlays that wish to be notified.


> Nice work laying all this out Kai. Code looked really good.

I'm glad to hear that, thanks a lot, but note that I had good teachers ;)

Comment 26

[Kai Engert (:KaiE:)]

Attachment: Updated patch

Now using <listbox> instead of <tree>.

Now also includes the changes for Mac makefiles.

Comment 27

[Scott MacGregor]

Comment on attachment 75564 [details] [diff] [review]
Updated patch

Here's an idea I had last night for the secureCompose stuff in
MsgComposeCommands.js. Why don't we just fire custom events in
MsgComposeCommands.js:

This is how I notify the smime code that the message pane has been loaded. I
created a custom event and fired it from the generic mailnews code. Then the
smime code was registered as an event listener for this custom event so it gets
the notification. 

Here's a snippet:
i.e. 
  var event = document.createEvent('Events');
  event.initEvent('messagepane-loaded', false, true);
  var headerViewElement = document.getElementById("msgHeaderView");
  headerViewElement.dispatchEvent(event);

We can do the same thing here with 2 custom events:  compose-window-close
and
compose-window-reopen

What do you think?

Comment 28

[Kai Engert (:KaiE:)]

Attachment: Updated patch

> What do you think?

This patch implements what you suggested.

Comment 29

[Scott MacGregor]

Comment on attachment 76030 [details] [diff] [review]
Updated patch

sr=mscott. Thanks for making that additional change. You'll still need another
r=  from a mailnews person. 

Also, any packaging issues we need because of this change? I don't think there
were but I thought I'd mention it.

Comment 30

[Kai Engert (:KaiE:)]

> Also, any packaging issues we need because of this change? I don't think there
> were but I thought I'd mention it.

I agree. I added a lot of files, but no new libraries or archives.
I changed the jar.mn files and the makefiles.

I also gave a Win32 test build to Stephane, and he was able to use it.
My primary browser is currently a Linux build which includes this patch, and
this build has been installed using the installer, which I built myself.

In addition, Javi tested that the patch compiles on Mac, if he uses the GIFs
that I gave him separately.

Comment 31

[Jean-Francois Ducarroz]

Comment on attachment 76030 [details] [diff] [review]
Updated patch

Looks ok to me. R=ducarroz

Comment 32

[Asa Dotzler [:asa]]

Comment on attachment 76030 [details] [diff] [review]
Updated patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 33

[Kai Engert (:KaiE:)]

patch checked in, fixed.

Comment 34

[John Unruh]

Verified on 3/28 Win2000 trunk build.