1017408 - Both command & oncommand attribute is set in some toolbarbutton/meuitem's. command element shouldn't be observed by observes(id_of_command). Use command attribute only, or if needed, use <broadcaster>/<observes> for attributes of required elements.

Status: NEW

(Thunderbird :: Mail Window Front End, defect)

Description

[WADA:World Anti-bad-Duping Agency]

Both command & oncommand is specified for some toolbarbuttons.
Intentional?
If not intentional, oncommad should be removed.

Following is elements(menuitem and toolbarbutton only) which returned string to both element.getAttribute("command") and element.getAttribute("oncommand") .
Checked messenger window

> Messenger
> Both command & oncommand is specified for toolbarbutton(id=appmenu-cut) command = cmd_cut oncommand = goDoCommand('cmd_cut')
> Both command & oncommand is specified for toolbarbutton(id=appmenu-copy) command = cmd_copy oncommand = goDoCommand('cmd_copy')
> Both command & oncommand is specified for toolbarbutton(id=appmenu-paste) command = cmd_paste oncommand = goDoCommand('cmd_paste')
> Both command & oncommand is specified for toolbarbutton(id=button-chat) command = cmd_chat oncommand = goDoCommand('cmd_chat')
> Both command & oncommand is specified for toolbarbutton(id=qfb-show-filter-bar) command = cmd_toggleQuickFilterBar oncommand = goDoCommand('cmd_toggleQuickFilterBar');
> Both command & oncommand is specified for toolbarbutton(id=ElementWithoutID_1) command = cmd_goBackSearch oncommand = goDoCommand('cmd_goBackSearch')
> Both command & oncommand is specified for toolbarbutton(id=ElementWithoutID_1) command = cmd_goForwardSearch oncommand = goDoCommand('cmd_goForwardSearch')
> Both command & oncommand is specified for menuitem(id=mailContext-printpreview) command = cmd_printpreview oncommand = goDoCommand('cmd_printpreview')
> Both command & oncommand is specified for menuitem(id=mailContext-print) command = cmd_print oncommand = goDoCommand('cmd_print')

> MessageWindow
> Both command & oncommand is specified for toolbarbutton(id=appmenu-cut) command = cmd_cut oncommand = goDoCommand('cmd_cut')
> Both command & oncommand is specified for toolbarbutton(id=appmenu-copy) command = cmd_copy oncommand = goDoCommand('cmd_copy')
> Both command & oncommand is specified for toolbarbutton(id=appmenu-paste) command = cmd_paste oncommand = goDoCommand('cmd_paste')
> Both command & oncommand is specified for toolbarbutton(id=button-chat) command = cmd_chat oncommand = goDoCommand('cmd_chat')
> Both command & oncommand is specified for menuitem(id=mailContext-printpreview) command = cmd_printpreview oncommand = goDoCommand('cmd_printpreview')
> Both command & oncommand is specified for menuitem(id=mailContext-print) command = cmd_print oncommand = goDoCommand('cmd_print')

> Composer
> Both command & oncommand is specified for toolbarbutton(id=button-send) command = cmd_sendButton oncommand = goDoCommand('cmd_sendButton')
> Both command & oncommand is specified for toolbarbutton(id=spellingButton) command = cmd_spelling oncommand = goDoCommand('cmd_spelling')
> Both command & oncommand is specified for toolbarbutton(id=button-attach) command = cmd_attachFile oncommand = goDoCommand('cmd_attachFile')
> Both command & oncommand is specified for toolbarbutton(id=button-save) command = cmd_saveDefault oncommand = goDoCommand('cmd_saveDefault')

Comment 1

[WADA:World Anti-bad-Duping Agency]

Checked messenger, messagewindow, composer only.

Comment 2

[:aceman]

That is interesting. Fortunately it looks like both attributes specify the same command ID. But what would happen if they differed? Which one takes precedence? Or both are executed?

Comment 3

[[e-mail]]

If the original target of a XUL command event is a XUL element with a command= attribute then a new event is generated to target the command element and the original event does not get dispatched, so no oncommand= runs for the original target or its ancestors.

Comment 4

[WADA:World Anti-bad-Duping Agency]

I set "BookmarkingUI.onCommand(event);" or "PlacesCommandHook.showPlacesOrganizer('AllBookmarks');"  in oncommand, and set "Tools:Addons" in command of a Toolbar button of Firefox, by utilizing "Custom Buttons" addon. Firefox always opened Addon Manager in a Tab.
command took precedence as  neil explained.

Comment 5

[:aceman]

OK, then let's try to remove the oncommands.

