609877 - Don't access DOM elements before the document loads

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, defect)

Description

[[e-mail]]

Otherwise, XPConnect will try to attach XBL for them, which does particularly nasty things to the status bar, which makes lightweight themes unsuable.

Comment 1

[[e-mail]]

Attachment: Proposed patch

* sComposeMsgsBundle, sBrandBundle: moved to onload handler
* gMessengerBundle: unnecessary, already set in CreateMailWindowGlobals
* mMsgNotificationBar: switched to using a local variable

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 488457 [details] [diff] [review]
Proposed patch

I haven't actually tried this yet, but this concerns me a bit:

>-  mMsgNotificationBar: document.getElementById('msgNotificationBar'),

TB has this member variable, too, so it would possibly hurt extension compatibility to remove this. Actually I think we could/should copy what TB does:

http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#2547
  get mMsgNotificationBar() {
    delete this.mMsgNotificationBar;
    return this.mMsgNotificationBar = document.getElementById('msgNotificationBar');
  },

With that you could leave the rest as-is.

Minusing feedback for now; if you explain to me why we cannot go that way, or provide a new patch, I'll dig deeper (i.e. actually apply it and try it out).

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 488457 [details] [diff] [review]
Proposed patch

>+++ b/suite/mailnews/mail3PaneWindowCommands.js	Fri Nov 05 13:25:37 2010 +0000
>@@ -40,7 +40,6 @@
>  * ***** END LICENSE BLOCK ***** */
> 
> var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(Components.interfaces.nsIPromptService);
>-var gMessengerBundle = document.getElementById("bundle_messenger");

Funny, TB still has this line even though they don't use it in that file anymore. ;-)

We only use the variable in MsgDeleteFolder in that file. To test whether it still works (with the above line removed), just try to delete a special IMAP folder like Sent.

Comment 4

[[e-mail]]

(In reply to comment #2)
> Minusing feedback for now; if you explain to me why we cannot go that way, or
> provide a new patch, I'll dig deeper (i.e. actually apply it and try it out).
Well, I was really hoping for your feedback on integration with lwthemes...

(In reply to comment #3)
> We only use the variable in MsgDeleteFolder in that file. To test whether it
> still works (with the above line removed), just try to delete a special IMAP
> folder like Sent.
Actually as I already found out you have to be sneakier than that as the menuitem is normally disabled in that case ;-)

Comment 5

[[e-mail]]

(In reply to comment #2)
> I haven't actually tried this yet, but this concerns me a bit:
> >-  mMsgNotificationBar: document.getElementById('msgNotificationBar'),
> TB has this member variable, too, so it would possibly hurt extension
> compatibility to remove this.
Well, personally, I find that unlikely. And a Google search didn't turn up anything. But let's see what my reviewer says ;-)

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 488457 [details] [diff] [review]
Proposed patch

(In reply to comment #5)
> (In reply to comment #2)
> > I haven't actually tried this yet, but this concerns me a bit:
> > >-  mMsgNotificationBar: document.getElementById('msgNotificationBar'),
> > TB has this member variable, too, so it would possibly hurt extension
> > compatibility to remove this.
> Well, personally, I find that unlikely. And a Google search didn't turn up
> anything. But let's see what my reviewer says ;-)

For one, a Google search isn't proving anything here. I'd guess you couldn't search the source of most TB/MailNews extensions that way. Also I think it's cleaner using the getter. But you're right, that's up to the reviewer you choose.

Speaking of which, I thought about my role here again. I don't particularly like it if I ask someone for feedback and in the end get a review. Still that's what I did here. So, time to revisit my decision:

Your patch, with or without my suggested improvement, solves the problem in the main window, compose window, and standalone message window. Of course the lightweightthemesfooter="status-bar" line needs to be added back to messageWindow.xul in order to see the correct footer image in the standalone message window. Also be sure to check that you have no interfering extensions installed (like I had: Mailbox Alert 0.13.2). Use -safe-mode if in doubt.

For a final check we should make sure that all string bundles affected by this patch still work. Suggestions? Or leave it to the reviewer. ;-)

Comment 7

[[e-mail]]

Comment on attachment 488457 [details] [diff] [review]
Proposed patch

I certainly didn't notice any issues in my testing, otherwise I wouldn't have even posted the patch ;-)

Comment 8

[[e-mail]]

Pushed changeset 5970a061c135 to comm-central.