Closed Bug 1319930 Opened 8 years ago Closed 8 months ago

When sub-menus are created in folderWidgets.xml,. oncommand is cloned onto the submenu leading to multiple executions

Categories

(MailNews Core :: Backend, defect)

Product:
MailNews Core
Component:
Backend
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED INVALID

People

(Reporter: jorgk-bmo, Assigned: aceman)

References

Details

User Story

This bug is about investigating whether:
1. oncommand can/should be moved back onto the parents (undoing some of bug 878805)
2. the clone code should be changed to prevent multiple execution
3. oncommand should even be moved onto the popup once the clone code is fixed.

Statistics:
-37 items found
-13 times (on)command is already on menulist
-23 times (on)command is on menupopup
--20 of those can be moved to the parent (e.g.menulist), assuming all the various parents can take a command (there is e.g. a <button>, <menu>, <toolbarbutton> used)
--03 of those already have another command in the parent. It seems those could be merged and only the parent command left.
-1 time there is no (on)command defined as running the command is handled specially via a xbl binding.
-10 from all those (on)command function calls (whether on menupopup or the parent) could be potentially removed once bug 473009 and bug 802609 are done.

Attachments

(4 files, 6 obsolete files)

Button script V4, to see event object content when command event(and click event) is propagated from element under Toolbar button of "Get Messages"
18.93 KB, text/plain
Details
patch for task 1 - TB
30.59 KB, patch
Details | Diff | Splinter Review
patch for task 1 - SM
4.09 KB, patch
iannbugzilla
: review+
Details | Diff | Splinter Review
patch for task 2 v1.3
4.40 KB, patch
Details | Diff | Splinter Review 
We have a lot of configuration like this:

<menulist id="searchableFolders" flex="2" class="folderMenuItem"
           displayformat="verbose">
  <menupopup class="searchPopup menulist-menupopup"
             type="folder" mode="search"
             showAccountsFileHere="true" showFileHereLabel="true"
             oncommand="updateSearchFolderPicker(event.target._folder);"/>
</menulist>

or

<button id="fileMessageButton" type="menu" label="&moveButton.label;"
        accesskey="&moveButton.accesskey;"
        observes="file_message_button"
        oncommand="MoveMessageInSearch(event.target)">
  <menupopup type="folder" showFileHereLabel="true" mode="filing"/>
</button>


where a popup is the child of a menulist or button. In bug 857230 a clean-up was done where the 'oncommand' was moved onto the menupopup.

Sadly that led to multiple executions due to the way that sub-menus of the popup are created at:
https://dxr.mozilla.org/comm-central/rev/5aa6e96146e52ae578d36c5ac5afc246c5b77c5a/mailnews/base/content/folderWidgets.xml#514

Those multiple executions caused bug 1106225 (fixed in bug 1319299 (TB) and bug 133212 (SM)).

This bug here is intended to identify more dormant problems with multiple execution and to offer a fix in the clone code.

