Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: contextmenu in V4 is not default-preventable #2658

Open
ldhasson opened this issue May 10, 2024 · 11 comments
Open

[Bug]: contextmenu in V4 is not default-preventable #2658

ldhasson opened this issue May 10, 2024 · 11 comments

Comments

@ldhasson
Copy link

Current versus expected behaviour

In v3.4, we had a context menu handler on an element:

      this._paper.on('element:contextmenu', function(cellView, evt, x, y) {
         evt.stopPropagation();
         evt.preventDefault();
         var str = '<LI>'+(cellView.model.get('showKeys'   ) != true ? 'Show Keys'    : 'Hide Keys')+'</LI>'
                  +'<LI>'+(cellView.model.get('showColumns') != true ? 'Show Columns' : 'Hide Columns')+'</LI>'
                  +'<LI>Remove from Canvas</LI>'
                  ;
         showContextMenu(str, evt.pageX, evt.pageY);
      });

When inspecting the event, it was the proper "contextmenu" event, and so prevending default and stopping propagation would work in preventing the default context menu from the browser.

image

In V4 (we are upgrading!!!) this behavior no longer works. The issue i believe is that the event is not the "mousedown" event which triggers the contextmenu logic in joinjs. However, the default browser contextmenu responds to the "mouseup" event. As such, the stopPropagation and preventDefault calls have no effect because we are on the "wrong" event?

image

I couldn't not find documentation about this change of behavior and wonder if this is an error due to the removal of jQuery which was handling such things?

Steps to reproduce

Adding a contextmenu on an element cannot prevent default browser behavior

Version

4.0.2

What browsers are you seeing the problem on?

Firefox, Chrome, Microsoft Edge

What operating system are you seeing the problem on?

Windows

@ldhasson ldhasson added the bug label May 10, 2024
@kumilingus
Copy link
Contributor

It has nothing to do with jQuery removal. This was introduced in v3.6.0 in #1727.

Can you use paper.options.preventContextMenu = true?

@ldhasson
Copy link
Author

ldhasson commented May 11, 2024

Ah, ok. I remember that jquery had some sophisticated event mappings at some point, so i guessed (wrongly) it might be related to that. We moved from V3.4 straight to v4, so missed this.

I tried your suggestion but it didn't work. Am i doing something wrong?

     this._graph = new joint.dia.Graph();
     this._paper = new joint.dia.Paper({
        el: this._canvasElement.childNodes[0]
       ,width: "5000px"
       ,height: "5000px"
       ,model: this._graph
       ,gridSize: 10
     });
     this._paper.options.preventContextMenu = true;

      this._paper.on('element:contextmenu', function(cellView, evt, x, y) {
         evt.stopPropagation();
         evt.preventDefault();
         var str = '<LI>'+(cellView.model.get('showKeys'   ) != true ? 'Show Keys'    : 'Hide Keys')+'</LI>'
                  +'<LI>'+(cellView.model.get('showColumns') != true ? 'Show Columns' : 'Hide Columns')+'</LI>'
                  +'<LI>Remove from Canvas</LI>'
                  ;
         showContextMenu(str, evt.pageX, evt.pageY);
      });

I also tried for good measure

     this._paper = new joint.dia.Paper({
        el: this._canvasElement.childNodes[0]
       ,width: "5000px"
       ,height: "5000px"
       ,model: this._graph
       ,gridSize: 10
      ,options: { preventContextMenu: true }
     });

and

     this._paper = new joint.dia.Paper({
        el: this._canvasElement.childNodes[0]
       ,width: "5000px"
       ,height: "5000px"
       ,model: this._graph
       ,gridSize: 10
       ,preventContextMenu: true
     });

The default browser context menu still shows. I am testing on Windows with Chrome, Firefox and Edge.. I did not try this on Mac or other environments.

Thank you!

@kumilingus
Copy link
Contributor

Is it happening in this simple example? Note that preventContextMenu is true by default.

https://jsfiddle.net/kumilingus/pv087q1w/

@ldhasson
Copy link
Author

Hello Roman. This example is working as expected... I have to check this in more details as i am not seeing a difference with our code. This is very strange. Will investigate further and report back. Thank you!

@ldhasson
Copy link
Author

Hello Roman.

I was able to reproduce my issue, but i am dumbfounded as to why that is happening. I have made simple updates to your fiddle here: https://jsfiddle.net/Lqjhy1v8/6/

What i found is that if you uncomment the two lines

//  e.style.left=evt.pageX+"px";
//  e.style.top=evt.pageY+"px";

then you can reproduce my issue. With those 2 lines commented out, all is good.

Looking at the browser, it looks like the new div is shown and positioned under the cursor, and the mouseup event then fires on that div, short-circuiting JointJS's internal mechanics for the events?

I am not sure if you'd classify this as a jointJS issue or not (i think it is), but thinking of work-arounds for me:

  • adding a delay for the popup to show with a setTimeout.
  • making the popup window itself right-click-proof.

I'll try those later.

Is there a best practice in jointJS for context menus? I can't be the only person doing context menus with this issue?

Thank you!

@kumilingus
Copy link
Contributor

We show a context menu e.g. in our Kitchen Sink application. The difference is that we append the context menu as a paper's child (so it scrolls with the paper). That's why the paper prevents the default for the subsequent event I assume.

I would like to first understand why it works in v.3.5.5 (and not in v3.6.0).

Another workaround is to position the context menu 1px to the left and 1px to the bottom from the pointer:

e.style.left = `${evt.pageX+1}px`;
e.style.top = `${evt.pageY+1}px`;

Here's the same JSFiddle using JointJS v3.5 https://jsfiddle.net/kumilingus/rku5wsme/ (preventing contextmenu works fine)
It can be reproduced on MacOS too.

@ldhasson
Copy link
Author

ldhasson commented May 15, 2024 via email

@kumilingus
Copy link
Contributor

It was to unify the behaviour between Windows and macOS: #1590

Windows - contextmenu fired on mouseup
Mac - contextmenu fired on mousedown

Also, it seems that people have experienced the same issues in past: #1891

@ldhasson
Copy link
Author

OK Roman, maybe i can help. Can you direct me to the piece of code where you handle these events so i can have a look?

Many developers use the mouse events and check if the right button has been clicked. There is a good discussion here: https://stackoverflow.com/questions/76047602/determine-if-a-mousedown-will-trigger-contextmenu-in-js.

The most important part is that there is a native browser event "contextmenu" because technically, the context menu may be invoked via key shortcuts (no mouse events, and possibly no target) for example.

https://developer.mozilla.org/en-US/docs/Web/API/Element/contextmenu_event

I don't know how your code works, but i can have a look and maybe i can offer an option or two?

Thank you.

@kumilingus
Copy link
Contributor

The most important part is that there is a native browser event "contextmenu" because technically, the context menu may be invoked via key shortcuts (no mouse events, and possibly no target) for example.

That's a good point. Normalizing the contextmenu event seems to be a bad decision.

An ideal solution could be to:

  • use native contextmenu event for cell:contextmenu and blank:contextmenu events.
  • trigger cell:pointerdown, cell:pointermove and cell:pointerup no matter what button was triggered (also valid for blank:* events). We currently do not trigger the events.

This would allow users:

  • to always have access to the native contextmenu event
  • be able to drag & drop paper with right button
    paper.on('blank:pointerdown', (evt) => { evt.button === 2 && scroller.startPanning(evt); });

This would greatly simplify the code.
The users could use guard paper callback to prevent cell:pointer** events from triggering when the right button is clicked.

It’s a breaking change though. So we would need to bump the version.

A non-breaking change would be to add an option to switch between the current and the new contextmenu behavior:

new dia.Paper({ normalizeContextmenuEvent: false });

@ldhasson
Copy link
Author

I agree 100%. This is more in line with the browser architecture and less surprising to a web developer.

As for how to implement, it's a tough one. One solution breaks, while the other one adds complexity and messier code (as you have to keep both options available). As the library owner, i think this is for you to decide :)

My preference is to break in general to keep the long-term cleanliness of the code in check. Also, 4.0 is brand new so it feels like a breaking change now will have a lesser impact to the community for a 4.1, which i expect would see the growth of adoption really picking up?

Thank you,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants