Need a way to pass Event object from oncommand handler of <command> element to dispatched final command processor via <command oncommand="goDoCommand( ... )"/> When modifier such as Shift/Ctrl is used in an XUL element, Event object is passed to oncommand handler of <menuitem>, <button>, <command> etc.. So, code like following can be used if Event object is processed in oncommand handler by himself. > <toolbarbutton id="button_delete" label="Delete" command="cmd_delete" /> > <menuitem id="menu_delete" label="Delete" command="cmd_delete" /> > <key id="key_delete" command="cmd_delete" /> > <key id="key_shiftDelete" command="cmd_shiftDelete" modifiers="shift" /> > <command id="cmd_delete" oncommand="goDoCommand( > event.shiftKey ? 'cmd_shiftDelete' : 'cmd_delete' > ) /> > Registered command controller dispaches passed aCommand. This technique is hard to use for global menu item which is shared among many components, foe example, "Edit/Delete" in main MenuBar. In main MenuBar, modifier key is not used by major components. So, global definition is following in many cases. <command id="cmd_delete" oncommand="goDoCommand('cmd_delete') /> However, in some situations, modifier key is needed even in menu. For example, "Shift + Delete menu/button/key" for "deleting message(s) without moving mails to Trash". This is same as "Delete files without moving to Trash" on Win. In such case, major users of "Edit/Delete" in main MenuBar usually doesn't have cmd_shiftDelete support in his Command Updater and Command dipatch controller. "Adding support of modifier for a particular component" is pretty hard in such case when <command oncommand="goDoCommand(command_name)"/> is already used by someone. See bug 956446 for an example of such case, please If Event object is passed to final command handler invoked by command dispatch controller, "modifier handling" can be transfered to the final dispatched commad handler who needs to support modifier. This is easily achieved by; > http://mxr.mozilla.org/mozilla-central/source/toolkit/content/globalOverlay.js#89 > 89 function goDoCommand(aCommand) > => function goDoCommand(aCommand,passed_event) > 95 controller.doCommand(aCommand); > => controller.doCommand(aCommand,passed_event); By this enhancement, following is possible. <command id="cmd_xxx" oncommand="goDoCommand(command_name,event)"/> Controller calls needed function with "Func_Name(event,parm1,..);" Because goDoCommand is impleented as global window.goDoCommand object, following by add-on is possible. Save original window.goDoCommand. Replace window.goDoCommand by add-on's one. function goDoCommand(aCommand,passed_event) if(!passed_event) { original_window.goDoCommand(aCommand); } Do same loic as original, and controller.doCommand(aCommand,passed_event); However, this kind of enhancement is better implemented in basic component.

Or should we use code like following always? <command id="cmd_delete" oncommand=" var controller=document.commandDispatcher.getControllerForCommand("cmd_shiftDelete"); (event.shiftKey&&controller)?goDoCommand('cmd_shiftDelete'):goDoCommand('cmd_delete'); " />

Following may be better as "generic enhancement". > 89 function goDoCommand(aCommand) > => function goDoCommand(aCommand,aParm) > 95 controller.doCommand(aCommand); > => controller.doCommand(aCommand,aParm); and, oncommand='var parm={};parm["event"]=event;parm["this"]=this;parm["parmX"]="xxx"; goDoCommand("CommandName",parm);'