Comment 6

[:aceman]

The problem is none of the oncommand attributes are found in the source files. I suspect they get onto the elements at run time by some other mechanism. E.g. there is command=cmd_cut on a button (e.g. id="appmenu-cut"). Then oncommand=goDoCommand("cmd_cut") is set automatically on the <command id="cmd_cut"> and that attribute get automatically copied to the id="appmenu-cut" button via the observation mechanism. So I don't know now if that is actually any problem. Also I do not understand why some buttons get this oncommand and some not.

Comment 7

[WADA:World Anti-bad-Duping Agency]

Reason why I checked about "both command & oncommand" is that I saw "both command & oncommand" in DOM Inspector's display of some elements but I couldn't find ."both command & oncommand" in XUL definition source.

For next phenomenon.
> Both command & oncommand is specified for menuitem(id=mailContext-printpreview)
>         command = cmd_printpreview oncommand = goDoCommand('cmd_printpreview')
> Both command & oncommand is specified for menuitem(id=mailContext-print)
>         command = cmd_print oncommand = goDoCommand('cmd_print')

Symptom like next is relevant?
> DupID : newNewMsgCmd : menuitem(parent=menupopup/id=appmenu_newMenupopup),
>                                            menuitem(parent=menupopup/id=menu_NewPopup)
XUL overlay is executed based on "ID of element". So, result is unpredictable if duplicated ID is defined.

Comment 8

[[e-mail]]

Well that one's easy - it's got both command= and observes= for some reason. I don't know why Standard8 added it in bug 498018, there's no mention of it there.

Comment 9

[WADA:World Anti-bad-Duping Agency]

I also have found it.
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#835
<menuitem id="mailContext-print"> had  observes="cmd_print" and command="cmd_print".

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/Broadcasters_and_Observers#Making_elements_observers
> Broadcasters aren't used frequently as commands can generally handle most uses.
> One thing to point out is that there really is no difference between the command element and the broadcaster element.
> They both do the same thing. The difference is more semantic.
> Use commands for actions and use broadcasters for state.
> In fact, any element can act as broadcaster, as long as you observe it using the observes attribute.

Observer for <menuitem id=mailContext-print> observes "oncommand attribute of <command id=cmd_print>" and perhaps updates "oncommand of <menuitem id=mailContext-print>", even though <menuitem id=mailContext-print> already uses command="cmd_print".
It's never wrong action but it's pretty confusing action, because "command" always kills "oncommand".

Comment 10

[[e-mail]]

Except for keys and menuitems, command="cmd_print" is the same as observes="cmd_print". All attributes except for id, command, observes, persist and ref are broadcast to the element.

For a <key command="cmd_print"> then no attributes are broadcast to the key element. Instead the C++ that handles the key checks the disabled attribute on the <command id="cmd_print"> element directly.

For menuitems, no attributes are broadcast to the menuitem element. However when its popup is opened then the menuitem has its disabled attribute copied from its command (removing the attribute if necessary). The label, accesskey, checked and hidden attributes can also be copied, but are never removed.

Comment 11

[:aceman]

Yes, I noticed some of the elements have both "observes" and command set to the same cmd_* . Do we try to remove them?

Comment 12

[WADA:World Anti-bad-Duping Agency]

(a) toolbarbutton(id=button-chat), menuitem(id=mailContext-printpreview), menuitem(id=mailContext-print) 

It looks that all "oncommand attribete of menuitem/toolbarbutton which has same observes==command" was set by broadcaster of <command id="Cmd_ID"> and observer by <element observes="Cmd_ID">.
If menubaritem, use of both observe="Cmd_ID" & command="Cmd_ID" has advantage like;
   Attribute changes in <command id="Cmd_ID"> can be known before onpopupshowing.
If toolbarbutton, advantage of observe="Cmd_ID" & command="Cmd_ID" is;
   "oncommand attriute set by observer" is convenient when we check XUL element by DOM Inspector.
   We can quickly know used function in <command> without searching <command> element :-)
I don't think removal of "observes" is needed, even when toolbarbutton.

(b) toolbarbutton(id=qfb-show-filter-bar)
     <observe> is used.
     Target of <observe> is< menuiem>. So, it's not "observes==command of toolbarbutton" case.

I think toolbarbutton(id=appmenu-???)  cases are also "Target of observer is< menuiem>" case.
toolbarbutton(id=ElementWithoutID_1) is "no id attribute in <toolbarbutton>" case, so it's hard to find. But it seems "Target of observer is< menuiem>" case. 
Sorry but I didn't check these other cases yet.

