Once upon a time, I filed bug 180512 to override the context menu for XUL textboxes. I now realize that was a big mistake, in that a replaced context menu essentially cannot easily use the controllers for the textbox. Simply put, it's a lot more work for end-users to implement cut, copy, paste, select all, undo, delete operations in a custom context menu than it has to be. This bug proposes backing out the changes in bug 180512 and replacing them with a friendlier solution. XUL app developers should be able to add context menu items, separators and submenus by simply appending them as child elements of the textbox element. This bug is on file to implement these features for: XUL textbox XUL multiline textbox XUL autocomplete textbox XUL editable menulist The patches I submit will be for both xpfe and toolkit.

Comment on attachment 200012 [details] [diff] [review] patch for xpfe fine by me

Comment on attachment 200012 [details] [diff] [review] patch for xpfe Why have you stopped inheriting the context attribute? Your last change has bitrotted, especially for toolkit ;-)

(In reply to comment #5) > (From update of attachment 200012 [details] [diff] [review] [edit]) > Why have you stopped inheriting the context attribute? Because it was a bad idea (bug 180512). The controllers for cut/copy/paste broke when a new context menu took over, and it was not repairable. > Your last change has bitrotted, especially for toolkit ;-) I'll look into posting a new fix.

(In reply to comment #6) >The controllers for cut/copy/paste broke when a new context menu took over Sorry, what do you mean by this?

You know, I was so positive this would not work, could not work, etc., that before I posted the attachment, I had to be sure I'd tried everything to make it not work... and then I made it work. :-p That said, compare the code (or for that matter, the file size) between this attachment and attachment 199976 [details]. The latter case is smaller, more efficient, and involves some internal textbox API code to pull off the same result. This attachment maintains the current textbox context menu items, but at a cost in code size. Please, Neil, let the old version die. It was a bad idea that I regret having done in the first place.

err, the latter case is not as small, not as efficient, and requires internal API. The former case is leaner and better.

(In reply to comment #8) >Please, Neil, let the old version die. It was a bad idea that I regret having >done in the first place. Well you'd better start by removing all existing consumers then.

(In reply to comment #10) > Well you'd better start by removing all existing consumers then. http://lxr.mozilla.org/seamonkey/search?string=context%3D I just manually inspected every instance in our tree of the string "context=". The context attribute on textboxes is used precisely once: http://lxr.mozilla.org/seamonkey/source/mail/extensions/newsblog/content/feed-properties.xul#74 I'm cc'ing mscott to see what he wants to do here.

cc'ing Myk on the off chance TB took this usage from Forumzilla.

Comment on attachment 200014 [details] [diff] [review] patch for toolkit toolkit patch bitrotted; I'll submit a new patch with the Thunderbird change as well.

Comment on attachment 200012 [details] [diff] [review] patch for xpfe Not surprisingly, the xpfe patch bitrotted too.

I have compiled Mozilla Thunderbird debug build with this patch applied, and *gasp* I was able to test it and show it worked. This patch is essentially the same as the xpfe and toolkit patches, combined, plus the solitary needed fix for Thunderbird. The xpfe portion has r+ from timeless. mscott, I'm asking you to review for the Thunderbird UI change. The toolkit change has not been officially reviewed yet, but fundamentally is identical to the xpfe change. Neil, if you could help out there in addition to sr, I'd appreciate it.

Comment on attachment 229037 [details] [diff] [review] patch, v1.1 r=mscott for the thunderbird change, I didn't look over the toolkit/xpfe changes. lemme know if you need me to. Thanks for the patch!

Does this conflict with dynamic spellcheck menuitems in any way?

I don't see how it would, but could you give me an example? (I've not used spellcheck in Mozilla that much, as I'm naturally very good at spelling.)

Oh ho! I see what you mean, but no, I don't think this is an issue. You're talking about #input_box@_doPopupItemEnabling(popupNode). It calls: // suggestion list var spellSeparator = document.getAnonymousElementByAttribute(this, "anonid", "spell-suggestions-separator"); var numsug = spellui.addSuggestionsToMenu(popupNode, spellSeparator, 5); The popup menu looks like: <xul:menuitem label="&spellNoSuggestions.label;" anonid="spell-no-suggestions" disabled="true"/> <xul:menuseparator anonid="spell-suggestions-separator"/> <!-- ... --> I insert the custom context menu items before that menuitem. Now, the add/remove menuitems code for spellchecking caches all its added menuitems in this.mSuggestionItems, and removes based on that array. So my custom menuitems precede the spellcheck menuitems, but do not conflict with them in any way, and cannot be removed inadvertently by the current binding code.

Comment on attachment 229037 [details] [diff] [review] patch, v1.1 This is fine from an XBL point of view but at least in the theme I was using, the custom menuitems don't know that they're in a menu because their DOM parent is a textbox not a menupopup, which means that they aren't styled correctly.

:( Can you give me a testcase, including the theme in question? (i.e. is this SeaMonkey Modern, SeaMonkey classic, Firefox?)

(In reply to comment #21) >Can you give me a testcase, including the theme in question? Using your first attachment, the issue applies to both SeaMonkey themes.

Okay, so I needed to add the textbox > menu and textbox > menuitem selectors. That was a very good catch, subtle enough that I missed it. Thank you.

Comment on attachment 229899 [details] [diff] [review] patch, v1.2 Whoops. Forgot the mail client change.

Comment on attachment 229899 [details] [diff] [review] patch, v1.2 1) menulists 2) this is starting to get very hacky.

What's hacky about this? The XBL has not changed from your previous review, nor has the XUL. The CSS changes, while somewhat unpleasant, appear to be a necessity.

Comment on attachment 229899 [details] [diff] [review] patch, v1.2 >+textbox > menu, > menupopup > menu, > popup > menu, >+textbox > menuitem, > menupopup > menuitem, > popup > menuitem { > padding: 2px; > max-width: 42em; > } > > menupopup > menu[_moz-menuactive="true"], > menupopup > menuitem[_moz-menuactive="true"], To see what I meant by the css being hacky, you forgot menulist > menu menulist > menuitem textbox > menu[_moz-menuactive="true"] textbox > menuitem[_moz-menuactive="true"] menulist > menu[_moz-menuactive="true"] menulist > menuitem[_moz-menuactive="true"] So what I was thinking is that it might be better if you made the submenu style the default and apply specific styles to menus in menubars.

With the check-in for bug 345510, now I have to deal with numberbox as well. :(