Thanks WADA! Yes, this bug is needed. Excellent analysis and hints for implementation. In principle, this is simple to implement without breaking legacy command controllers because it seems that javascript functions don't break on extra parameters not specified in the function parameters (extra arguments will just be available in function's arguments object). However, we might face challenges where existing command handling already provides another argument in second position: https://dxr.mozilla.org/comm-central/rev/1a8f56b5d40991354b30c019cfa805e970fb2673/mail/base/content/mail3PaneWindowCommands.js#603 > doCommand: function(command, aTab) // note aTab, where we want to have aEvent (this bug) So event is not the only extra parameter out there, which must be considered for this bug, but I hope it won't be as complicated as comment 2, maybe if we use the type of argument to tell them apart (I'll comment on that later). *** Why would this bug be useful? *** TB's current workaround is to define the command's target function directly in the <command> element (e.g. for cmd_reply's Shift modifier): <menuitem command="cmd_id"/> > <menuitem id="appmenu_replyMainMenu" > label="&replyMsgCmd.label;" > key="key_reply" > command="cmd_reply"/> <command id="cmd_id" oncommand="cmdFunction(event)"/> > <command id="cmd_reply" oncommand="MsgReplyMessage(event)"/> This isn't ideal for at least two reasons: 1) function executed by cmd_id is defined in XUL, but should be in the separate code layer (js) (easier all-in-one maintenance) 3) extensions might still use goDoCommand(), so we still always have to define doCommand function of the cmd in the controller's cmd definition, too, but then you end up with two different cmd behaviours, and it's kinda confusing because you won't know easily which one will actually be used. In terms of maintenance, we're maintaining the target function call in two different spots (js and xul), but we only use the remote spot in XUL, and pray that noone will use goDoCommand. https://dxr.mozilla.org/comm-central/rev/1a8f56b5d40991354b30c019cfa805e970fb2673/mail/base/content/mail3PaneWindowCommands.js#628-630 > case "cmd_reply": > MsgReplyMessage(null); > break; So confusingly, while this looks like the place where the cmd's target function is defined, TB itself will never execute this, as we avoid using goDoCommand and just rely on the <command>'s direct oncommand target function call, defined in the XUL layer. This bug would simplify this a lot by defining the target function *only* in the controller's command definition, all in one spot in the JS layer where it belongs. *** Implementation details outline *** This is essentially how things would look if we implement this bug. CAVEAT: This does NOT yet handle the above-mentioned problem of other doCommand parameters like aTab. // application xul <command id="cmd_id" oncommand="goDoCommand('cmd_id')"/> <command id="cmd_id" oncommand="goDoCommand('cmd_id', event)"/> // toolkit js function goDoCommand(aCmd_id, aEvent) // this bug: toolkit should accept and pass on the event controller.doCommand(aCmd_id, aEvent); // application js let controller = { doCommand: function(aCommand) // many legacy controllers just won't see the event, so nothing bad happens controller.commands[aCmd_id].doCommand(); --- doCommand: function(command, aTab) // some legacy controllers expect something else, this must be looked into --- doCommand: function(aCommand, aEvent) // new controller can pass on the event controller.commands[aCmd_id].doCommand(aEvent); } commands: { cmd_id: { doCommand: function (aCommand) // most legacy command don't see event passed, but nothing bad happens cmdFunction() --- doCommand: function (aCommand, aEvent) // new command can handle the event cmdFunction(aEvent) } }

