Open Bug 312869 Opened 15 years ago Updated 10 years ago

Support XUL textboxes accepting new context menu items

Categories

(Core :: XUL, enhancement)

x86
Windows XP
enhancement
Not set

Tracking

()

People

(Reporter: WeirdAl, Unassigned)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
Attached file testcase
Attached patch patch for xpfe (obsolete) — Splinter Review
Attachment #200012 - Flags: review?(timeless)
Attached patch patch for toolkit (obsolete) — Splinter Review
Attachment #200014 - Flags: review?(mconnor)
Comment on attachment 200012 [details] [diff] [review]
patch for xpfe

fine by me
Attachment #200012 - Flags: superreview?
Attachment #200012 - Flags: review?(timeless)
Attachment #200012 - Flags: review+
Attachment #200012 - Flags: superreview? → superreview?(neil)
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.
Blocks: 287623
No longer blocks: 287623
Blocks: 287623
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.
Attachment #200014 - Attachment is obsolete: true
Attachment #200014 - Flags: review?(mconnor)
Comment on attachment 200012 [details] [diff] [review]
patch for xpfe

Not surprisingly, the xpfe patch bitrotted too.
Attachment #200012 - Attachment is obsolete: true
Attachment #200012 - Flags: superreview?(neil)
Attached patch patch, v1.1 (obsolete) — Splinter Review
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.
Attachment #229037 - Flags: superreview?(neil)
Attachment #229037 - Flags: review?(mscott)
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!
Attachment #229037 - Flags: review?(mscott) → review+
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.
Attachment #229037 - Flags: superreview?(neil) → superreview-
:( 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.
Attached patch patch, v1.2Splinter Review
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.
Attachment #229037 - Attachment is obsolete: true
Attachment #229899 - Flags: superreview?(neil)
Attachment #229899 - Flags: review+
Comment on attachment 229899 [details] [diff] [review]
patch, v1.2

Whoops.  Forgot the mail client change.
Attachment #229899 - Attachment is obsolete: true
Attachment #229899 - Flags: superreview?(neil)
Comment on attachment 229899 [details] [diff] [review]
patch, v1.2

1) menulists 2) this is starting to get very hacky.
Attachment #229899 - Attachment is obsolete: false
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.  :(
Assignee: ajvincent → nobody
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.