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

ldhasson commented 6 months ago

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.

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?

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

kumilingus commented 6 months ago

It has nothing to do with jQuery removal. This was introduced in v3.6.0 in https://github.com/clientIO/joint/pull/1727.

Can you use paper.options.preventContextMenu = true?

ldhasson commented 6 months ago

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 commented 6 months ago

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

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

ldhasson commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

Ok, I will check those examples :) thank you so much.

As to why the behavior changed, my analysis is so ple I think. Previously, the context menu event in JointJS was activated at the end of the sequence, i.e., mouseup. That's the event the browser responds to for its context menu. So preventing default on the mouseup did its job. Since v3.6, the context menu sequence mousedown+mouseup was broken down and contextmenu is activated for mousedown, leaving mouseup unhandled and inaccessible from the handler: the prevent default won't block mouseup which is coming next.

I wonder why the native event contextmenu wasn't handled directly instead of the mousedown+mouseup combo? Is it because of the complexities involved regarding Firefox and potentially being activated by shortcuts vs mouse behavior?

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

Thank you, Laurent Hasson

From: Roman Bruckner @.> Sent: Wednesday, May 15, 2024 04:31 To: clientIO/joint Cc: @.; Author Subject: Re: [clientIO/joint] [Bug]: contextmenu in V4 is not default-preventable (Issue #2658)

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.

— Reply to this email directly, view it on GitHubhttps://github.com/clientIO/joint/issues/2658#issuecomment-2111893870, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AADEHB4UZJAQSLPRMGPL4VDZCMMNVAVCNFSM6AAAAABHQWQSLOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJRHA4TGOBXGA. You are receiving this because you authored the thread.Message ID: @.***>

kumilingus commented 6 months ago

It was to unify the behaviour between Windows and macOS: https://github.com/clientIO/joint/issues/1590

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

Also, it seems that people have experienced the same issues in past: https://github.com/clientIO/joint/issues/1891

ldhasson commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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,

clientIO / joint