(In reply to :aceman from bug 1106225 comment #46)
> It fixes all cases where oncommand is on the menupopup (which are many for
> now). That can be fixed [...] without enumerating them specifically.

Aceman, please show me at least three of the "many".
Noteworthy, bug 1106225 comment #16 (quoted here) from Neil:

===
A <menupopup type="folder"> clones itself otherwise its subfolders don't work properly on OSX. However this causes a duplication of attributes which is not wanted in the case of the oncommand attribute. There are three cases:
1. The oncommand attribute is on the parent element, so no duplication occurs
2. The oncommand action just updates a variable without doing anything twice
3. The oncommand action tries perform an action multiple times
I don't know whether it would be a good idea to blocklist certain attributes.

In this case of bug 1106225 the oncommand attribute would probably work on the parent element [the button] but I don't know for sure whether this would be appropriate in all cases.
===

This bug is about investigating whether
- oncommand can/should be moved back onto the parents (undoing some of bug 857230)
- the clone code should be changed to prevent multiple execution
- oncommand should even be moved onto the popup once the clone code is fixed.

It's partly an academic discussion, partly there are some dormant problems due to double execution.
Correction, wrong bug number for SM:
Those multiple executions caused bug 1106225 (fixed in bug 1319299 (TB) and bug 1133212 (SM)).
(In reply to Jorg K (GMT+1) from comment #0)
> where a popup is the child of a menulist or button. In bug 857230 a clean-up
> was done where the 'oncommand' was moved onto the menupopup.

Bug 878805,

> Those multiple executions caused bug 1106225 (fixed in bug 1319299 (TB) and
> bug 133212 (SM)).

Bug 1133212.

> 
> This bug here is intended to identify more dormant problems with multiple
> execution and to offer a fix in the clone code.
> 
> (In reply to :aceman from bug 1106225 comment #46)
> > It fixes all cases where oncommand is on the menupopup (which are many for
> > now). That can be fixed [...] without enumerating them specifically.
> 
> Aceman, please show me at least three of the "many".

8 occurences just here:
https://dxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/am-copiesOverlay.xul

> This bug is about investigating whether
> - oncommand can/should be moved back onto the parents (undoing some of bug
> 857230)

Bug 878805.

At least copy the history properly.

Why is there nobody CCed?
Sorry for quoting the wrong bug number. I pasted one that stated with "8" ;-)
Which history are you referring to. I think I've copied enough so this bug makes sense.

Thanks for the references. All of these could be fixed by moving oncommand up.

Nobody is CCed since these people might have been interested in the "move bug". Anyway, I've CCed them now.
(In reply to Jorg K (GMT+1) from comment #4)
> Thanks for the references. All of these could be fixed by moving oncommand
> up.
As I said, it's already not consistent:
https://dxr.mozilla.org/comm-central/rev/5aa6e96146e52ae578d36c5ac5afc246c5b77c5a/mail/base/content/mailWindowOverlay.xul#2931

Do you plan to move those down? Like you suggested to undo bug 1319299 and bug 1133212?
How big is the problem really?

Looking for
https://dxr.mozilla.org/comm-central/search?q=mode%3D%22filing%22&redirect=false
I get 30 - 9 = 21 references. It can't be so hard to visit those 21 and see whether the oncommand needs to be moved up. In many case it's "up" already.

Or can there be type="folder" without the mode="filing"?
(In reply to Jorg K (GMT+1) from comment #6)
> Or can there be type="folder" without the mode="filing"?
Yes, there can be, for example:
https://dxr.mozilla.org/comm-central/rev/5aa6e96146e52ae578d36c5ac5afc246c5b77c5a/mailnews/base/content/virtualFolderProperties.xul#65
(In reply to Jorg K (GMT+1) from comment #4)
> Thanks for the references. All of these could be fixed by moving oncommand
> up.

Yes, they could, but the question was about how many of oncommands are on menupopups NOW. Not how many will stay there if we move them as high up as possible.

> (In reply to Jorg K (GMT+1) from comment #4)
> > Thanks for the references. All of these could be fixed by moving oncommand
> > up.
> As I said, it's already not consistent:
> https://dxr.mozilla.org/comm-central/rev/
> 5aa6e96146e52ae578d36c5ac5afc246c5b77c5a/mail/base/content/mailWindowOverlay.
> xul#2931

As said (in the history you claim is not needed), these may be an attempt to prevent the cloning command bug for commands that have a permanent action where it is visible when they are run multiple times.

> Do you plan to move those down? Like you suggested to undo bug 1319299 and
> bug 1133212?

I would, if it makes sense for any consolidation.

So let's make the statistics already and not throw conflicting claims around:
1. who many oncommands are on menupopups (with type="folder") now?
2. how many could be moved to the parent element (there is no command on it yet)?
3. how many menupopups with type="folder" do not have a command because it is already on the parent?
(In reply to Jorg K (GMT+1) from comment #6)
> I get 30 - 9 = 21 references. It can't be so hard to visit those 21 and see
> whether the oncommand needs to be moved up. In many case it's "up" already.

Yes, let's visit them all to collect the stats. I will do it tomorrow if nobody does it until then.
 
> Or can there be type="folder" without the mode="filing"?

I would ignore any "mode", just count all type="folder" (I claimed ~40).
<off-topic>
How can I said it nicely ;-)

While I appreciate clean-up, we have blockers for TB 52 that someone should dedicate time to. So this bug here is stealing me a lot of time without any immediate benefit for our users.

I tried to untangle the clean-up and more academic discussion of where the 'oncommand' should be from the real world problem of causing real *corruption* in the "move from search panel" case. I'm sorry that not all the history was extracted from the other bug and that I misquoted numbers.

I'm happy with any solution that works and is consistent. I think I prefer the 'oncommand' being "up" so this attribute isn't part of what is cloned and therefore there is also no reason that it would be removed. As I said before, we could issue a warning should we ever find it in the clone operation.

Code churn is part of our daily lives, just look at the "supports array". So for clarity, I'm not opposed to moving the 'oncommand' up even if it churns a lot of code.
</off-topic>
(In reply to Jorg K (GMT+1) from comment #10)
> While I appreciate clean-up, we have blockers for TB 52 that someone should
> dedicate time to. So this bug here is stealing me a lot of time without any
> immediate benefit for our users.

Well, you first said you have no opinion and then suddenly you are driving this discussion to new heights:)
 
> As I said
> before, we could issue a warning should we ever find it in the clone
> operation.

We will find it as there are already cases of this and we do not want to clone it. So a warning is not enough and just annoying.
 
> Code churn is part of our daily lives, just look at the "supports array". So
> for clarity, I'm not opposed to moving the 'oncommand' up even if it churns
> a lot of code.

Sure, but some churn can't be avoided. And churn from A to B and then B to A is a different beast :)
OS: Unspecified → All
Hardware: Unspecified → All
See Also: → 1106225
(In reply to :aceman from comment #11)
> > As I said
> > before, we could issue a warning should we ever find it in the clone
> > operation.
> We will find it as there are already cases of this and we do not want to
> clone it. So a warning is not enough and just annoying.
Maybe a misunderstanding here: Move them up and then implement a warning to find anything we missed and avoid new cases.

> Sure, but some churn can't be avoided. And churn from A to B and then B to A
> is a different beast :)
Well, I wasn't involved in the "A to B", but since "B" has caused real *corruption* it wasn't a good choice in the first place. Sure, "B + B-fix" could be an option, but since - as stated - there is no good reason for "B" since consistency can also be achieved with "A" (and less code and less cloning and less removal), I think "A" would have been the better choice, so I'm happy to churn back to "A".

But why don't we ask people with long-standing experience like IanN or Ratty. Anyway, let's do the survey to see what we're talking about. How many "A"s do we have, how many "B"s and how many of the "B"s can't be fixed by churning them back to "A". Those should be left at "B" with a clear comment. And if we need to leave any "B"s, then clearly the warning is not a good idea. BTW: "A" is "up" and "B" is "down on the popup".
BTW, I looked at what was landed in bug 878805. This bug did a lot of clean-up. It moved some 'oncommand's from "A" to "B" but not all.

Moved: https://hg.mozilla.org/comm-central/rev/62ad06c7ba12#l2.16
Fatally moved: https://hg.mozilla.org/comm-central/rev/62ad06c7ba12#l4.33
Didn't move: https://hg.mozilla.org/comm-central/rev/62ad06c7ba12#l18.15
Didn't move: https://hg.mozilla.org/comm-central/rev/8779877620ee#l3.6

So we wouldn't be doing a "A to B" and then "B to A" ;-)
(In reply to :aceman from comment #3)
> 8 occurences just here:
> https://dxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/am-copiesOverlay.xul

Aha!
A typical use was '<menulist> single <menupoopup type="folder" oncommand="xxx();"> only </menulist>', which is containd in <row> under <radiogroup>.
> Fcc folder selection(Sent folder) in Copies&Folders.
> 
> <radiogroup id="doFcc" aria-labelledby="identity.doFcc">
>   <grid class="specialFolderPickerGrid">
>     <columns> <column/> <column flex="1"/> </columns>
>     <rows>
>       <row align="center">
>           <radio id="fcc_selectAccount" oncommand="setPickersState('msgFccAccountPicker', 'msgFccFolderPicker', event)" observes="broadcaster_doFcc"/>
>           <menulist id="msgFccAccountPicker" observes="broadcaster_doFcc">
>             <menupopup id="msgFccAccountPopup" type="folder" mode="filing" expandFolders="false" 
>              oncommand="noteSelectionChange('fcc', 'Account', event)"/>
>           </menulist>
>       </row>
>       <row align="center">
>           <radio id="fcc_selectFolder" oncommand="setPickersState('msgFccFolderPicker', 'msgFccAccountPicker', event)" observes="broadcaster_doFcc"/>
>           <menulist id="msgFccFolderPicker" observes="broadcaster_doFcc">
>             <menupopup id="msgFccFolderPopup" type="folder" mode="filing" expandFolders="false"
>              oncommand="noteSelectionChange('fcc', 'Folder', event)"/>
>           </menulist>
>       </row>
>     </rows>
>   </grid>
> </radiogroup>

oncommand on menulist is funny, is semantically bad or wrong in ordinal XUL definition.
=> moved oncommand from <menulist> to <menupopup type="folder" mode="filing">
It's semantically never wrong because of type="folder", even if "oncommand on menupopup" is funny.
And, oncommand on <button type="menu or menu-button"> in SearchDialog.xul was also moved to <menupopup type="folder" mode="filing">.

If it was <menu><menupopup type="folder"></menu>, oncommand on <menu> is not funny, is rather natural, for XUL creator, and oncommand on <menupoopup type="folder"> is funny.
But, above <menulist> is identical to <select> of HTML <form>.
So, <menu> is perhaps not usable, and even if usable, it's wrong because it's semantically never "Menu definition".

Fortunately, above typical use is actually for "Select" instead of "Do an action at menu or button". 
After folder selection by type="folder", selected folder is shown in <menulist>(==<select> in HTML).
So, "invoking multiple noteSelectionChange() at same time" is merely wasting of CPU power in above case.

How about Thunderbird only <folderpicker> element instead of <menupopup type="folder">?
- <folderpicker mode="???" oncommand="xxx();"> (no type="folder" attribute)
- <folderpicker> is container of hidden <menupopup type="folder" mode="???">
- spec of <folderpicker> can be freely defined => don't copy oncommand/command to hidden <menupopup>
  => no one can specify oncommand on <menupopup type="folder">
  => problem can't occur
- If parent is menulist, it's similar to menuitem for menulist.  
  If parent is menu/button, it's also similar to menuitem for menu/button.
- Less confusions than current.
- No need to rewrite code for <menupopup type="folder">
- No war of "A" vs. "B", no war of "oncommand on parent" vs. "oncommand on menupopup".
FYI.
Copies&Folders has setting for Sent, Archives, Drafts, Templates, and Junk settings has setting for Junk.
So, at least five <menulist><menupopup type="folder" mode="filing" oncommand></menulist> can be pretty easily/safely changed back 
to original <menulist oncommand><menupopup type="folder" mode="filing"></menulist>.
Trash folder selection in Server settings perhaps has similar definition.
So, six <menulist><menupopup type="folder"></menulist> can be safely changed back to original.
FYI.
Move To/Copy To in Thread pane context menu, Message menu, App menu, uses similar or same <menupopup type="folder" mode="filing"> which is semantically identical to one in SearchDialog.xul.
I don't remember whether oncommand was placed on parent or menupopup. IIRC, it was SearchDialog.xul only problem, but I'm not sure.
Unless change like following will be shortly made, 
(a) Don't copy oncommand in <menupopup type="folder" oncommand="xxxx()"> to expanded folder picker elements.
(b) Hide <menupopup type="folder"> from user by Thunderbird only <folderpicker> element.
I believe there is two options only.
(1) Do nothing, because excess CPU consumption by merely some elements is negligible. (Keep "B")
(2) Change back definition to original, because excess CPU consumption is not negligible. (Go back to "A")

Which is claim of this bug or you?
(i)  Original design allows "B", so current "B" should be respected.
     Bad is in current code for <menupopup type="folder">, so code should be corrected.
(ii) Even if current code is different from original design/intent,
     it'll take long to change code.
     (ii-1) And, we should respect current "B", so do (1).
     (ii-2) And, we are better to change back to "A", so do (2).
Or different one?
What is <folderpicker>? I can't see such a XUL element.

I have already stated my preference:
- Change everything which isn't already at "A" to "A", that is, move 'oncommand' to parent where
  possible.
- Check whether there are any "B"s that need to remain at "B".
  Document why.
- Implement a warning so no more "B"s are created. I assume that all uses where we have
  two 'oncommand's the lower one has a stopPropagation(), in which case we won't issue a warning.
(In reply to Jorg K (GMT+1) from comment #18)
> What is <folderpicker>? I can't see such a XUL element.

I wanted to say ;
 How about creating new Thunderbird only <folderpicker> element to avoid useless "A" vs. "B" war?
<menupopup type="folder"> ustilizes standard <menupopup>, but it's not simple function enhancement of standard <menupopup>.
It doesn't(or can't) support or inherit all features/functionality of standard XUL <menupopup> element.
If not, why implementation is interfered by Mac OS X and/or XBL?
If so, "avoid to use name of <menupopup>" is a clever solution :-)
(In reply to WADA from comment #19)
>  How about creating new Thunderbird only <folderpicker> element to avoid
> useless "A" vs. "B" war?
Hmm, I think XUL is an M-C thing and we won't be able to change it or make additions. Or is there a way to define your own elements?

BTW, there is no *war*. We're merely discussing the best strategy forward. As far as I'm aware there isn't even any visible problem any more, just some inconsistencies and some inefficient double-processing.
(In reply to Jorg K (GMT+1) from comment #20)
> (In reply to WADA from comment #19)
> >  How about creating new Thunderbird only <folderpicker> element to avoid
> > useless "A" vs. "B" war?
> Hmm, I think XUL is an M-C thing and we won't be able to change it or make
> additions. Or is there a way to define your own elements?

I easily thought that "simply define new tag by XML" is possible, because XUL is based on XML, but it doesn't seem so.
Syntax is defined by XML, but processing of XUL tag seems done by CPP code. It was different from XHTML.
If so, I think following is possible.
> Hide great <menupopup type="folder" mode="???"> from user,
> by <menulist/menu/button type="folder" mode="???">,
> with utilizing XXX.xml for menulist/menu/button,
> as currently done by folderWidgets.xml and XBL binding for <menupopup type="folder" mode="???>.
> What should be done by XXX.mml is merely : to generate <menupopup type="folder" mode="???"> without copying oncommand.
From perspective of XML creator, '<menulist type="folder" mode="filing" oncommand=xxx> without child element' is better 
than current <menulist><menupopup type="folder" oncommand=xxx></menulist>.
> - It's same as "no needed <option>'s, predefined/hidden <option>'s, of <select> in HTML'.
>   Predefined <option>'s of <select>(==<menulist>)
>   === folder list(folder picker) generated by folderWidgets.xml.
> - (1) <menulist oncommand=xxx> is identical to <select onclick=xxx> in HTML.
>   (2) <menuitem><menupopup oncommand=xxx type="folder"></menuitem>
>       is identical to <select>multiple <option onclick=xxx></select> in HTML.
>   If <menupopup type="folder"> is hidden, XUL user surely specifies oncommand on (1),
>   because no child is visible.
> - <menupopup type="folder"> is still available,
>   but if it's described as "for advanced or special purpose",
>   <menulist type="folder" oncommand=xxx> is usually used.
(In reply to Jorg K (GMT+1) from comment #0)
> Sadly that led to multiple executions due to the way that sub-menus of the popup are created at:
> https://dxr.mozilla.org/comm-central/rev/5aa6e96146e52ae578d36c5ac5afc246c5b77c5a/mailnews/base/content/folderWidgets.xml#514
> // Create the submenu
> // (We must use cloneNode here because on OS X the native menu
> // functionality and very sad limitations of XBL1 cause the bindings
> // to never get created for popup if we create a new element.

createElement is document.createNode, importNode is document.importNode.
But cloneNode is Node.cloneNode.
(I want feature like Node.getElementById, Node.getElenentsByTag, Node.getElenentsByAttribute, ... :-) )

document.createNode is MyWindowScope.document.createElement, and node is created under the MyWindowScope.document
document.importNode is MyWindowScope.document.createElement, and node is created under the MyWindowScope.document
Node.cloneNode is A_WindowScope.document. ... .thisnode.cloneNode, and node is created under the A_WindowScope.document.
And, appendChild(==Node.appendChild) is perhaps not "move created JavaScript node object".
Instead, it's "generate reference to created JavaScript node object", as done on ordinal JavaScript Object object.

(a) When Apple menu of Mac OS X, I think following occurs,
  MyWindowScope where this script is running != A_WindowScope where the <menupopup> node is held
because Apple menu is entity of Mac OS X instead of entity of browser window owned by Thunderbird.
(b) In this case, "reference from Apple menu environment to Thunderbird browser environment" perhaps doesn't work.
I don't think it's pretty simple limitation in XBL1 binding. Mac OS X is Alien :-)

A possible way to avoid this kind of issue is:
 Get owner document object(call DocX, held under Apple menu) of processing <menupopup> node object(call PopupX). 
 PopupX.appendChiled( DocX.createElement(...) ).

This kind of issue also occurs in Singleton environment(jsm, JavaScript module).
Window scope in sinmgleton module is different from one in browser environment.
(no preset/predefined/preallocated top level object named 'window' if singleton environment)
So, window object/document object etc. should be passed to singleton module(jsm module), if access to object of browser's WindowScope is needed by called jsm module.

If cloneNode is used, all attributes including id, oncommand, command, ..., are copied, although eventListener etc. is not cloned because of these are not "attribute of the element/node".
So, "who is responsible to remove/change id, oncommand, command etc." is always "who used cloneNode".
This is *SPEC* of cloneNode(and importNode too).

Even if limitation by Man OS X and XBL1 binding currently actually exists,
as far as code creator of folderWidgets.xml used cloneNode,
if unwanted problem for us occurs due to cloned oncommand,
at least "remove(or change) oncommand/command attribute of cloned node" should be done by who used cloneNode.
Needless to say, "cloned id attribute" is better changed or removed, even if problem due to duplicated id attribute is not exposed yet.
Note:
If duplicated id, "Node.id property of Node object in DOM" is not set, because id property of Node object should be unique in a browser or in a DOM environment.
In contrast to it, id attribute of XUL elements can have same id attribute, because XUL doesn't check uniqueness.
document.getElementById() is DOM function instead of XUL entity, so "id is unique" is guaranteed in DOM level instead of XUL level.
So, essential problem due to duplicated id attribute in XUL is "different node from code writer's expectation is returned by getElementById" or "node is not found by getElementById in someone's code" only.

I can't understand reason why Thunderbird developers hate doing both of next.
 (1) change back to "A" from "B"
 (2) remove at least oncommand/command from cloned node
Why one of two only has to be chosen?
Why BOTH is wrong or bad?
Why "(1) first then (2) later, because (2) needs testing" is wrong or bad?
I believe BOTH can pretty quickly/easily be achieved by you while you are discussing, as bug 1319299 was pretty quickly fixed :-)
For oncommand on generated elements by folderWidgets.xml for type="folder".

Following is seen in code. setAttribute/getAttribute for "command" is not seen.
> 519 var popup = this.cloneNode(false);
>  
> 528 popup.setAttribute("oncommand",
> 529                 this.getAttribute("oncommand"));

https://hg.mozilla.org/comm-central/annotate/tip/mailnews/base/content/folderWidgets.xml
Above 3 lines looks original code since initial of current folderWidgets.xml.
As for oncommand attribute, it looks copied before change to Node.cloneNode from document.createElement due to Mac OS X issue.

If so, original design/implementation was "copy oncommsand to generated child", "don't copy command to generated child".
I don't know it's intentional or not.
However, it is sure that "copied oncommand on generated child" was *SPEC* for long time, regardless of it's design/intent or not.
(but why command was not copied? code was written before command element was introduced?)
If so, "B", which was done in the past, was merely misuse of type="folder" at least after type="folder" is changed to XBL.

I believe "B" is never bad idea. It's similar to my idea of <folderpicker> element. Consider 'type="folder" menupopup' as 'Folder Picker Element'.
I believe no one wants "oncommand for I'm-selected! at each expanded/generated/hidden element" when he uses type="folder" and specifies oncommand on <menupopup type="folder">.
I believe "change code of folderWidgets.xml asap" is best choice.
I believe following is sufficient as quick/short term solution.
> 528 popup.setAttribute("oncommand",
> 529                 this.getAttribute("oncommand"));
> =>
> 528 popup.setAttribute("oncommand",null);
> 529 popup.setAttribute("command",null);
(In reply to Jorg K (GMT+1) from comment #12)
> But why don't we ask people with long-standing experience like IanN or
> Ratty.

I asked the people involved in the change in bug 878805, but you removed the requests.
So let's try again.

> Anyway, let's do the survey to see what we're talking about. How many
> "A"s do we have, how many "B"s and how many of the "B"s can't be fixed by
> churning them back to "A". Those should be left at "B" with a clear comment.
> And if we need to leave any "B"s, then clearly the warning is not a good
> idea.

Yes, that is what I said.

I'll do the stats in the next comment.


(In reply to WADA from comment #22)
> (In reply to Jorg K (GMT+1) from comment #0)
> (I want feature like Node.getElementById, Node.getElenentsByTag,
> Node.getElenentsByAttribute, ... :-) )

Try Node.querySelector() and .querySelectorAll().

> I can't understand reason why Thunderbird developers hate doing both of next.
>  (1) change back to "A" from "B"
>  (2) remove at least oncommand/command from cloned node
> Why one of two only has to be chosen?
> Why BOTH is wrong or bad?
> Why "(1) first then (2) later, because (2) needs testing" is wrong or bad?

The point of this bug is to do both and (2) is actually easier/faster to do.
Blocks: 878805
User Story: (updated)
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(alta88)
Attached patch patch for task 2 (obsolete) — — Splinter Review 

        Attachment #8814491 -
        Flags: review?(squibblyflabbetydoo)

    
    

    
  
Comment on attachment 8814491 [details] [diff] [review]
patch for task 2

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

::: mailnews/base/content/folderWidgets.xml
@@ +529,5 @@
> +                                    "showFileHereLabel", "disableServers", "position"];
> +              for (let i = attrs.length - 1; i >= 0; i--) {
> +                if (!allowedAttrs.includes(attrs[i].name)) {
> +                  // attrs is a live NamedNodeMap of the attributes on the node.
> +                  attrs.removeNamedItem(attrs[i].name);

I'd really like to see a warning here if 'oncommand' is found and its value doesn't include "stopPropagation".

    
    

    
  
FYI.

A quick history of Node.cloneNode and oncommand attribute..
Bug 516353 : changed Node.cloneNode to document.creaeElement,
 and added setAttribute from parent for some attributes including oncommand.
 https://bug516353.bmoattachments.org/attachment.cgi?id=402011
 i.e. When cloneNode was used due to Mac OS X issue, no setAttribute was used, because all attributes are cloned.
 i.e. attributes copied here is mandatory cloned attributes, except "oncommand".
=> Bug 529071 (opened on 2009-11-16, FIXED on 2009-11-17, VERIFIED on 2009-11-18)
 changed back to Node.cloneNode with adding comments on OS X which we know well,
 but it's "change back to Node.cloneNode" only.
 https://bug529071.bmoattachments.org/attachment.cgi?id=412809
 There was no deed to remove setAttribute. There was no difference when Node.cloneNode is used again.

It was pretty simple.
1. Due to Mac OS X issue, Node.cloneNode was used since initial.
   There was no need to remove any attribute including oncommand and commnad,
   because no one specified oncommand on <menupopup type="folder">.
2. One day, someone specified oncommand on <menupopup type="folder"> in SearchDialog.xul. 
   Then oncommand of all expanded/generated/hidden elements started to be fired suddenly.
3. Unfortunately, or fortunately, SearchDialog used Virtual Folder for search result, and it was "Do move mails".
   Then, problem of Bug 1110583, which is long lived problem, which was/is mystery for us, was fortunately exposed.

(In reply to :aceman from comment #24)
> (In reply to WADA from comment #22)
> > (I want feature like Node.getElementById, Node.getElenentsByTag, Node.getElenentsByAttribute, ... :-) )
> Try Node.querySelector() and .querySelectorAll().

Thanks. I'll try it.

    
    

    
  
I went over all the occurrences of type="folder" widget and I put the statistics into the user story.
User Story: (updated)

    
    

    
  
Nice work. Could you post links for
--03 of those cannot be moved to the parent because there is already another command there.
I guess one is:
https://dxr.mozilla.org/comm-central/rev/5aa6e96146e52ae578d36c5ac5afc246c5b77c5a/mail/base/content/mailWindowOverlay.xul#3128

What are the other two? Do they also have stopPropagation()?

    
    

    
  
They are all in the same file and they have stopPropagation().

    
    

    
  
Thanks, that would make the warning I asked for possible, right? A warning if 'oncommand' is ever cloned that doesn't contain "stopPropagation". That would of course mean that we go with moving the 23 back up onto the parent.

    
    

    
  
nothing further to add to what i've already stated elsewhere, so to repeat: the approach in this patch is correct, generated items (folderpicker binding) must be consistent, and xul patterns should also strive to be consistent, and handle the event bubbling problem.
Flags: needinfo?(alta88)

    
    

    
  
Ok, so you also do not see a reason the commands have to be on the menupopup.
It's either all on menupopup or all on parent.
Having all commands on the parent element instead of menupopup is consistent too.
Thanks.

    
    

    
  
See bug 1106225 comment #39:

(Quoting alta88 from bug 1106225 comment #39)
> ..., removing oncommand etc. when cloning nested folderpicker
> menupopups is fine; so is moving the oncommand to the menulist (except that
> would be more change).
> 
> What is not ok, is mixing and expecting multiple commands, one from the
> menupopup and one from the menulist/button, ...

It's all been said before ;-)

    
    

    
  
Comment on attachment 8814491 [details] [diff] [review]
patch for task 2

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

> + const allowedAttrs = ["class", "type", "mode", "fileHereLabel",
> +          "showFileHereLabel", "disableServers", "position"];

setAttribute was added when Node.cloneNode() was changed to document.createElement() once.
It was immediately changed back to Node.cloneNode() with comments on OS X what we know well.
But setAttribute() was not removed, because it never does bad thing.
I think this table can be used as mandatory attribute list when document.createElement will be used in future.

> + for (let i = attrs.length - 1; i >= 0; i--) {
> +   if (!allowedAttrs.includes(attrs[i].name)) {
> +     // attrs is a live NamedNodeMap of the attributes on the node.
> +     attrs.removeNamedItem(attrs[i].name);
> +   }
> + }

I didn't know Array.include and utilizing NamedNodeMap. I'll try it. Thanks for teaching.

Which is efficient?
> const allowedAttrsObject = { class:true,type:true,mode:true,
> fileHereLabel:true,showFileHereLabel:true,disableServers:true,position:true }
> for (let i = attrs.length - 1; i >= 0; i--) {
>   if ( !allowedAttrsObject[ attrs[i].name) ] )
>     attrs.removeNamedItem(attrs[i].name);
> }

Array is identical to {0:"d1",1:"d2",...,10:"d10"} with length property from perspective of data access.
Does Array object have internal index of data for include() etc.?
allowedAttrs is pretty small array, so I believe scan of array by include() is sufficiently efficient.
But I prefer this kind of code for existence/duplication check, because index of key is internally held when Object object.
(binary search is perhaps used)
Object object consumes larger resources than Array when pretty small?

> y

What is this "y" at last line?

    
    

    
  
(In reply to WADA from comment #35)
> setAttribute was added when Node.cloneNode() was changed to
> document.createElement() once.
> It was immediately changed back to Node.cloneNode() with comments on OS X
> what we know well.
> But setAttribute() was not removed, because it never does bad thing.

Thanks for the history. It is probable that something like that happened.

> But I prefer this kind of code for existence/duplication check, because
> index of key is internally held when Object object.
> (binary search is perhaps used)
> Object object consumes larger resources than Array when pretty small?

I don't know, but it is unsafe to use Object for this type of case. Because the Object may have some more internal attributes that you can hit by accident.
E.g. allowedAttrsObject["prototype"] could suddenly exist (even if you didn't specify it in the declaration. See e.g. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map#Objects_and_maps_compared
So I always use a JS array or a Set() which will only ever return the indexes and values you put into them, never some internal keys (attributes).

> > y
> 
> What is this "y" at last line?

Thanks, must be some typo. I'll remove it.

    
    

    
  
(Off-Topic)

(In reply to :aceman from comment #36)
> I don't know, but it is unsafe to use Object for this type of case.
> Because the Object may have some more internal attributes that you can hit by accident.
> E.g. allowedAttrsObject["prototype"] could suddenly exist (even if you didn't specify it in the declaration.
> See e.g.
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map#Objects_and_maps_compared
> So I always use a JS array or a Set() which will only ever return the indexes and values you put into them,
> never some internal keys (attributes).

I didn't know Set/Map object, Object.create(null), (for of) iterator etc. and dark side of Object object.
And I didn't know (+0===-0)==true but (+0==-0)==false. (spec seems changed to (+0==-0)==true)
Thanks for teaching again.

Code like next is better in near future?
> const allowedAttrsSet = new Set( [ "class","type","mode", ... ,"position" ] ) ;
> for (let i = attrs.length - 1; i >= 0; i--) {
>   if ( !allowedAttrsSet.has(attrs[i].name) )
>     attrs.removeNamedItem(attrs[i].name);
> }
Or still allowedAttrs.includes(attrs[i].name) is better in such small array case because efficient/faster?
(at least hashing is requested for Set/Map. if pretty small array, array scan is perhaps faster than hashing)

I prefer Obj[xxx] notation because I can use Object object as associative array.
So, I want to use var AssocArray=Object.create(null).
What is dark side of var mymap=Object.create(null) compared with var mymap=new Map()?
Needless/excess resource consumption, performance etc.?
Is Object.create(null) not recommended in Thunderbird code if Set/Map is sufficient or appropriate?

    
    

    
  
(In reply to WADA from comment #37)
> Or still allowedAttrs.includes(attrs[i].name) is better in such small array
> case because efficient/faster?
> (at least hashing is requested for Set/Map. if pretty small array, array
> scan is perhaps faster than hashing)

I haven't tested it, but for such small number of items I'd use an array.

> I prefer Obj[xxx] notation because I can use Object object as associative
> array.
> So, I want to use var AssocArray=Object.create(null).
> What is dark side of var mymap=Object.create(null) compared with var
> mymap=new Map()?
> Needless/excess resource consumption, performance etc.?
> Is Object.create(null) not recommended in Thunderbird code if Set/Map is
> sufficient or appropriate?

Map() is also an associative array and is designed for this kind of use. With Object there may be other surprises in the future (as JS develops). You just do not need to think/worry about anything with Map().

    
    

    
  
(In reply to Jorg K (GMT+1) from comment #31)
> Thanks, that would make the warning I asked for possible, right? A warning
> if 'oncommand' is ever cloned that doesn't contain "stopPropagation". That
> would of course mean that we go with moving the 23 back up onto the parent.

Yes, then the warning would be possible. But why would it be useful? If we fix the cloning (with my patch) why is the code with the warning useful? It will never get run. Or is that defense against a bug somebody will introduce in the future so that command gets cloned again? In that case would could maybe make a mozmill test for this.

    
    

    
  
(In reply to :aceman from comment #39)
> Yes, then the warning would be possible. But why would it be useful? If we
> fix the cloning (with my patch) why is the code with the warning useful? It
> will never get run. Or is that defense against a bug somebody will introduce
> in the future so that command gets cloned again? In that case would could
> maybe make a mozmill test for this.
The warning was meant for future developers so we guarantee consistency. Something like:

*** 'oncommand' should be placed onto the parent unless the
*** there is a specific reason to place it on the child. In
*** this case, did you forget to add stopPropagation()?

Are you going to submit a patch to move those 23 up?

    
    

    
  
(Off-Topic)

(In reply to :aceman from comment #36)
> So I always use a JS array or a Set() which will only ever return the indexes and values you put into them,
> never some internal keys (attributes).

Oh, Array.prototype.includes() was feature of next ES6.x/ES7.
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_Next_support_in_Mozilla
Using such more modern feature in Thunderbird code is fault play. Red card!!!
If spec of ES7 will be changed and includes() will be removed or function name will be changed to Includes() as done on QuerySomething/querySomething, you have to silently change your code to Set or Map by yourself :-)

By the way, can I add support of "[" && "]" notation to Set/Map by merely adding my simple code to prototype of Set/Map?
I want "[" && "]" notation instead of set()/get(), although I can afford to "for of MyMap.keys()" as alternative of "for in MyObj".
If possible, I can completely stop using new Object()/Object.create(null) for new Map()/new Set() :-)

(Non-Off-Topic)

Anyway, please land your patch asap. If slightly risky, please provide us tryserver build asap.

    
    

    
  
(In reply to Jorg K (GMT+1) from comment #40)
> The warning was meant for future developers so we guarantee consistency.
> Something like:
> *** 'oncommand' should be placed onto the parent unless the
> *** there is a specific reason to place it on the child. In
> *** this case, did you forget to add stopPropagation()?

If aceman's patch is applied, oncommand can be specified on any of Parent and <menupopup type="folder">, because oncommand won't be copied any more.
oncommand is kicked at one of Parent and <menupopup type="folder"> only.
So, no problem won't occur in this case.
Almost all cases is this type, and oncommand is placed on Parent in many cases..

Problematic "--03" cases.
  Same function is used in oncommand of Parent and <menupopup type="folder">.
  Parent : A_Function(); (after bug fix by aceman)
    <menupopup type="folder"> : A_Function(event.target._folder); event.stopPropagation();
      Some <munuitem command=xxx> under menupopup.
(1) When clicked at folder picker
(1-a) Current : oncommand is kicked at "selected folder of folder picker" only, because stopPropagation() is copied.
(1-b) After patch : oncommand is not copied, so oncommad is kicked at <menupopup type="folder">,
      and click event is not passed to Parent because stopPropagation is specified by aceman.
(2) When clicked at Button for "Get new messages".
Problem won't occur because function call is changed to A_Function() by aceman.
(3) When clicked at other <menuitem> under <menupopup type="folder">.
(3-a) Current : command on <menuitem> is kicked and oncommand on <menupopup type="folder"> is kicked. In this case, "undefined" occurs at menupopup due to A_Function(event.target/_folder), but called A_Function does do nothing when event.target/_folder is undefined. So no problem occurs except warning on "undefined". And, because stopPropagation() is specified, event is not passed to Parent.
(3-b) After patch : No difference from current, because click is done at <menuitem>

What aceman is trying in this case is:
After patch, sort out the A_Function used by these elements, and change to clean XUL definition.
For example,
  oncommand="A_Function(event);". It can be placed on any of Parent and menupopup.
  In the A_Function, if(!event.target._folder) return; Action_Module(event.target._folder);
In this case, because aceman's patch is applied, oncommand works well at any of Parent and menupopup.
However, for consistent definition with many other elements, or because original was on Parent, he wants to move back to Parent.
I don't care about such thing.
If user wants oncommand on Parent, write in Parent, and if user wants oncommand on menupopup, write in menupopup.
That's all :-)

Problem is in following case:
  Parent oncommand=FunctionA(...)
    <menupopup type="folder" oncommand="FunctionB(...)>
       <menuitem oncommand="FunctionC(...)>
       <menuitem oncommand="FunctionD(...)>
In such case, it's usually intentional, so there is no need to care about Function(event.target._folder) like error.
If user knows well about "oncommand is copied to generated/expanded/hidden elements",
and if user designed FunctionB like next,
   if(event.currentTarget==event.target, i.e. I'm selected folder) do something#1;
   else if(event.currentTarget is parent folder, i.e. I'm parent) do something#2
   else if(event.currentTarget is grandpa folder, i.e. I'm grandpa) do something#3
   else if ...
   else if(event.currentTarget==menupopup, i.e. I'm menupopup) do somethingAtMenupopup
expected "do something#n" won't be executed after aceman's patch, because oncommand is not copied.
So, warning or guide may be needed for such user.
However, this is folder picker to select one folder.
Who will do or need such implementation?
Who will expect that oncommand is kicked at each generated/expanded/hidden element of folder picker?
Why stopPropagation is needed?
If stopPropagation is needed after patch in this case, it's merely mistake in function design/implementation/XUL definition.
  To bypass error due to wrong implementation in FunctionA on Parent,
  issue stopPropagation() at <menupopup> in order not to pass event to Parent.
  (this is what was done by aceman when he used stopPropagation to resolve bug)

    
    

    
  
(In reply to WADA from comment #42)
> Why stopPropagation is needed?
> If stopPropagation is needed after patch in this case, it's merely mistake
> in function design/implementation/XUL definition.

Maybe, but we can't fix that mistake (if it is one).

>   To bypass error due to wrong implementation in FunctionA on Parent,
>   issue stopPropagation() at <menupopup> in order not to pass event to
> Parent.
>   (this is what was done by aceman when he used stopPropagation to resolve
> bug)

Yes. Even after all the fixes we will do here, in the future somebody may want one command on the parent and another on the menupopup and then he will have to use stopPropagation(). But there is nothing wrong with using it.

    
    

    
  
(In reply to :aceman from comment #43)
> Even after all the fixes we will do here, in the future somebody may
> want one command on the parent and another on the menupopup and then he will
> have to use stopPropagation(). But there is nothing wrong with using it.

There is nothing bad in following example.
> <Parent oncommand=FunctionA(...)>, with or without stopPropagation
>   <menupopup type="folder" oncommand="FunctionB(...)>, with or without stopPropagation
>      <menuitem oncommand="FunctionC(...)>, with or without stopPropagation
>      <menuitem oncommand="FunctionD(...)>, with or without stopPropagation
When clicked at menuitem, click at menuitem is propagated in bubble phase : menuitem -> menupopup -> Parent, as defined by W3C event model.
When clicked at folder picker generated by menupopup, click at folder picker is propagated in bubble phase : selected folder -> parent folder -> ... -> menupopup -> Parent, as defined by W3C event model.
If oncommand is needed at an XUL element to handle event, write it. If oncommand is not needed at an XUL element to handle event, don't write it. That's all.
All is up to function/XUL element designer. Designer can use stopPropagation to reduce workload of function code writing. No need to write code for "Don't care" case.
And, developer, who has to fix bug due to unknown or not-well-implemented or stupid FunctionA, can add stopPropagation on menuitem and can correct bug in FunctionX. All is up to developer who has to resolve problem.
Sorry but I don't know about "command" case. Can both command and oncommand be requested at an XUL element?
"command" is shared by many elements, so stopPropagation in script of "command" may produce unwanted problem".
Anyway, W3C event model, stopPropagation etc. are clearly defined by W3C.
And, stopPropagation is utilized in many places.
  When link click, page is changed to different one, so further action at higher level is not needed,
  or it may produce excess problem. In such case, stopPropagation is one of best practice.
  When <form action=...> <a href="...">...</a> </form> and link is clicked,
  Firefox invoked both "http get for link" and "http get/post for form", so funny phenomenon was observed :-)
And, once your patch is landed, no one has to care about invisible/hidden/silently-generated XUL elements.
Why who has to write/maintain folderWidgets.xml code should care about oncommand use by XUL element/function designer?
Why warning or guide is needed about stopPropagation for XUL element user?

    
    

    
  


  
(1) Install "Custom Buttons" addon, add a Toolbar button
(2) Copy&Paste attached script to the button
(3) Open Error Console
(4) Click the button => event listener is added
(5) Click "Get Messages" of Toolbar and select account
    => log is written to Error Console by event listener

id of -03 problematic elements are already held in script.
Please add/remove/modify id in source code.
removeEventListener doesn't work well. So, please restart Thunderbird after test.

I gave up to use Object.create(null). I used Set/Map, despite that bracket notation use is impossible because of "Set/Map has no property".

    
    

    
  


  
Button script V2 :
event.bubbles, event.cancelBubble, event.cancelable etc. are added to report by click event listener, for ease of understanding what happens.

Toolbar button of "Get Messages" consist of ;
- "Get Messages" text part for this button itself
- triangle mark part to expand menupopup under the button
So, click event can be initiated at following.
(A) Button element(Parent) : "Get Messages" text part of this button itself
(B) Button element(Parent) : triangle mark part to expand menupopup under the button
(C) Folder picker element expanded/generates by <menupopup type="folder">
(D) <menuitem> element under the <menupopup type="folder"> ("Get All New Messages")

If you do following,
> (1) Restart Thunderbird, open Error Console, clear Error Console
>     click button and add event listener, clear Error Console,
>     and do (A)
you can see next error.
> Warning: ReferenceError: reference to undefined property event.target._folder
> Source File: chrome://messenger/content/messenger.xul Line: 1"
Because above error message is shown only once after restart of Thunderbird, error message is not shown when ((A) is repeated or other element is clicked.
Reason why error is still shown.
  oncommand="A_Not_Well_Implemented_Function(event.target._folder);" exists on <menupopup type="folder">
Above error may be shown by Just In Time Compiler, Singleto module processor, JavaScript interepreter etc.

To Jorg K:
One of biggest reason why I provided the Button script is :
  Let you know such thing and W3C event model.
  note: another big reason == Show evidence of "I now can use Set/Map" to aceman :-)
Please try the Button script and see log in Error Console.

To aceman:
I can't understand reason why click event listener of <menupopup type="folder"> is invoked even when "event.stopPropagation() on menupopup" is copied to folder picker elements and the event.stopPropagation() is executed at clicked folder picker element.
It may be due to event.cancelBubble=false.

        Attachment #8815961 -
        Attachment is obsolete: true

    
    

    
  


  
Sorry, I forgot that click, enter etc. were consolidated to "command" event appropriately in XUL.

V3 : eventListener for "command" event is also added.

Click event is propagated and processed, and the click event is changed to "command" event and propagated.
When "command" event arrived, I believe following is done,
  if "command" is specified, do oncommand on "command" element;
  else if "oncommand" is specified, invoke the oncommand;
instead of
  if "command" is specified, do oncommand on "command" element;
  if "oncommand" is specified, invoke the oncommand;

        Attachment #8816162 -
        Attachment is obsolete: true

        Attachment #8816179 -
        Attachment description: Button script V3, to see event object content when command event(and click event) is propagated from element under Toolbar button of "Get messages" R → Button script V3, to see event object content when command event(and click event) is propagated from element under Toolbar button of "Get Messages"

    
    

    
  
(In addition to my comment #46)
> It may be due to event.cancelBubble=false.

To aceman:

> https://developer.mozilla.org/en-US/docs/Web/API/UIEvent/cancelBubble
Non-standard/Deprecated UIEvent.cancelBubble is still supported by Mozilla perhaps because it's needed for implementation.
Although stated "Use standard stopPropagation instead", it's perhaps effective at many places in XUL support of Mozilla.

    
    

    
  


  
V4 :
eventListener is added to each generated/hidden menuitem/memu/menupopup,
in order to see structure like next in eventListener log for "command event".
> menupopup (oncommand is specified)
>   If other XUL elements are defined under this <menupopup> by user, they are placed here.
>   menu <= root folder as container(account#1), <menuitem> if account selection
>     menupopup (oncommand is copied)
>       menu <= 1st top level folder as container(if no sub folder, <menuitem> to select folder)
>         menupopup (oncommand is copied, id is also copied)
>           menuitem <= top level folder as selected folder(file here)
>           menu     <= subfolder as container
>             menupopup (oncommand is copied, id is also copied)
>               menuitem <= subfolder as selected folder(file here)
>               menu     <= subsubfolder as container(if no sub folder, <menuitem> to select folder)
>                 ... <menu> is nested by <menupopup> until folder has no subfolder
>       menu <= 2nd top level folder as container(if no sub folder, <menuitem> to select folder)
>       menu <= 3rd top level folder as container(if no sub folder, <menuitem> to select folder)
>   menu <= root folder as container(account#2), <menuitem> if account selection
>   menu <= root folder as container(account#3), <menuitem> if account selection
This "eventListener for each generated/hidden menuitem/memu/menupopup" is done only on following <menupopup> element.
> id="button-getMsgPopup" (Toolbar button, Get Messages)
> id="actionFolderPopup"  (Junk Settings, Junk folder selection at Other:)
> <menupopup> under button id="fileMessageButton" (File button of SearchDialog)

New findings while changing code.
(1) oncommand is copied to generated <menupopup> only, is not copied to <menu>/<menuitem>, because cloneNode is done on each node type, and onmcommand is specified on <menupopup> only..
(2) UIEvent.cancelBubble=true was set after stopPropagation() by aceman. I thought patch of bug 989090 was already landed on Thunderbird 45, but it was wrong. Sorry for my confusion.
(3) If "other <menuitem command=CMD> under <folder type=folder oncommand=A_function>(xxx)" is clicked, "command event" is not propagated to this <folder type=folder oncommand=A_function(xxx)>.
i.e. 
 "command attribute" may be invoked during propagation of "click event", and if "command" is invoked, it may be that "oncommand event" is not generated.
Or, "command event" is propagated at different event propagation path where the used "command" is held.
Or, "command=xxx attribute" may be "create same event at <command id=xxx> upon click, Enter etc.".
And, when "Get Messages", <menupopup>" is not generated because this is for "account selection". Top level <meniitem> for account only is generated under the defined <folder type=folder oncommand=A_function>(xxx)".
(4) "id" is also cloned to generated <menupopup> currently. It's pretty confusing and it's pretty hard to distinguish many <menupopup>s. The "confusing" part will be removed by aceman's patch, because "id" attribute is not copied, and issue of "duplicated id attribute" will be resolved.
(5) "id=folderURI of this element" is set only in "<menuitem> for folder selection". "id attribute of <menu> as subfolder container" is not set. "event.target._folder" == msgFolder of selected folder.

Because of (4)/(5), when "oncommand of generated/expanded <menupopup>" is invoked, it's almost impossible to know "Who I am", "I'm working for which container folder(parent, grandpa, ...,) of selected folder".
Therefore, "try to do something by copied oncommand to generated/expanded <menupopup> at a folder hiearchy level for the folder hiearchy level" is almost nonsense.
Therefore, "don't copy oncommand to generated/expanded <menupopup>" is absolutely correct and required and mandatory action.

        Attachment #8816179 -
        Attachment is obsolete: true

    
    

    
  
Quick Summary of current <menupopup type=folder> use.
> (1) Many of them : oncommand in <Parent>, no oncommand in <menupopup type=folder>
> (1-a) majority of (1) : <menupopup type=folder> only is defined under <Parent>
> (1-b) some of (1)     : other elements like <menuitem> are defined under the <menupopup type=folder>
> (2) No oncommand in <Parent>, oncommand in <menupopup type=folder> only
>     oncommand was moved from <parent> to <menupopup type=folder> by consolidation work,
>     or is placed on <menupopup> since initial by XUL user or was moved by someone else.
> (2-a) majority of (2) : <menupopup type=folder> only is defined  under <Parent> (Copies&Folders, Junk Settings)
> (2-b) some of (2)     : other elements like <menuitem> are defined under the <menupopup type=folder>
> (2-c) (2-a) in SearchDialog.xul : already moved back to Parent by Bug 1319299
> (3) Elements modified by bug 989090 in order to bypass problem due to not-well-implemented function/XUL definition.
>     Same function in oncommand of <Parent> and <menupopup type=folder>, but passed parameter is different.
>     Well known elements for us by "stoPropagation use".
>     One or more elements such as <menuitem> are defined under the <menupopup type=folder>

In any case, there is no need to do something on above elements after aceman's patch by this bug will be landed.
"Move back to parent", "move to menupopup", "modify function/XUL definition", are all up to you, developers.
I don't think "discussing about action on above elements" is needed in this bug.
For (3), "open bug for removing bad event.target._folder from XUL definition" is sufficient.
If you want "oncommand on <Parent> instead of on <menupopup type=folder>" in all definitions, please open separate bug for it.

FYI.
Even if excess execution of copied oncommand of generated/expanded menupopup happened, after problem of Bug 1319299 is fixed, majority of it is only at (a) folder picker of Server Settings/trash folder selection, (b) folder picker of Other: of Copies&Folders/Junk Settings".
Even if oncommand is executed multiple times when deep level folder is selected by user, it merely updateds "selected folder display" multiple times, and user usually never selects folder placed at deep level for trash, sent, drafts, archives, templates, junk, and frequency of setting change is pretty low.
And, in (3), "excess execution of copied oncommand of generated/expanded menupopup" never occurs.
Because it's for account selection(no-expand), there is no "generated/expanded menupopup" in this case.

    
    

    
  
(Off-topic for resolving this bug)

I saw following known issue again after I knew about folder picker by this bug.
  When sub folder is created at a messenger window, it's not shown by folder picker in such as message filter.
This is worked around by "open new messenger window" and "request folder selection at the new messenger window".

It indicates that "Object relevant to folder structure in messenger which is referred by folder picker" is not correctly updated after creation of sub folder(s).

To aceman:

Whose problem? Who created sub folder? Folder picker holds reference to old version of the relevant object?
 ObjectX(v1) is for folder structure. Folder picker holds reference to ObjectX(v1).
 By sub folder creation, ObjectX is replaced by ObjectX(v2), then reference count of ObjectX(v1) is reduced by 1.
 Folder picker still have reference to ObjectX(v1), so ObjectX(v1) is not destroyed or purged by garbage collector.

From which object/how is folder picker structure generated?

    
    

    
  
(Off-topic for resolving this bug)

(In addition to my comment #51)
In OnItemAdded: function act_add(aRDFParentItem, aItem), when new subfolder is created, aRDFParentItem==Where sub folder is created(instance of nsIMsgFolder), and aItem==created sub folder(instance of nsIMsgFolder).
aRDFParentItem is already known folder, but aItem is "Not-known-yet folder" because it's newly created folder, and folder event listener is not regitered yet for the newly created sub folder.
When OnItemAdded is invoked, _teardown is called, but _teardown looks to do nothing on the aItem(==newly creared subfolder).
I guess this is a cause of "newly created folder is not shown" phenomenon.
Sooner or later, re-scan of all folders will be done at somewhere by someone, so problem is not consistent. This is perhaps a 
reason why problem analysis was hard. No one can get data such as trace log.
Note: "OnItemRemoved + _teardown", which is same code as OnItemAdded, perhaps works well, because aItem(removed folder) is known folder and required action is remove from menu.

I'll search bug for "newly created folder is not shown" phenomenon.

    
    

    
  
FYI All:  I was about to file the following when I discovered this current bug and several related items...

I'm running the TB standard public release 45.5.1 and I get this problem.  When searching and then moving selected emails to a destination folder, the selected emails move "correctly" -- message content & format OK, with heading display info tagged and bolded (or not) as per original.  However, contrary to what others seem to see, I do NOT get any duplicates of existing messages being moved, nor are any pre-existing messages in the destination folder duplicated.

I DO get multiple instances of zero-byte emails ("phantom", I guess you're calling them), all in account "Local Folders", all dated (and received) "31/12/1969 7:00 PM".  There are many (three, seven, etc) and the count does not relate to the number of items being moved.  

The fact of searching/moving/phantom issue is always repeatable, but I can't say whether the counts are consistent or random for any given move.

    
    

    
  
Dan, the bug you're encountering is already fixed, see bug 1319299. This will eventually ship in a version of TB 45.x, most likely TB 45.7.

    
    

    
  
Thanks, Jorg.

And of course my reported date stamp of 1969 which looks so different from the reported 1970 is only -5hrs off GMT New Years, which is correct for my location (EST).

    
    

    
  
(In reply to WADA:World Anti-bad-Duping Agency from comment #41)
> Oh, Array.prototype.includes() was feature of next ES6.x/ES7.
> > https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_Next_support_in_Mozilla
> Using such more modern feature in Thunderbird code is fault play. Red card!!!
> If spec of ES7 will be changed and includes() will be removed or function
> name will be changed to Includes() as done on QuerySomething/querySomething,
> you have to silently change your code to Set or Map by yourself :-)

There is no mention at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes that Array.includes() would be experimental feature. We use it at so many places that it is not worth worrying about it here.

    
    

    
  
How do we move here?

To summarize:
I'm not a fond of moving the commands from menupopup to parent, as we have all the properties for the picker binding on the menupopup so why not also the command. If all commands would be on the menupopup and we programmatically DO NOT copy it to the children menupopups, we would not allow the usecase when somebody in the future would want the menupopups copied. But I can't imagine such a case. If the chosen leaf folder gets passed to the oncommand code, it can choose to such something on all the parent folders. You do not need this top-heading traversal to be done for you by the levels of oncommands attached to the menupopups AND hoping that all of them really get run (currently they do). Also in this case there would not be the warning asked for by Jorg as it would not be a mistake to place command on menupopup. Also using stopPropagation() would be dependent on whether the parent element has its own command.

On the other hand, the picker widget does nothing with the command so it is questionable if it should be considered a unit together with the other attributes. BUT if there are cases where we want to have different command on the parent than on the menupopup, we have to leave these on the menupopup. Then these occurrences would be inconsistent with all the other in the tree.

Anyway, assuming we want the placing of command consistent (either on menupopup or the menulist) AND that all the parent elements there are will a execute a command, the choice is between:

1.moving 26 occurrences up (supported by Jorg)
2.moving 13 occurrences down (supported by aceman)

Alta is fine with any solution (even though I claim option 1 can't ever be consistent in all cases (when menulist must have different command)). I'm not sure which one WADA liked.

Squib, can we get your opinion now?

    
    

    
  
Given that regular (non-folder) <menupopup>s don't appear to have an oncommand attribute in the docs[1] (but <menulist>s do[2], as do <toolbarbutton>s and - kinda - <button>s[3]), I think it's more consistent to move oncommand "up".

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/menupopup
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/menulist
[3] <button>s don't list oncommand in their attributes but there's an example showing it in use.
Flags: needinfo?(squibblyflabbetydoo)

    
    

    
  
Comment on attachment 8814491 [details] [diff] [review]
patch for task 2

r- per my previous comment.

        Attachment #8814491 -
        Flags: review?(squibblyflabbetydoo) → review-

    
    

    
  
(In reply to Jim Porter (:squib) from comment #59)
> Comment on attachment 8814491 [details] [diff] [review]
> patch for task 2
> 
> r- per my previous comment.

I think we need a form of the patch in any case. Even if we decide here to move all commands to parent element. Somebody in the future may place it on menupopup again (and repeat the dataloss bugs again). So we need the protection against that.

    
    

    
  
OK, I'll make the patch for task 1 now.
User Story: (updated)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch for task 2 v1.1 (obsolete)
        — 
          — Splinter Review
      

    


  
I still think this is needed in case somebody puts command on the menupopup in the future. Do we want a warning in that case?

        Attachment #8814491 -
        Attachment is obsolete: true

        Attachment #8844257 -
        Flags: review?(squibblyflabbetydoo)

        Attachment #8844257 -
        Flags: feedback?(jorgk)

    
    

    
  


  
Moves all commands to the parent element.
Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c4919d1709b3b66ac2d156d00693cf1600c0af85

        Attachment #8844258 -
        Flags: review?(squibblyflabbetydoo)

    
    

    
  


  

        Attachment #8844259 -
        Flags: review?(philip.chee)

    
  
Status: NEW → ASSIGNED

    
  

        Attachment #8844259 -
        Flags: review?(iann_bugzilla)

    
    

    
  
Comment on attachment 8844257 [details] [diff] [review]
patch for task 2 v1.1

(In reply to :aceman from comment #63)
> I still think this is needed in case somebody puts command on the menupopup
> in the future. Do we want a warning in that case?
Me personally, yes (and I said so a few times). So since what I asked for is not in the patch, I'll remove the f? since f- is not nice to give ;-) You have my feedback anyway.

Aceman, great job of getting back to this, it had already slipped my mind.

        Attachment #8844257 -
        Flags: feedback?(jorgk)

    
    

    
  
So as there is currently no usage of command on the menupopup the warning can be done. On the other hand, it is not an error if somebody puts the command on the menupopup. With the current patch, no harm should be done, no multiple execution. So it is merely coding style warning, or that attribute "sanitization" may get wrong again in the future so it is better if no commands on menupopup get suddenly exposed and duplicated.

So I'll see how to produce such info-level message.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch for task 2 v1.2 (obsolete)
        — 
          — Splinter Review
      

    


  
Added warning message.

        Attachment #8844257 -
        Attachment is obsolete: true

        Attachment #8844257 -
        Flags: review?(squibblyflabbetydoo)

        Attachment #8845194 -
        Flags: feedback?(jorgk)

    
    

    
  
Comment on attachment 8845194 [details] [diff] [review]
patch for task 2 v1.2

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

Thanks for adding this.

::: mailnews/base/content/folderWidgets.xml
@@ +315,5 @@
> +          // but still warn about the usage on menupopup. Command should be placed
> +          // on the parent element.
> +          if (this.hasAttribute("command") || this.hasAttribute("oncommand")) {
> +            console.info("It may not be safe to use command attribute on menupopup " +
> +                         (("id" in this)?("id=" + this.id):"") +

Hmm, don't we put spaces around the "?" or ":" ?
The message will look funny when "" is evaluated. Can that happen? Maybe also print the value of "command" or "oncommand".

        Attachment #8845194 -
        Flags: feedback?(jorgk) → feedback+

    
    

    
  
(In reply to Jorg K (GMT+1) from comment #69)
> > +                         (("id" in this)?("id=" + this.id):"") +
> 
> Hmm, don't we put spaces around the "?" or ":" ?
Yes, ok.

> The message will look funny when "" is evaluated. Can that happen?
What do you mean? "" evaluated? I think appending empty sting to the surrounding strings will have no effect.

> Maybe also print the value of "command" or "oncommand".
That seems overkill to me. Even printing the ID is non-standard bonus in these lands :)

    
    

    
  
(In reply to :aceman from comment #70)
> > The message will look funny when "" is evaluated. Can that happen?
> What do you mean? "" evaluated? I think appending empty sting to the
> surrounding strings will have no effect.
If |(("id" in this)?("id=" + this.id):"")| evaluates to "", your resulting message will have two spaces:
"It may not be safe to use command attribute on menupopup " + "" + " of type='folder'. See bug 1319930."

So maybe not use "" but "(no id)" or similar. I was just a nit.

Or make it:
"It may not be safe to use command attribute on menupopup" + (("id" in this)?(" id=" + this.id):"") + ...

    
    

    
  


  
Yes, good catch with the double spaces.

        Attachment #8845194 -
        Attachment is obsolete: true

        Attachment #8845678 -
        Flags: review?(squibblyflabbetydoo)

    
    

    
  
Comment on attachment 8844259 [details] [diff] [review]
patch for task 1 - SM

Assuming the main patch gets and r+ r=me

        Attachment #8844259 -
        Flags: review?(philip.chee)

        Attachment #8844259 -
        Flags: review?(iann_bugzilla)

        Attachment #8844259 -
        Flags: review+

    
  
No longer blocks: 878805
Depends on: 802609, 473009, 878805

    
    

    
  
Comment on attachment 8845678 [details] [diff] [review]
patch for task 2 v1.3

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

Clearing review here, since I've forgotten most of what I know about the TB codebase and wouldn't be able to give a proper review. Sorry about that... :(

        Attachment #8845678 -
        Flags: review?(jporter+bmo)

    
    

    
  
Comment on attachment 8844258 [details] [diff] [review]
patch for task 1 - TB

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

Ditto.

        Attachment #8844258 -
        Flags: review?(jporter+bmo)

    
  
Severity: normal → S3

    
    

    
  
Obsolete with version 115


Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → INVALID

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: