Bug 446281 comment 14: [ neil@parkwaycc.co.uk 2008-08-14 15:36:51 PDT I wonder why these are still observes= instead of command= (I didn't try to see whether command= works or not). ]

"Found 202 matching lines in 5 files" *** See bug 613435 as an example.

This is so going to bitrot me, but as my patches probably won't make it into 2.1, due to string freeze, I suppose it doesn't really matter.

(In reply to comment #3) > This is so going to bitrot me, but as my patches probably won't make it into > 2.1, due to string freeze, I suppose it doesn't really matter. I would think it would matter. I'll wait until you check in your patch then I'll fix whatever needs fixing. It's not like this bug is in a hurry to be fixed. :)

(In reply to comment #4) > It's not like this bug is in a hurry to be fixed. :) It's not, but now that your patch is done... Or your could do replacement only now and postpone reformatting.

Comment on attachment 523878 [details] [diff] [review] s/observes=/command=/g in /editor/ui/* >- <observes element="paragraph-select-container" attribute="disabled"/> >- <observes element="cmd_paragraphState" attribute="state" onbroadcast="onParagraphFormatChange(this.parentNode, 'cmd_paragraphState')"/> >+ <command element="paragraph-select-container" attribute="disabled"/> >+ <command element="cmd_paragraphState" attribute="state" onbroadcast="onParagraphFormatChange(this.parentNode, 'cmd_paragraphState')"/> This change is wrong. The other changes which are wrong relate to cmd_renderedHTMLEnabler. This element is badly named, as it's really a broadcaster that is used to disable elements when the focus is in the subject field in a message, and doesn't have an action. This means that we shouldn't use command= as it inhibits any oncommand event on the element or its ancestors. (It doesn't appear to inhibit the event on child nodes, but that's not so useful as command= is designed for menuitems and keys and they don't have children anyway.)

Comment on attachment 524056 [details] [diff] [review] s/observes=/command=/g in /editor/ui/* (v2) >diff --git a/editor/ui/composer/content/editor.xul b/editor/ui/composer/content/editor.xul The two changes in this file are wrong ones that you forgot to revert. > <menupopup id="fontStyleMenuPopup" onpopupshowing="initFontStyleMenu(this)"> Sadly initFontStyleMenu doesn't work with command= So all the changes in this menupopup have to be removed, pending a followup. >+ <menuitem id="fontColor" >+ label="&formatFontColor.label;" >+ accesskey="&formatFontColor.accesskey;" >+ command="cmd_fontColor" >+ oncommand="EditorSelectColor('Text', null);" Can't have both command and oncommand on the same element. I'm not quite sure what the point of the cmd_fontColor element is, but let's keep it observes=. >+ <menuitem id="removeStylesMenuitem" >+ key="removestyleskb" >+ command="cmd_removeStyles" >+ position="6"/> >+ <menuitem id="removeLinksMenuitem" >+ key="removelinkskb" >+ command="cmd_removeLinks" >+ position="7"/> >+ <menuitem id="removeNamedAnchorsMenuitem" >+ label="&formatRemoveNamedAnchors.label;" >+ key="removenamedanchorskb" >+ accesskey="&formatRemoveNamedAnchors.accesskey;" >+ command="cmd_removeNamedAnchors" >+ position="8"/> These changes don't work for some reason that I didn't bother to figure out. >- <menuitem id="objectProperties" oncommand="goDoCommand('cmd_objectProperties')" observes="cmd_renderedHTMLEnabler"/> >+ <menuitem id="objectProperties" >+ oncommand="goDoCommand('cmd_objectProperties')" >+ observes="cmd_renderedHTMLEnabler"/> You didn't undo the edit you made to this correctly. > <toolbarbutton id="AlignPopupButton" > class="formatting-button" > tooltiptext="&AlignPopupButton.tooltip;" > type="menu" >- observes="cmd_align"> >+ command="cmd_align"> I'd like to remove the changes to menubuttons. I'm not sure they're useful. >+ <menuitem id="InsertLinkItem" >+ class="menuitem-iconic" >+ command="cmd_link" >+ oncommand="goDoCommand('cmd_link')" ... >+ <menuitem id="InsertTableItem" >+ class="menuitem-iconic" >+ command="cmd_table" >+ oncommand="goDoCommand('cmd_table')" I said before you don't want both command and oncommand. Fortunately in these cases the command= does all the work and the oncommand is now unnecessary. > <toolbarbutton id="smileButtonMenu" > class="formatting-button" > tooltiptext="&SmileButton.tooltip;" > type="menu" >- observes="cmd_smiley"> >+ command="cmd_smiley"> Again, I don't think this one needs to be changed.

(In reply to comment #8) > Can't have both command and oncommand on the same element. I'm not quite sure > what the point of the cmd_fontColor element is, but let's keep it observes=. cmd_fontColor was meant to be reusable elsewhere. The handler is here because the color boxes accept a shift-click to select directly the last selected color, IIRC.

Comment on attachment 524623 [details] [diff] [review] s/observes=/command=/g in /editor/ui/* (v3) >- <menuitem id="InsertLinkItem" class="menuitem-iconic" observes="cmd_link" >- oncommand="goDoCommand('cmd_link')" label="&linkToolbarCmd.label;" >- tooltiptext="&linkToolbarCmd.tooltip;" /> >- <menuitem id="InsertAnchorItem" class="menuitem-iconic" observes="cmd_anchor" >- oncommand="goDoCommand('cmd_anchor')" label="&anchorToolbarCmd.label;" >- tooltiptext="&anchorToolbarCmd.tooltip;" /> >- <menuitem id="InsertImageItem" class="menuitem-iconic" observes="cmd_image" >- oncommand="goDoCommand('cmd_image')" label="&imageToolbarCmd.label;" >- tooltiptext="&imageToolbarCmd.tooltip;" /> >- <menuitem id="InsertHRuleItem" class="menuitem-iconic" observes="cmd_hline" >- oncommand="goDoCommand('cmd_hline')" label="&hruleToolbarCmd.label;" >- tooltiptext="&hruleToolbarCmd.tooltip;" /> >- <menuitem id="InsertTableItem" class="menuitem-iconic" observes="cmd_table" >- oncommand="goDoCommand('cmd_table')" label="&tableToolbarCmd.label;" >- tooltiptext="&tableToolbarCmd.tooltip;" /> >+ <menuitem id="InsertLinkItem" >+ class="menuitem-iconic" >+ observes="cmd_link" >+ oncommand="goDoCommand('cmd_link')" >+ label="&linkToolbarCmd.label;" >+ tooltiptext="&linkToolbarCmd.tooltip;"/> >+ <menuitem id="InsertAnchorItem" >+ class="menuitem-iconic" >+ observes="cmd_anchor" >+ oncommand="goDoCommand('cmd_anchor')" >+ label="&anchorToolbarCmd.label;" >+ tooltiptext="&anchorToolbarCmd.tooltip;"/> >+ <menuitem id="InsertImageItem" >+ class="menuitem-iconic" >+ observes="cmd_image" >+ oncommand="goDoCommand('cmd_image')" >+ label="&imageToolbarCmd.label;" >+ tooltiptext="&imageToolbarCmd.tooltip;"/> >+ <menuitem id="InsertHRuleItem" >+ class="menuitem-iconic" >+ observes="cmd_hline" >+ oncommand="goDoCommand('cmd_hline')" >+ label="&hruleToolbarCmd.label;" >+ tooltiptext="&hruleToolbarCmd.tooltip;"/> >+ <menuitem id="InsertTableItem" >+ class="menuitem-iconic" >+ observes="cmd_table" >+ oncommand="goDoCommand('cmd_table')" >+ label="&tableToolbarCmd.label;" >+ tooltiptext="&tableToolbarCmd.tooltip;"/> These you changed from observes="" to command="", then you reformatted, then I asked you to remove the oncommand="", but instead you changed the command="" back to observes="" ... rest of the patch is OK though.

Sorry about that. Your instructions were very clear. It's my brain that was befuddled. The next patch should address that.

Comment on attachment 524848 [details] [diff] [review] s/observes=/command=/g in /editor/ui/* (v4) [Checked in: Comment 14] http://hg.mozilla.org/comm-central/rev/b36443e20146

(In reply to comment #8) > These changes don't work for some reason that I didn't bother to figure out. Turned out to be the same problem as the font style submenu - command confuses the code by synchronising some attributes that observes doesn't.

(In reply to comment #1) > "Found 202 matching lines in 5 files" Down to "Found 54 matching lines in 3 files" :-) (In reply to comment #6) > The other changes which are wrong relate to cmd_renderedHTMLEnabler. This > element is badly named, as it's really a broadcaster that is used to disable > elements when the focus is in the subject field in a message, and doesn't have > an action. This means that we shouldn't use command= as it inhibits any > oncommand event on the element or its ancestors. (It doesn't appear to inhibit > the event on child nodes, but that's not so useful as command= is designed for > menuitems and keys and they don't have children anyway.) Ftr, http://mxr.mozilla.org/comm-central/search?string=cmd_renderedHTMLEnabler&case=1&find=%2Fcompose "Found 72 matching lines in 9 files" (including /mail and /suite) http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/ComposerCommands.js#463 465 var nsDummyHTMLCommand = 469 return (IsDocumentEditable() && IsEditingRenderedHTML()); 478 dump("Hey, who's calling the dummy command?\n"); https://developer.mozilla.org/en/XUL_Tutorial/Broadcasters_and_Observers Neil, should I file a separate bug to rename cmd_renderedHTMLEnabler? And possibly change it to a broadcaster instead of a (dummy) command? (In reply to comment #8) > So all the changes in this menupopup have to be removed, pending a followup. [And following comments.] We should (re-)check the few remaining cases to summarize what's left to do (here or in follow-ups).

(In reply to comment #16) > Neil, should I file a separate bug to rename cmd_renderedHTMLEnabler? > And possibly change it to a broadcaster instead of a (dummy) command? Yeah, it should definitely become a broadcaster, and probably be renamed too.

Comment on attachment 524848 [details] [diff] [review] s/observes=/command=/g in /editor/ui/* (v4) [Checked in: Comment 14] > <!-- label and accesskey are set in InitObjectProperties --> >- <menuitem id="objectProperties_cm" observes="cmd_objectProperties"/> >+ <menuitem id="objectProperties_cm" >+ command="cmd_objectProperties"/> This doesn't work :-( Problem caused by incorrect comment - InitObjectPropertiesMenuitem also tries to set the disabled state.

Resolving. Any remnant issues can be spun off into a new bug.

(In reply to neil@parkwaycc.co.uk from comment #17) > Yeah, it should definitely become a broadcaster, and probably be renamed too. I filed bug 702374.