Skip to content
This repository

Usage when selector isn't available #88

Closed
mstratman opened this Issue · 17 comments

2 participants

Mark A. Stratman Rodney Rehm
Mark A. Stratman

If a selector isn't available, is there any other way to use this plugin?

For example, given:

$("#container .thing").each(function() {
    $(this).anotherPlugin(); // 
});

If anotherPlugin wants to create a context menu, there appears to be no way to limit its effects to only the .thing elements in #container, since anotherPlugin() does not get a selector.

If contextMenu() can't behave like other jquery plugins (i.e. $(selector).contextMenu()), then can its 'selector' attribute also allow objects in addition to string selectors?
e.g. I could, within anotherPlugin, do something like:

$.contextMenu({ selector: $(this) });

Or am I missing an altogether better solution? thanks,

Rodney Rehm
Owner

This is currently not possible. We would have to "calculate" a selector matching the given element and create the menu for that. I don't like this at all, though. It allows sloppy integration that will ultimately lead to bad things happening. That said, I'm open for discussion…

so anotherPlugin() is supposed to be limited to the scope of $('#container .thing'). What's the rationale for this? A plugin cannot be sandboxed to a specific DOMNode - it can litter all over the global scope if it wants to. So what exactly do you want to achieve with this? Why do you need to create context menus for individual elements?

Mark A. Stratman

Thanks for the info.

It sounds like this simply isn't going to be possible without significant reengineering of the contextMenu.

I believe jquery plugins can, do, and should sandbox to a specific dom element, or set of elements. Calling $("#foo").bar() should only modify or change the behavior of #foo, and should not affect other things on the page unexpectedly.

If you're curious - let's say, for example, the other plugin is something that makes tables editable (it's not, but hopefully this is a simple example to illustrate the problem)" $("table.editable").editable(). If the editable plugin wants to provide a context menu for its 'td' elements, it needs a selector to pass to contextMenu().

Unfortunately if it simply passed "td" as the selector, this would be a brazen violation of the user's expectations: i.e. ALL "td" elements on the page would be given a context menu, rather than simply the one on which editable() was called.

It might be tempting to suggest it should simply provide "table.editable td" as the selector, but editable() cannot always know what its selector was: e.g. $(domObj).editable(). For that matter, it might not even have been a table at all - maybe the user called editable() on a div! This leaves other plugins the choice of either a) not integrating contextMenu, or b) adding a context menu to every element on the page (not much of a choice)

This is one of the primary reasons why the jquery plugin convention is to operate on a jquery object rather than pass it a selector. i.e. $(".menu").contextMenu() rather than $.contextMenu({ selector: ".menu" })

Mark A. Stratman

Actually, this might be a simpler way to illustrate the problem:

$(selector, context) vs $(selector). I'm looking to achieve the same effect as the former... and this is a very common requirement in jquery code.

Rodney Rehm
Owner

I believe jquery plugins can, do, and should sandbox to a specific dom element,

While I agree with you on this front - it simply isn't the case. In some circumstances it's even impossible (consider breaking out of an overflow:hidden element, for example). contextMenu appends its menus to document.body for that and several other reasons. So I'm myself breaking the law I'd like other plugins to respect.

This is one of the primary reasons why the jquery plugin convention is to operate on a jquery object rather than pass it a selector. i.e. $(".menu").contextMenu() rather than $.contextMenu({ selector: ".menu" })

true, which is why I suggested "calulating" a (rather specific) selector for the root-node a menu is to be registered on. But that just feels ugly. Instead I would keep the selector as it is (td) and make contextMenu accept a context property ($("table.editable")) on which the triggers (delegated events, actually) should be registered. I haven't looked at the code, I can't say if this is feasible, but it's what "feels right". At least it doesn't feel as wrong as allowing people to initialize a new menu for every single trigger element.

nice, now that I've answered your first comment, I see you've had the same idea in the next.

There are some limitations, though. One of them being that you can't access menus created for $node on the global scope. So if you removed the table.editable node and lost all references to it, you won't be able to destroy it (as a result you're leaking memory). There is no "remove" event a plugin could hook into. To prevent this from happening, I'd have to overhaul the internal data handling. Not sure how easy that's going to be, or if this should even be a thing to consider at this stage…

Objections? Thoughts?

Mark A. Stratman

Unfortunately this is a tough situation, and I'm not sure there's a clear or best answer. I'm just going to hold off using this contextmenu in this situation for now. I'll probably revisit this when I have more time to invest in getting familiar with the guts of this plugin.

Rodney Rehm
Owner

I'm interested in what alternative you've settled for.

Mark A. Stratman

Right now I'm using this old one: http://www.trendskitchens.co.nz/jquery/contextmenu/

I was recently evaluating your contextMenu since it makes it easier to visually disable menu items, and because it's more actively developed (and just all-around cleaner and nicer, especially the api). But switching is not a pressing need since what we have works.

Rodney Rehm
Owner

jQuery.ui.widget duck punches jQuery.cleanData() (apparently not publicly documented, function to handle data-cleanups for removed dom nodes). jQueryUI adds the triggering of a custom remove event to achieve the very thing I was afraid of. There even is a ticket for making this a jQuery core thing: #12213 - Add hook to cleanData to allow cleanup.

So in theory this change shouldn't have a very high impact. Duck punch jQuery.cleanData() unless jQuery.widget is available, add if (operation.selector) check to jQuery.fn.contextMenu() and pass request (adding the property context: this) to jQuery.contextMenu() where we'll use o.context || $document to bind events. Sounds easy enough.

Rodney Rehm
Owner

Right now I'm using this old one: http://www.trendskitchens.co.nz/jquery/contextmenu/

I see, you're not interested in supporting Firefox then, right? That demo-page doesn't show a menu on Firefox (but does on Safari, Chrome, Opera)

Mark A. Stratman

I'm glad you point that out - I didn't realize it had problems. I'll have to look into it. Thanks!

Rodney Rehm rodneyrehm referenced this issue from a commit
Rodney Rehm rodneyrehm Fixing Issue #88 - Creating contextMenu on DOM Element
(removed obsolete Firefox 12 hack in the process)
5ccc2e3
Rodney Rehm
Owner

Well… I've implemented it and it seems to work fine. As I only push complete releases to gh-pages on github, I've pushed you the mstratman-preview branch. check it out and tell me if that works for you. (no docs, but there is a demo-page showing it off)

The next official release will take a while, so play with the branch for now :)

Mark A. Stratman

That's great! :) Very cool - I'll give it a try very soon and let you know what I find.

Mark A. Stratman

I haven't yet had time to investigate it (so sorry for no patch or test case), but I just wanted to give you a heads-up that it seems to not be backwards compatible. e.g. $.contextMenu({ selector : "li" }); won't cause menus for all "li" elements like it does in the gh-pages branch.

I'll try to look at this closer later, and follow up when I can. Thanks again.

Rodney Rehm
Owner

yes, there was a bug I fixed a couple of minutes ago… git pull and it should work again :)

Rodney Rehm
Owner

Have you tried it out? does it work as expected?

Mark A. Stratman

Sorry for the long delay. I haven't forgotten about your hard work!

I did a quick, cursory trial with some examples and so far it seems great.

Unfortunately, I haven't yet tested it in my larger app, since we still haven't been able to upgrade jquery (stuck on 1.6.4 temporarily) - and it seems this plugin uses on() which comes from 1.7.

As soon as we get this upgraded I'll give it a try - or maybe sooner if possible.

Thanks again.

Mark A. Stratman

I've given it a closer look and everything seems to work great. Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.