Contextmenu

[rowild]

Add contextmenu to waveforms and segment-shape.

[rowild]

Hi, Chris! I am still trying to implement contextmenu, for the sake of "feature completeness" I thought it might be good to add this functionality to the waveforms and the segment shapes, too. Problems arise, when contextmenu is used on pointer AND on the zoomview. Then, both implementations get fired. And for some reason the click event is also still recognised.

I was looking at eventEmitter3 (I think this is what peaks uses, right?) and if it offers any cancelation settings (stop bubbling). But so far no good...

The video shows, what happens in the console. Any ideas?

https://user-images.githubusercontent.com/213803/156645489-57225d4b-ce0e-40a0-8e1b-d62592e3eda2.mp4

PS: I should add a code example right here:

// Index.html

peaksInstance.on('segments.contextmenu', function(segment) {
  event.stopPropagation();
  event.preventDefault();
  console.log('segments.contextmenu:', segment);
});

PPS: Konva provides a cancelBubble option: https://konvajs.org/docs/events/Cancel_Propagation.html Not sure how to implement this...

[chrisn]

Problems arise, when contextmenu is used on pointer AND on the zoomview. Then, both implementations get fired.

The pointer and zoomview handlers are independent of each other, so I'm not sure yet how to make one have precedence over the other.

And for some reason the click event is also still recognised.

We could work around this by using the click event button attribute:

peaks.on("zoomview.contextmenu", (event) => {
  event.evt.preventDefault();
});

peaks.on("zoomview.click", (event) => {
  if (event.evt.button === 2) {
    // Right click
  }
  else {
    // Left click
  }
});

I was looking at eventEmitter3 (I think this is what peaks uses, right?) and if it offers any cancelation settings (stop bubbling).

Yes, we use EventEmitter3. You're right, it doesn't provide any cancellation settings.

[rowild]

Where would I have to add this code snippet? In "index.html" (examples)event.evt (...) does not work. The .evt. seems to be too much...

UPDATE: Ah, I see. That would be an addition to the core scripts, in the given case waveform-zoomview, around line 238?

If both, points.contextmenu and zoomview.contextmenu are defined, and a button detection would be integrated, than still both contextmenus would get fired, no matter how many cancelBubbles and bubbles and propagations are set to false...:

// index.html

  peaksInstance.on('points.contextmenu', function(point) {
    event.preventDefault();
    event.stopPropagation();
    event.stopImmediatePropagation();

    // // read-only, changing them does nothing
    // event.cancelBubble = true
    // event.bubbles = false

    if(event.button === 2) {
      console.log('points: right click detected');
    }
  });

  peaksInstance.on('zoomview.contextmenu', function(time) {
    event.stopPropagation();
    event.preventDefault();
    event.stopImmediatePropagation();

    if(event.button === 2) {
      console.log('zoomview: right click  detected');
    }
  });

[rowild]

My thinking is wrong: whether the contextmenu is already active or not should not be something that peaks should handle. Assuming that a marker will always be on top of the waveform, the points.contextmenu could simply set a variable, e.g.cmActive = true. The zoomview in turn would check, whether that variable is true or not and react accordingly:

// index.html

let cmActive = false

peaksInstance.on('points.contextmenu', point => {
  if(event.button === 2) {
    this.cmActive = true
    // call the right-click drop-down menu
  }
});

peaksInstance.on('zoomview.contextmenu', time => {
  if(event.button === 2) {
    if(!this.cmActive) {
      // Do something!
    } else {
      // Do nothing!
    }
  }
});

In my scenario this solution works fine. What do you think?

[rowild]

Hi, Chris,

I was wondering if you see a chance to implement the contextmenu option into peaks.

The current use case would be to add a simple delete function via right click to all the markers. Currently a double click is needed which opens a modal, and only there users can set a marker to deleted.

The Problems I address on github would "only" occur, if there was a right-click event assigned to the zoomview AND the markers at the same time. For those – probably very limited – cases the workaround I provided could be used.

Thanks for your feedback!

Robert

On Sat, Mar 5, 2022 at 5:59 PM Chris Needham @.***> wrote:

Problems arise, when contextmenu is used on pointer AND on the zoomview. Then, both implementations get fired.

The pointer and zoomview handlers are independent of each other, so I'm not sure yet how to make one have precedence over the other.

And for some reason the click event is also still recognised.

We could work around this by using the click event button attribute:

peaks.on("zoomview.contextmenu", (event) => { event.evt.preventDefault();}); peaks.on("zoomview.click", (event) => { if (event.evt.button === 2) { // Right click } else { // Left click }});

I was looking at eventEmitter3 (I think this is what peaks uses, right?) and if it offers any cancelation settings (stop bubbling).

Yes, we use EventEmitter3. You're right, it doesn't provide any cancellation settings.

— Reply to this email directly, view it on GitHub https://github.com/bbc/peaks.js/pull/444#issuecomment-1059796596, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABUGK32ZHVPQUAPI2WCR6TU6OHFNANCNFSM5P3N77FQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

[chrisn]

Sorry for the delay...

I should try to clarify my previous reply. With this code I'm suggesting that we should expose an event object rather than just the time position (as we do for click events), so you could do:

peaks.on("zoomview.contextmenu", (event) => {
  event.evt.preventDefault(); // event.evt exposes the DOM event
  console.log(event.time);
});

This is simiar to how Konva events work.

I notice your code uses window.event which I think we should avoid if we can (see MDN docs) - but I understand it's the only option right now.

Then, for consistency probably all events should have a similar interface (see https://github.com/bbc/peaks.js/pull/427). I haven't merged this yet as it's a breaking API change. I'm interested in your feedback on this idea.

For contextmenu specifically, an issue I've found is that when you right-click you get both events:

• zoomview.click
• zoomview.contextmenu

So I'm thinking that contextmenu would only be used to prevent the browser default behaviour and applications would then use the points.click or zoomview.click events and check event.evt.button as in:

peaks.on("zoomview.click", (event) => {
  if (event.evt.button === 2) {
    // Right click
    console.log(event.time); // Time position
  }
  else {
    // Left click
    console.log(event.time); // Time position
  }
});

You've changed your mind on whether Peaks should suppress zoomview.contextmenu if the user clicks on a point. I think it might make sense for Peaks to handle this itself, but happy to leave that to applications for now and revisit if we need to.

[rowild]

Good evening, Chris!

Thank you for your message! #427 https://github.com/bbc/peaks.js/pull/427 looks very interesting. Would that allow for recognising key combinations, e.g. SHIFT + Click or ALT + Drag? And would those key combinations be the same on a touch pad? If so, that would open up a universe of possibilities and bring "desktop experience / usability" to peaks. I vote with 2 thumbs up!

You write: You've changed your mind on whether Peaks should suppress zoomview.contextmenu if the user clicks on a point. I think it might make sense for Peaks to handle this itself, but happy to leave that to applications for now and revisit if we need to.

Of course I agree with you and would find it "consistent" and logical, if peaks could handle overlapping point/zoomview scenarios. You mentioned that this might be a problem, since click also manages seek. Not sure if that would be true for contextmenu. If, as you write, contextmenu would simply be there to suppress the standard behaviour, and the rest is handled via the recognition of the button number, the consequence would be that every click would have to run through an if statement, right? I wonder how much effort that would be for you to integrate (is there more code to change than shown in #427?) and if that would be a performance impact?

Also, the "overlap" behaviour would also have to be checked on "double key" events (e.g. SHIFT + click), right? In case somebody prefers to implement a context menu using SHIFT + click, the chance a point and the zoomview react at the same time is still given, if I am not overthinking it...

In any case: I would totally support those features! 2 thumbs and 2 paws (from my neighbour's dog) up! But I could also dispense with contextmenu, if key combinations (that also work on touch pads) would work.

How can I help besides testing? What are the implications of breaking changes? Sooner or later there usually are breaking changes. Wouldn't a 1.0.0 release help to not destroy existing implementations (through auto update)?

Eager to read/hear what you decide to do!

Thanks again! Till soon! Robert

On Fri, Mar 11, 2022 at 10:04 PM Chris Needham @.***> wrote:

Sorry for the delay...

I should try to clarify my previous reply. With this code I'm suggesting that we should expose an event object rather than just the time position (as we do for click events), so you could do:

peaks.on("zoomview.contextmenu", (event) => { event.evt.preventDefault(); // event.evt exposes the DOM event console.log(event.time);});

This is simiar to how Konva events work.

I notice your code uses window.event which I think we should avoid if we can (see MDN docs https://developer.mozilla.org/en-US/docs/Web/API/Window/event) - but I understand it's the only option right now.

Then, for consistency probably all events should have a similar interface (see #427 https://github.com/bbc/peaks.js/pull/427). I haven't merged this yet as it's a breaking API change. I'm interested in your feedback on this idea.

For contextmenu specifically, an issue I've found is that when you right-click you get both events:

• zoomview.click
• zoomview.contextmenu

So I'm thinking that contextmenu would only be used to prevent the browser default behaviour and applications would then use the points.click or zoomview.click events and check event.evt.button as in:

peaks.on("zoomview.click", (event) => { if (event.evt.button === 2) { // Right click } else { // Left click }});

You've changed your mind on whether Peaks should suppress zoomview.contextmenu if the user clicks on a point. I think it might make sense for Peaks to handle this itself, but happy to leave that to applications for now and revisit if we need to.

— Reply to this email directly, view it on GitHub https://github.com/bbc/peaks.js/pull/444#issuecomment-1065521762, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABUGKYARVHBEJHOSJOVA3DU7OYL5ANCNFSM5P3N77FQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

[rowild]

My lack of experience of working with git in a team now shows itself. Usually, when a PR is merged, github tells me that I can delete m branch. But this is not the case here. So even though I think that you merged your and my scripts, I am not sure if I can delete my branch.

The reason I ask is, because I need to implement this lib on Vercel. They do support private repos, but only from the master branch, if I am not mistaken. And my master branch is "2 commits ahead". I must have something committed sth on the master branch (which is a no-go on forks, I know).

Any hint what my next steps should be? What would happen, if I deleted my fork now? would this mess up some of your current work on peaks?

[chrisn]

I haven't merged this PR yet. GitHub says there are some conflicts to resolve. I'll fix those and merge, then publish a new release from the master branch.

[rowild]

I cloned peaks, installed dependencies, build it, then npm link, and in my project folder npm link peaks.js. Unfortunately the changed events do not work.

When I add

/**
* Event on a point: click, dblclick, enter,
*/
this.peaks.on('points.contextmenu', function (event) {
    event.evt.preventDefault();

    console.log('points.contextmenu:', event);
});

nothing happens. Same thing for my updated scripts.

// Old
this.peaks.on('points.dblclick', point => {
  console.log('points.dblclick point =', point);
  if (point.id !== 'peaks.point.1' && point.id !== 'peaks.point.2') {
    $nuxt.$emit('showModalForDescriptors', point)
  }
})

// New
this.peaks.on('points.dblclick', event => {
  console.log('points.dblclick event =', event);
  if (event.point.id !== 'peaks.point.1' && event.point.id !== 'peaks.point.2') {
    $nuxt.$emit('showModalForDescriptors', event.point)
  }
})

[rowild]

Oh... overlap! Sorry!

[chrisn]

This is now merged. Thank you for contributing! :+1: