contextmenu() shouldn't just return sendEvent() since it returns the wrong type/value #549

Closed
awk opened this Issue Jul 14, 2011 · 0 comments

1 participant

@awk

There looks to be a bug in the contextmenu() method of root_responder.js :

 contextmenu: function(evt) {
   var view = this.targetViewForEvent(evt) ;
   return this.sendEvent('contextMenu', evt, view);
 },

sendEvent returns the target which handled the method (a View somewhere - because even if you implement the isContextMenuEnabled property the view with that property will still handle this message to check the property).

However the event handling code in SC.Event.handle which calls contextmenu() expects the method to either return 'NO' or some other value to indicate whether the event should be propagated etc. Since the comparison in SC.Event.handle is '!==' there's no type coercion (not that any would work here since the target returned is non-null).

The end result is the menu always shows up regardless of the property (or even if you override contextMenu in your view).

I think that the fix would be to make contextmenu() work more like key and mouse event handling and to use the evt.hasCustomHandling flag ?

@publickeating publickeating added a commit that referenced this issue Apr 3, 2013
@publickeating publickeating Fixes bug that allowed the context menu to appear regardless of overr…
…iding contextMenu in a view or setting SC.CONTEXT_MENU_ENABLED or isContextMenuEnabled to false. This makes the context menu event handling behave the same as the key, mouse, etc. event handling. Fixes #549.
3e09e24
@publickeating publickeating added a commit that closed this issue Apr 3, 2013
@publickeating publickeating Fixes bug that allowed the context menu to appear regardless of overr…
…iding contextMenu in a view or setting SC.CONTEXT_MENU_ENABLED or isContextMenuEnabled to false. This makes the context menu event handling behave the same as the key, mouse, etc. event handling. Fixes #549.
9e11b42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment