989090 - "reference to undefined property event.target._folder" warning when clicking "Get messages" button

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[:aceman]

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.

Comment 1

[:aceman]

Attachment: 989090.patch

Comment 2

[Magnus Melin [:mkmelin]]

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

Comment 3

[:aceman]

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.

Comment 4

[WADA:World Anti-bad-Duping Agency]

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?

Comment 5

[:aceman]

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.

Comment 6

[WADA:World Anti-bad-Duping Agency]

(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?

Comment 7

[:aceman]

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).

Comment 8

[Magnus Melin [:mkmelin]]

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

Comment 9

[Magnus Melin [:mkmelin]]

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?

Comment 10

[:aceman]

I see it on a clean trunk and in safe mode.

Suyash, can you reproduce this?

Comment 11

[Richard Marti (:Paenglab)]

I see this too on Win7.

Comment 12

[Suyash Agarwal (:sshagarwal)]

(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 13

[:aceman]

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)".

Comment 14

[:aceman]

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

Comment 15

[Jim Porter (:squib)]

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.

Comment 16

[:aceman]

(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).

Comment 17

[Jim Porter (:squib)]

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

Comment 18

[:aceman]

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.

Comment 19

[Jim Porter (:squib)]

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.

Comment 20

[:aceman]

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

Comment 21

[Jim Porter (:squib)]

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 22

[Magnus Melin [:mkmelin]]

Comment on attachment 8398171 [details] [diff] [review]
989090.patch

Review of attachment 8398171 [details] [diff] [review]:
-----------------------------------------------------------------

Cancelling review. Doing _folder null does sound better

Comment 23

[:aceman]

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.

Comment 24

[:aceman]

Attachment: patch v2

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 25

[Jim Porter (:squib)]

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?

Comment 26

[:aceman]

Attachment: patch v3

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

Comment 27

[Jim Porter (:squib)]

Comment on attachment 8736051 [details] [diff] [review]
patch v3

Review of attachment 8736051 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me

Comment 28

[:aceman]

Thanks.

Comment 29

[aleth [:aleth]]

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