135214 - signed icon in 3-pane isn't going away

Status: VERIFIED FIXED

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

Description

[Scott MacGregor]

I just downloaded 2002040103 onto a laptop I'm using for a demo today. I noticed
after clicking on a signed message and then clicking on non signed messages that
that the little pen icon located next to the Total messages count in the status
bar isn't getting removed. 

This makes you think the current message you are reading is signed when it isn't.

Comment 1

[John Unruh]

Works for me with the 20020402 Win2000 trunk build.

Comment 2

[Charles Rosendahl]

I also cannot confirm this bug.  Using build 2002040203, the signature icon
works as expected.

Closing, works for me.

Charles

Comment 3

[Charles Rosendahl]

Closed - reconfirmed as of 04/08/02, wfm.

Comment 4

[Scott MacGregor]

re-opening. I'm still seeing this (at least on the trunk). Just had it happen again.

Build ID: 2002051608.

the signed icon isn't going away in the status bar of the mail 3-pane. Just like
before. 

Not always reproducible though, but something is getting us in a state where we
don't try to hide the icon anymore. 

Comment 5

[Kai Engert [:KaiE:]]

Now I saw this, too. Not exactly, but very similar. Instead of a signed icon not
going away, the signature icon on the statusbar was no longer showing up.

Only the message header area showed the correct icon.

Unfortunately I was seeing this with a build that had not the DOM inspector
built in.

Whenever you see that problem next, please try to see whether the status bar
item has a "signing" property or not. If it has not, we somehow seem to fail to
set it. It it has it, what we see would be a XUL bug.

Comment 6

[Kai Engert [:KaiE:]]

The property is "signed" (not signing as said in the previous comment).


I have an idea. The code that is trying to modify the property is in
msgHdrViewSMIMEOverlay.js.

The code to set the property on statusbar and header area is directly located to
each other, so it should never happen, that we try to set only one of the
properties.

Is it possible, that setting the property of the statusbar does not work from
within that overlay for the header area?

Comment 7

[Kai Engert [:KaiE:]]

I continue to see this from time to time.

I am unable to reproduce it when I want to. The problem sometimes suddenly
appears after having run the application for a while. (If you are able to
reproduce the problem consistently, please let me know.)


I have seen multiple variations of the problem. The common behaviour is that one
or two icons are permanently shown on the status bar, and do not change when
switching messages. This is not restricted to a particular icon. I have seen
good and bad, signature and encryption icon to "stick".


When I last saw the problem, I used the DOM inspector to learn more.
I saw the correct attributes do actually arrive in the DOM.

In my opinion, it is a problem with the DOM and the CSS rules that sometimes the
UI does not get updated any more.


Note that this problem is restricted to a particular open window. When you close
the mail window, and reopen it, everything works again.

Comment 8

[Kai Engert [:KaiE:]]

In an attempt to produces ideas, as to what could be the cause of the problem, I
read the code in msgHdrViewSMIMEOverlay.js, which is responsible for setting the
XUL UI attributes, and the style sheets msgReadSMIMEOverlay.css.

I found something that could theoricatically be responsible, as it could confuse
the CSS interpreter.

The mail window contains the XUL elements signed-status and encrypted-status.
The style sheet contains visibility rules. Depending on XUL attributes, the
elements are defined to be visible or collapsed.
However, the JavaScript code is also making changes to the visibility attributes
of those elements.

Maybe that's a not a good idea. I think it is reasonable to get rid of the
duplicate control logic, in the hope we eliminate potential CSS confusion and
avoid a potential bug in CSS.

I'll attach a patch that removes the duplicate logic, and still sets the correct
UI. I would like to try that code on the trunk. I suggest to verify that code on
the trunk, and if it does not cause any problems, I suggest to check that
simpler logic in to the branch.

Comment 9

[Kai Engert [:KaiE:]]

Attachment: Patch v1 / suggested simplification

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that msgReadSMIMEOverlay.css has a few CSS syntax errors in it.  (CSS error
reporting is now turned on for DEBUG builds.  See bug 155855.)  In particular,
it uses 'collapsed' instead of 'collapse' in a number of places.  See the patch
on bug 139942.

Comment 11

[Kai Engert [:KaiE:]]

Indeed. When I use a debug trunk build I see:

CSS Error (chrome://messenger/skin/smime/msgReadSMIMEOverlay.css :42.24): Error
in parsing value for property 'visibility'.  Declaration dropped.
CSS Error (chrome://messenger/skin/smime/msgReadSMIMEOverlay.css :61.24): Error
in parsing value for property 'visibility'.  Declaration dropped.

We can either fix the error, or remove the declarations completely, as I'm doing
it in the patch.

Comment 12

[Kai Engert [:KaiE:]]

David, can you please review the patch?

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 90802 [details] [diff] [review]
Patch v1 / suggested simplification

r=dbaron.  Are there similar problems for the icon in message compose?	And
does whether you use 'visibility: collapse' as well affect the amount of
spacing where the icon would have been?  (Which behavior is preferable, if
they're different?)

Comment 14

[Kai Engert [:KaiE:]]

I have never seen such a problem with the mail compose window.
But I see that we have the same stylesheet error in that window, too.
I will handle fixing it in a separate bug.

Comment 15

[Scott MacGregor]

Comment on attachment 90802 [details] [diff] [review]
Patch v1 / suggested simplification

sr=mscott

Comment 16

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 17

[Kai Engert [:KaiE:]]

Asa gave trunk approval by email.

Checked in to trunk, marking fixed.

Comment 18

[Christopher Blizzard (:blizzard)]

Did this make it into the 1.0 branch?

Comment 19

[Kai Engert [:KaiE:]]

No, I'd need ADT's approval to check it in. I'll ask them.

Comment 20

[Jaime Rodriguez, Jr.]

carosendahl: Charles, can you take a look at this one this morning and mark it
verified if it works, so that we may consider this one for the branch. thanks!

Comment 21

[Charles Rosendahl]

20020715 Trunk Builds.

As far as I can tell from my testing, it appears to work - I have never been
able to create a reproducible case for this.  All of this testing I have done so
far indicates that the status bar icons are now being reset correctly.

The fix itself does not cause any regressions that I can see, and perhaps it
does indeed fix the cases where te icons were randomly sticking to the screen.

Comment 22

[scottputterman]

adding adt1.0.1+.  Please check this into the branch today.

Comment 23

[Kai Engert [:KaiE:]]

I just pinged drivers for re-approval, since the original branch approval has
expired.

Blizzard, you are a driver, arent't you?

Comment 24

[Christopher Blizzard (:blizzard)]

Reapproved.  Do it.

Comment 25

[Kai Engert [:KaiE:]]

I did it. Checked in to branch.

Comment 26

[Charles Rosendahl]

Verified on 20020723 Builds