Comment 13

[[e-mail]]

(In reply to WADA from comment #12)
> If menubaritem, use of both observe="Cmd_ID" & command="Cmd_ID" has
> advantage like;
>    Attribute changes in <command id="Cmd_ID"> can be known before
> onpopupshowing.
That's only an advantage if you need the attribute to change. Unnecessary attribute changes are a wasteful overhead, which is why we don't bother updating the attributes until the popup is shown.

> If toolbarbutton, advantage of observe="Cmd_ID" & command="Cmd_ID" is;
>    "oncommand attriute set by observer" is convenient when we check XUL
> element by DOM Inspector.
>    We can quickly know used function in <command> without searching
> <command> element :-)
All the ones mentioned in the original description refer to <command id="cmd_foo" oncommand="goDoCommand('cmd_foo');"/>

The other advantage of command= over observes= is that the JavaScript is only compiled once for each command element, instead of once for every menuitem/key/button.

I'm not sure why you would want a toolbarbutton to observe a menuitem. (I don't think we do anything like that in SeaMonkey, and I'm not familiar with the Thunderbird-specific UI code.)

Comment 14

[WADA:World Anti-bad-Duping Agency]

(In reply to [e-mail] from comment #13)
> I'm not sure why you would want a toolbarbutton to observe a menuitem.

If following, required attribute setting in each element can be minimized.
   <command id="CMD_1" oncommand="..." label="..." key="..." modifiers="accel"
                      class="function_group_1 functio_type_1 function_name_1 class_for_menu class_for_button"  
                      other-1="..." ... other-N="..." />
   <menu> <menuitem id="menu_1"   command="CMD_1" accesskey="m" /> </menu>
   <menu> <menuitem id="menu_1x" observes="menu_1" accesskey="c" /> </menu>
   <menu> <menuitem id="menu_1y" observes="menu_1" accesskey="d" /> </menu>
   <toolbarbutton          id="btn_1_a"   observes="menu_1"  />
   <toolbarbutton          id="btn_1_b"   observes="menu_1"  />
In this case, "functionality is same as menu_1" is explicitely expressed.
I don't think "which command will be executed" is not needed if button is an alternative of a menuitem.
Because "label" is mainly for representing functionallity instead of name of menuitem, label, associated key binding etc. is better consolidated to <command>.

Comment 15

[WADA:World Anti-bad-Duping Agency]

Correction. Sorry for typo.
> I don't think "which command will be executed" is needed if button is an alternative of a menuitem.
> I think "which command will be executed" is never needed if button is an alternative of a menuitem.

Comment 16

[WADA:World Anti-bad-Duping Agency]

(In reply to [e-mail] from comment #13)
> Unnecessary attribute changes are a wasteful overhead, 
> which is why we don't bother updating the attributes until the popup is shown.

(b) toolbarbutton(id=qfb-show-filter-bar) case is;
    <toolbarbutton id="qfb-show-filter-bar"
                  type="checkbox" command="cmd_toggleQuickFilterBar" >
          <observes element="view_toolbars_popup_quickFilterBar"  attribute="checked" />
    </toolbarbutton>
    <menuitem id="view_toolbars_popup_quickFilterBar"
                 type="checkbox" command="cmd_toggleQuickFilterBar" />

Does "unnessary attribete change broadcasting/observing" exist in this case?
Is oncommand of <command id="cmd_toggleQuickFilterBar"> propagated to <toolbarbutton id="qfb-show-filter-bar">?
If so, why oncommand is not propagated to <menuitem id="view_toolbars_popup_quickFilterBar">?

Following?
  1. Broadcaster of <command id="cmd_toggleQuickFilterBar"> passes attributes to toolbarbutton & menuitem
  2. Observer by command= doesn't set oncommand attribute of toolbarbutton & menuitem.
      It's perhaps because broadcaster of command doesn't pass needless oncommand.
  3. Broadcaster of <menuitem id="view_toolbars_popup_quickFilterBar"> passes all attributes to toolbarbutton,
      because toolbarbutton has <observes>
  4. Observer of  <toolbarbutton id="qfb-show-filter-bar"> by <observes  attribute="checked" /> filters data,
      and changes checked attribute only.
      Because broadcasting data is not generated by command, is generated by broadcaster for <observes>,
      observer by command= sets oncommand attribute of the toolbarbutton.

I think following is sufficient. Right?
    <toolbarbutton id="qfb-show-filter-bar" observes="view_toolbars_popup_quickFilterBar" />

Anyway, "both oncommand attribute & command attribute" is merely a result of Broadcast/Observe.
It 's merely an indicator of that "broadcasting/observing by observes" occurred on element which has command attribute, when oncommand attribute exists at somewhere.

What we should correct is 'both observes="same_command" & command="same_command">' case only?
If so, both toolbarbutton & menuitem cases? or toolbarbutton case only?

Comment 17

[WADA:World Anti-bad-Duping Agency]

FYI.

(In reply to [e-mail] from comment #13)
> Unnecessary attribute changes are a wasteful overhead, 
> which is why we don't bother updating the attributes until the popup is shown.

Bug 316459 & Bug 328746 is example of "necessary attribute change before onpopupshowing".
   On Mac OS X, shortcut key of menuitem can be changed by OS. 
   When Firefox 2, this shorcut change didn't wok until menu is shown after shortcut change.
These bugs are morphed to Bug 435863 by Firefox 3, and shortcut change still doesn't  work unless menu is kept open. So, such  "attribute change before onpopupshowing" is currently useless for shortcut key change on Mac OS X.

After Australis(Firefox 29), some cases of "change of attribute is not effective until menu display" are reported. I think such cases are "attribute change before onpopupshowing is needed" case.

In any case, I believe "attribute change before onpopupshowing" should be limited to required attribute by <broadcaster> & <observes>.

Comment 18

[WADA:World Anti-bad-Duping Agency]

Following is better for shared attribute in toolbarbutton(id=qfb-show-filter-bar) case?
    Common/static attributes & oncommand : define in <command>
    Shared/updated/modified attributes        : define in <broadcaster>, refer by observes="..."

    <command    id="cmd_toggleQuickFilterBar" oncommand="..." type="checkbox" label="..."  />
    <broadcaster id="Shared_showQuickFilterBar_checkbox" checked="true"  />
    <toolbarbutton id="qfb-show-filter-bar"
              command="cmd_toggleQuickFilterBar" observes="Shared_showQuickFilterBar_checkbox" />
    </toolbarbutton>
    <menuitem id="view_toolbars_popup_quickFilterBar"
              command="cmd_toggleQuickFilterBar"  observes="Shared_showQuickFilterBar_checkbox" />

(i) There are many elements which still uses oncommand, even after command is already defined for the oncommand.
And, (ii) there are may elements which doesn't transfer from oncommand to command.
Should we open bug(s) of "transfer from oncommand world to command/broadcaster/observes world"?

Comment 19

[[e-mail]]

(In reply to WADA from comment #16)
> (b) toolbarbutton(id=qfb-show-filter-bar) case is;
>     <toolbarbutton id="qfb-show-filter-bar"
>                   type="checkbox" command="cmd_toggleQuickFilterBar" >
>           <observes element="view_toolbars_popup_quickFilterBar" 
> attribute="checked" />
>     </toolbarbutton>
>     <menuitem id="view_toolbars_popup_quickFilterBar"
>                  type="checkbox" command="cmd_toggleQuickFilterBar" />
> 
> Does "unnessary attribete change broadcasting/observing" exist in this case?
It depends on how the checked= attribute on the <menuitem> is updated. It might be possible to move the setting of the checked= attribute from the <menuitem> to the <command>. If this were to be the case the <toolbarbutton> would automatically observe the checked= attribute via the command= attribute, and the <menuitem> would update as necessary when its popup opens. However the current code is still pretty efficient as the <toolbarbutton> only has to observe one attribute from the <menuitem>.

> What we should correct is 'both observes="same_command" &
> command="same_command">' case only?
> If so, both toolbarbutton & menuitem cases? or toolbarbutton case only?
Definitely the <toolbarbutton> case, as observes= and command= are almost identical in that case. (Although both attributes cause the oncommand= attribute to be broadcast, the command= attribute then ignores it because the event fires on the <command> element anyway.) In theory the <menuitem> should also only use command= but that assumes that it only needs to observe the specific list of attributes that I mentioned in comment #10.

(In reply to WADA from comment #18)
> Should we open bug(s) of "transfer from oncommand world to
> command/broadcaster/observes world"?
It depends on the use case. If the action has more than one UI element (<toolbarbutton>, <menuitem>, <key> etc.) and they need to be disabled simultaneously, then it makes sense to use a <command>.

Comment 20

[[e-mail]]

To clarify previous comments, you would expect to see both command= and oncommand= for a <toolbarbutton>; the command= causes the oncommand= attribute to get observed but then ignored. (I don't know whether it's worth filing a bug to prevent the oncommand= attribute in that case.) You would not expect to see both command= and oncommand= for a <menuitem> or a <key>.