312869 - Support XUL textboxes accepting new context menu items

Status: RESOLVED WONTFIX

(Core :: XUL, enhancement)

Description

[Alex Vincent [:WeirdAl]]

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 1

[Alex Vincent [:WeirdAl]]

Attachment: testcase

Comment 2

[Alex Vincent [:WeirdAl]]

Attachment: patch for xpfe

Comment 3

[Alex Vincent [:WeirdAl]]

Attachment: patch for toolkit

Comment 4

[timeless]

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

fine by me

Comment 5

[[e-mail]]

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 ;-)

Comment 6

[Alex Vincent [:WeirdAl]]

(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.

Comment 7

[[e-mail]]

(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?

Comment 8

[Alex Vincent [:WeirdAl]]

Attachment: testcase based on current code (run from chrome)

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.

Comment 9

[Alex Vincent [:WeirdAl]]

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

Comment 10

[[e-mail]]

(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.

Comment 11

[Alex Vincent [:WeirdAl]]

(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.

Comment 12

[David :Bienvenu]

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

Comment 13

[Alex Vincent [:WeirdAl]]

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 14

[Alex Vincent [:WeirdAl]]

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

Not surprisingly, the xpfe patch bitrotted too.

Comment 15

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1.1

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 16

[Scott MacGregor]

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!

Comment 17

[[e-mail]]

Does this conflict with dynamic spellcheck menuitems in any way?

Comment 18

[Alex Vincent [:WeirdAl]]

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.)

Comment 19

[Alex Vincent [:WeirdAl]]

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 20

[[e-mail]]

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.

Comment 21

[Alex Vincent [:WeirdAl]]

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

Comment 22

[[e-mail]]

(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.

Comment 23

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1.2

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 24

[Alex Vincent [:WeirdAl]]

Comment on attachment 229899 [details] [diff] [review]
patch, v1.2

Whoops.  Forgot the mail client change.

Comment 25

[[e-mail]]

Comment on attachment 229899 [details] [diff] [review]
patch, v1.2

1) menulists 2) this is starting to get very hacky.

Comment 26

[Alex Vincent [:WeirdAl]]

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 27

[[e-mail]]

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.

Comment 28

[Alex Vincent [:WeirdAl]]

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