When clicking the "Get messages" button on the toolbar I get: Warning: ReferenceError: reference to undefined property event.target._folder Source file: chrome://messenger/content/messenger.xul Line: 1 I think it is because I did not choose any folder from the menupopup attached to the button and optionally accessible through the small arrow right of the button.

Can you give more STR for this? I don't seem to reproduce it.

In current trunk I get this javascript warning in the error console just by clicking the "Get messages" button on the main toolbar. The button has the small arrow to show dropdown list of account, but I do not operate it. I also have user_pref("javascript.options.showInConsole", true); user_pref("javascript.options.strict", true); in prefs.js.

Because oncommand handler is a kind of click event handler, I believe "event object upon oncommand at ToolBar button" doesn't have event.target._folder property. event.target is the button at ToolBar, and event.type="command". I think this is applicable to other ToolBar buttons. For example. > 3071 <toolbarbutton id="button-file" > 3072 type="menu" > 3073 class="toolbarbutton-1" > 3074 label="&fileButton.label;" > 3075 observes="button_file" > 3076 oncommand="MsgMoveMessage(event.target._folder)"> Copied from event handler of FolderTreeView?

Yes, only the menuitems inside menupopup have the _filter property (from the type="folder" binding). So if I do not choose an item from the list but click only the main button, the event.target in oncommand is the button that does not have _filter property.

(In reply to :aceman from comment #5) > So if I do not choose an item from the list but click only the main button, > the event.target in oncommand is the button that does not have _filter property. Why can "You do not choose an item from the list, or not, when click only the main button" be relevant to "event.target when ToolBar button is clicked"? When ToolBar button is clicked, event.target is always the ToolBar button regardless of "selected folder at Folder pane", isn't it?

Yes, event.target is the toolbar button and it does not have the ._filter property so we can't reference it unconditionally. I say nothing about "folder at folder pane". I was talking about the list of accounts that is accessible via the small arrow right of the toolbar button (widget-wise it is part of the button and the items are inside a menupopup).

I still can't reproduce this. (And it does work...)

Comment on attachment 8398171 [details] [diff] [review] 989090.patch Review of attachment 8398171 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review until I can reproduce. Are you sure it's not something in your queue that is causing it?

I see it on a clean trunk and in safe mode. Suyash, can you reproduce this?

I see this too on Win7.

(In reply to :aceman from comment #10) > I see it on a clean trunk and in safe mode. > > Suyash, can you reproduce this? Ya, I too see this error: Timestamp: 04/06/2014 09:47:05 PM Warning: ReferenceError: reference to undefined property event.target._folder Source File: chrome://messenger/content/messenger.xul Line: 1 On the tip. Though, the first account is selected in the accounts list.

Comment on attachment 8398171 [details] [diff] [review] 989090.patch I can still see this reliably. Can you guys reproduce now? It seems from the comments I am not alone. If you think it is too much code we could maybe change it to something like "MsgGetMessagesForAccount(event.target._folder || null)".

Or maybe MsgGetMessagesForAccount(("_folder" in event.target) && event.target._folder) .

Comment on attachment 8398171 [details] [diff] [review] 989090.patch I think it would make a lot more sense to just ensure that event.target._folder is always defined. Clearing review because I'll let Magnus decide whether he agrees.

(In reply to Jim Porter (:squib) from comment #15) > I think it would make a lot more sense to just ensure that > event.target._folder is always defined. How can we ensure that if we do not click in a folder picker so there is really no folder? In that case the called function knows to just get the currently selected server in the folder pane (or All). The problem is that event.target can point to the main button (which has no _folder) and to the dropdown which is a folder picker (which does have _folder).

Set `_folder` to `null` on the main button.

How is that done on a menu or toolbarbutton element? Hopefully not by looking up the elements by ID in some startup code and set the property.

You could finish up bug 473009 and use `.getAttribute("value")`. That would be my recommendation, since this seems like this patch is just removing a warning anyway.

Yeah, I'd love to. Can you help in that one? Looks I do not understand what Neil wants.

Sure. I don't see any significant issues with the patch, and would probably have given it provisional r+ already with a few minor changes.

Comment on attachment 8398171 [details] [diff] [review] 989090.patch Review of attachment 8398171 [details] [diff] [review]: ----------------------------------------------------------------- Cancelling review. Doing _folder null does sound better

I don't think finishing bug 473009 is relevant here as why should the main button reference the .getAttribute("value") of its menupopup, when we actually do not want the value. We just want to pass a clean null or undefined to the called function. But I got an idea here.

I thought this would work fine, however for some reason in addition to the oncommand on the menupopup, it is also firing the cmd_getNewMessages command and it is producing an error in the console (but the MsgGetMessagesForAccount() is succeeding and connecting to server). Any idea what happens?

Comment on attachment 8736007 [details] [diff] [review] patch v2 Review of attachment 8736007 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/mailWindowOverlay.xul @@ +1400,5 @@ > <menupopup type="folder" > mode="getMail" > id="appmenu_getAllNewMsgPopup" > + expandFolders="false" > + oncommand="MsgGetMessagesForAccount(event.target._folder);"> I think you'd want to add event.preventDefault(); to the end here?

Thanks for the hint. It seems event.stopPropagation() worked for me.

Comment on attachment 8736051 [details] [diff] [review] patch v3 Review of attachment 8736051 [details] [diff] [review]: ----------------------------------------------------------------- rs=me

Thanks.

https://hg.mozilla.org/comm-central/rev/f830f6f0b288eb16ec5b75177f554c71679b260c Bug 989090 - rework event handling on "Get messages" button to not throw a warning about "reference to undefined property event.target._folder". r=squib