(In reply to WADA:World Anti-bad-Duping Agency from comment #2) > Following may be better as "generic enhancement". > > 89 function goDoCommand(aCommand) > > => function goDoCommand(aCommand,aParm) > > 95 controller.doCommand(aCommand); > > => controller.doCommand(aCommand,aParm); > and, oncommand='var > parm={};parm["event"]=event;parm["this"]=this;parm["parmX"]="xxx"; > goDoCommand("CommandName",parm);' I hope very much that we can avoid this complex syntax. Having parameters on <command id="cmd_id" oncommand="let parm=...; goDoCommand('cmd_id', parm"/> does not make much sense at all because you could simply include those parameters in the js command definition instead, as they will *always* apply to that command. Maybe you could try to create new commands as variants of another command, I'm not sure how useful that is, because it would probably be easier and clearer to let the code side handle that (cmd2 function calling cmd1 function with parameters). <command id="cmd_id2" oncommand="let parm=...; goDoCommand('cmd_id1', parm"/> <!-- cmd_id2 calls cmd_id1 with parameters --> Even for oncommand attributes directly placed on menu items which need parameters, event allows you to get the id of the menuitem, then you can special case that id in the code. That said, we need to look into the aTab parameter case above and see how to handle that. If it must be a parameter and can't be determined otherwise, I think we could do this: - goDoCommand(theCmd_id, event, "par1", {par2}, [par3]) In goDoCommand, we just iterate arguments object and pass on anything from 2nd argument (event) to the last argument as we have received it all the way down to the controller. Then folks can just pass whatever they want. To check for event argument, we can check second argument if it is of type "event" and use it in that case. Something like that.

Core bug 461578 seems to want essentially the same thing, but realized on a deeper level. This bug has led to ugly and confusing implementations like this one: > <command id="file_message_button"/> > <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> https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mail/base/content/SearchDialog.xul#192-198 Instead of simply having <button command="cmd_nnn"> and <command id="nnn" oncommand="goDoCommand('cmd_nnn', event)", or even <command id="nnn" oncommand="goDoCommand('cmd_nnn')", then picking up the respective event in the command controller's doCommand function, the command's target function was attached directly to the button's oncommand attribute (primary layout level), instead of the doCommand function of the command controller where it belongs (this bug), or at least the <command>'s oncommand attribute where it could be (even with this bug). Then, due to the mislocation of target function, observes attribute was (mis-)used to get this command to have the right disabled state. Then, we end up thinking that the <command> is unused (because there's nothing specific in the command controller) and can be removed, thus breaking the enabled state (bug 1468940, comment 12). https://dxr.mozilla.org/comm-central/rev/51678c5b0b3edd827c57a70f1c5baf02d44acead/mail/base/content/SearchDialog.js#35-125 If this bug was implemented, this would allow for much cleaner and leaner coding like this: <button command="cmd_nnn"> and <command id="nnn" oncommand="goDoCommand('cmd_nnn', event)", or even <command id="nnn" oncommand="goDoCommand('cmd_nnn')", then picking up the respective event in the command controller's doCommand function.

Aceman, can you please comment on the two-line fix in globalOverlay.js as proposed by WADA and me. (In reply to WADA:World Anti-bad-Duping Agency from comment #2) > Following may be better as "generic enhancement". > > 89 function goDoCommand(aCommand) > > => function goDoCommand(aCommand,aParm) > > 95 controller.doCommand(aCommand); > > => controller.doCommand(aCommand,aParm); YES! Now I am seeing the beauty of this solution, as it's minimally invasive while flexibly catering for any argument passing needs which might occur, including simple passing of event which is likely the most frequent use case. So it's a two-line fix allowing us to eliminate all sorts of ugly and redundant notations for a thing as simple as Shift modifier with cmd_delete. > and, oncommand='var > parm={};parm["event"]=event;parm["this"]=this;parm["parmX"]="xxx"; > goDoCommand("CommandName",parm);' This syntax example is way too complicated, thus obscuring the beauty of WADA's proposal. As per above, globalOverlay.js will just pass through an extra argument after the command argument, and it doesn't care about the type of that argument. So for event, we can simply have this: <command oncommand="goDoCommand('cmd_delete', event)"> For passing any other arguments as needed: <command oncommand="goDoCommand('cmd_delete', {evt:event, argFoo:"foo", argBar:["bar"]})"> Nifty. Just to know it's possible if we'd ever need more.

Hi Justin (triage owner), I hope you'll be the right person to review this straightforward two-liner. Afasics, this patch does not affect existing implementations in any way, but enables consumers to use generic command handling with a convenient one-for-all custom parameter: A typical scenario for this bug is a command like cmd_delete where applications want to differentiate between [DEL] and [Shift][DEL]. <menuitem command="cmd_delete"/> <command id="cmd_delete" oncommand="goDoCommand('cmd_delete', event.shiftKey);"/> Toolkit (with this patch): function goDoCommand(aCommand, aArgument) { ... controller.doCommand(aCommand, aArgument); } Application-level controllers can then handle the custom argument according to their needs (for more details, see comment 3), neatly in a single command using the generic command handling channels. Due to this bug, we can't pass any custom paramter (e.g. event object) into the regular channels of command handling, so applications resort to all sorts of ugly and redundant workarounds: - Short-circuit command handling by adding the actual target function at <command> level: > <command id="cmd_delete" oncommand="delete-handler-function(event.shiftKey);"/> This is undesirable because command execution should happen in doCommand() function of the respective controller, not at some random lower level. - Create two different commands for what is essentially one: > <command id="cmd_delete" oncommand="goDoCommand( > event.shiftKey ? 'cmd_shiftDelete' : 'cmd_delete' This keeps command handling in the right place, but is still undesirable as we shouldn't force applications into creating two artificial commands where one would suffice. In fact, with proper separation of layout and code, this would need another helper function so that the shiftKey stuff is handled in the code layer.

Comment on attachment 8986418 [details] [diff] [review] 959494_toolkit_cmd_argument.patch Aceman, ping for ni? comment 6 (3 months ago). Pls confirm that this 2-liner patch does what it should, without any side effects.

Comment on attachment 8986418 [details] [diff] [review] 959494_toolkit_cmd_argument.patch nsIController::DoCommand doesn't take a second argument, so this should have no effect. I don't think the bug is valid anyway. Checking the modifiers within the <command> element seems more like the right way which I believe should already work.

(In reply to Neil Deakin from comment #9) > Comment on attachment 8986418 [details] [diff] [review] > 959494_toolkit_cmd_argument.patch > > nsIController::DoCommand doesn't take a second argument, so this should have > no effect. So the second argument would need to be added on all the layers. > I don't think the bug is valid anyway. Checking the modifiers within the > <command> element seems more like the right way which I believe should > already work. Maybe that is enough, we just need some way to get at the event that caused the command to run, to read the key modifiers. In bug 461578 comment 5 Neil says something like that is already available in nsIDOMXULCommandEvent . So we do not necessarily need to add arguments to the *Command layers, if there is another way. Can you show a sample code of what you say?