Skip to content

Loading…

Generating multiple individual menus using event delegation #51

Closed
2m3 opened this Issue · 6 comments

2 participants

@2m3
2m3 commented

I have a list of items, each of them has an individual context menu. I use event delegation to keep the number of bound events low and bind the context menu to the list itself instead of each list item. Then, I use the build callback to generate the individual menus.

$.contextMenu({
  selector: 'ul',
  build: function($trigger, e) {
    var $listEntry = $(e.target).closest('li');

    // build dynamic menu
    return {};
  }
}

The problem with this approach is when I right-click one list item the context menu opens but when I immediately right-click another one the existing (wrong) menu is simply moved to the new position instead of generating the menu for the new item. This behaviour makes sense when right-clicking multiple times on the same trigger, but not in my case when menu generation depends on event generation (where I have kind of multiple triggers).

What can be done here (not sure this justifies a github issue)?

Additionally it would be helpful if the selector option would accept a jQuery object or DOM element in addition to the selector string.

@rodneyrehm

How many different types of contextMenus are you handling here? Do you know that contextMenu itself uses delegated events? It'll bind one (to 3) events per registered contextMenu. Each menu has its own configuration space.

If it is unfeasible to register distinct menus, we might be able to allow the build function to overwrite the $trigger element. I haven't thought of the ramifications, yet, though…

Why would you want $.contextMenu() to accept a DOMElement / jQuery instance? That would defeat the event delegation logic. What exactly is your use-case for this?

@2m3
2m3 commented

Ok, I see events are bound to $body internally (even though I do not get the $body = $body assignment in L1149).

I'm talking about possibly 100s of list items. Even though there are only a few events bound at the document level, there is quite a lot of data associated with it. I'll see if this has any performance implications.

I moved the menu initialization to the list item level and might be even able to get rid of the build callback.

However, I found a serious quirk when handling multiple context menus. Do you possibly have FireGestures installed as a Firefox Add-on? When this Add-on is not installed or disabled, opening a second context menu will not close the first. Additional left and right clicks have also unexpected results.

This behavior can be reproduced on your test page http://jsfiddle.net/rodneyrehm/JdwxT/
(Tested on a couple Macs with Firefox 12, other browsers work as expected.)

I use your plugin in a Backbone environment, so each list item is rendered as a separate view that already has a root element tied to it. Using $el or simply el as the trigger would be less DRY instead of explicitly stating the selector.

There is another caveat, though. At first I used the same selector (the list item) in order to initialize each context menu. This is normally fine in Backbone as each view is scoped to a root element. As your delegation logic does not know about the root element, delegation will fail and the plugin will always show the first context menu.
I solved this by adding a unique id to the trigger element and pass the according selector to the plugin:

...selector: '.listEntry[data-id=' + this.model.id + ']'...

Too much information? :-)

@rodneyrehm

Ok, I see events are bound to $body internally (even though I do not get the $body = $body assignment in L1149).

I have no clue what drugs may have caused that. Fixed it though, thx!

I'm talking about possibly 100s of list items. Even though there are only a few events bound at the document level, there is quite a lot of data associated with it. I'll see if this has any performance implications.

Your use-case is pretty much what I designed for. You register one(!) menu for ul.some-menu > li and each <li> gets a distinct menu. Using the build() approach, you can customize the menu for distinct types of <li>. Either you overlooked that, or I did not understand what you're getting at.

However, I found a serious quirk when handling multiple context menus. Do you possibly have FireGestures installed as a Firefox Add-on? When this Add-on is not installed or disabled, opening a second context menu will not close the first. Additional left and right clicks have also unexpected results.

How the F* did you figure out I was using FireGestures? (and yes, I am)
disabling the addon I can see your problem.

After running this test in Firefox 12 and Nightly, Chrome, Safari and Opera, I'm tempted to conclude a bug in Firefox 12. Chrome, Safari and Firefox Nightly (would be Firefox 14/15) behave the same. Opera delayes the contextmenu event because of its »Opera Mouse Gestures Thingie« and Firefox 12 simply fires the contextmenu event on the wrong element.

I have not tried to work around this bug, yet. But at least we know what's going on… As the problem does not occur in nightly, I'm not compelled to sift through Bugzilla :/

I use your plugin in a Backbone environment, so each list item is rendered as a separate view that already has a root element tied to it. Using $el or simply el as the trigger would be less DRY instead of explicitly stating the selector.

I haven't used Backbone. But what keeps you from registering a menu for .gimme-some-menu > li and add the class gimme-some-menu to the <ul> created by Backbone? Use build() to create custom menus for each <li> on demand.

Too much information? :-)

There is such a thing?

@rodneyrehm rodneyrehm added a commit that referenced this issue
@rodneyrehm rodneyrehm fixing issue #51 - delay elementFromPoint test because Firefox 12 wou…
…ld trigger the contextmenu event on the newly uncovered element instead of the layer
e0ed809
@rodneyrehm

Well, setTimeout came to save the day… probably not the nicest solution, but it works. 1.5.17 contains a workaround for the Firefox 12 bug you found. Anything else?

@2m3
2m3 commented

1.5.17 works fine for me, thank you!

Either you overlooked that, or I did not understand what you're getting at.

I think I know what I was missing. I used the build option in the first place, but bound to theul instead of the individual lis. When I got to know the plugin uses event delegation internally, I switched to the lis and had a separate plugin call for each li in the corresponding list item view. That even allowed me to get rid of the build option as each context menu was now created in the context of this very list item (but required to use a distinct selector for each li).

Finally, I switched back to one single plugin call in the list view, but stating the lis as the selector (as proposed by you). In this case I of course need the build option again but only have one plugin call and one selector. Much better.

How the F* did you figure out I was using FireGestures? (and yes, I am)

I had the problem on one Mac but not another (same OS, same Firefox). So I disabled all Add-on on the one that was working and it also failed. Then I enabled the Add-ons one after another and bamm - FireGestures. That you have it installed was just a bad guess, then.

@rodneyrehm

Ok, happy it works for you :)

@rodneyrehm rodneyrehm closed this
@baohx2000 baohx2000 pushed a commit to Contatta/jQuery-contextMenu that referenced this issue
@rodneyrehm rodneyrehm fixing issue #51 - delay elementFromPoint test because Firefox 12 wou…
…ld trigger the contextmenu event on the newly uncovered element instead of the layer
baa3003
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.