feat(doubleClick): Add Double click detection (#375) #382

jbocce · 2023-01-23T18:50:30Z

Added a timeout in mouseDownListener to determine if a double click occurs. If a double click occurs during the timeout,
then any mouse down and up events that occurred during the timeout are suppressed. If the timeout expires, then the first mouse down and up events during the timeout are fired and any browser double click event that might occur is suppressed/ignored. Double clicks that occur during tool interaction are suppressed Double click event capture phase listeners are used to more reliably suppress double clicks.

The related issue is: #375

Added a timeout in mouseDownListener to determine if a double click occurs. If a double click occurs during the timeout, then any mouse down and up events that occurred during the timeout are suppressed. If the timeout expires, then the first mouse down and up events during the timeout are fired and any browser double click event that might occur is suppressed/ignored. Double clicks that occur during tool interaction are suppressed Double click event capture phase listeners are used to more reliably suppress double clicks.

netlify · 2023-01-23T18:50:37Z

✅ Deploy Preview for cornerstone-3d-docs ready!

Name	Link
🔨 Latest commit	`2af6f67`
🔍 Latest deploy log	https://app.netlify.com/sites/cornerstone-3d-docs/deploys/63d28e2019d28f0009510232
😎 Deploy Preview	https://deploy-preview-382--cornerstone-3d-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

jbocce · 2023-01-23T18:51:35Z

packages/tools/examples/doubleClickWithStackAnnotationTools/index.ts

@@ -0,0 +1,256 @@
+import {


A new example for double click.

jbocce · 2023-01-23T21:08:40Z

packages/tools/src/eventListeners/mouse/mouseDownListener.ts

@@ -9,6 +9,8 @@ import getMouseEventPoints from './getMouseEventPoints';
 const { MOUSE_DOWN, MOUSE_DOWN_ACTIVATE, MOUSE_CLICK, MOUSE_UP, MOUSE_DRAG } =
  Events;

+const DOUBLE_CLICK_TOLERANCE_MS = 400;


What is the best way to make this configurable?

I guess we can worry about this in another PR, since we are not configuring any other suff

mouseDownListener - even during annotation creation. It is up to the any listener of the dblclick event to decide how it should be handled.

…ick will not occur

sedghi · 2023-01-25T02:08:47Z

utils/test/testUtilsMouseEvents.ts

+/**
+ * Asynchronously dispatches a mouse down followed by a mouse up on the given element.
+ * Since mouse down events are performed on a timeout to detect potential
+ * double clicks, the mouse up event is not triggered until the mouse down
+ * event has been processed. An optional callback is invoked after the mouse
+ * down is triggered but before the mouse up. An optional callback is invoked
+ * after the mouse up has fired.
+ *
+ * @param element the element to dispatch to
+ * @param mouseDownEvent the mouse down event to dispatch
+ * @param mouseUpEvent the mouse up event to dispatch
+ * @param betweenDownAndUpCallback optional callback between the down and up
+ * @param afterDownAndUpCallback optional callback after the up
+ * @returns a Promise for the eventual completion of the mouse down and up
+ */


Love this doc, thanks

sedghi · 2023-01-25T02:09:42Z

utils/test/testUtilsMouseEvents.ts

+ * Asynchronously dispatches a mouse down followed by a mouse up on the given element.
+ * Since mouse down events are performed on a timeout to detect potential
+ * double clicks, the mouse up event is not triggered until the mouse down
+ * event has been processed. An optional callback is invoked after the mouse
+ * down is triggered but before the mouse up. An optional callback is invoked
+ * after the mouse up has fired.
+ *
+ * @param element the element to dispatch to
+ * @param mouseDownEvent the mouse down event to dispatch
+ * @param mouseUpEvent the mouse up event to dispatch
+ * @param betweenDownAndUpCallback optional callback between the down and up
+ * @param afterDownAndUpCallback optional callback after the up
+ * @returns a Promise for the eventual completion of the mouse down and up
+ */
+function doMouseDownAndUp(


can we rename this function to performMouseDownAndUp?

as you wish

sedghi · 2023-01-25T02:10:25Z

packages/tools/test/BidirectionalTool_test.js

@@ -1,6 +1,8 @@
 import * as cornerstone3D from '@cornerstonejs/core';
 import * as csTools3d from '../src/index';
 import * as testUtils from '../../../utils/test/testUtils';
+import * as cornerstoneTools from '@cornerstonejs/tools';


not a big deal, but just saying for future, let's order the library imports at top and the local ones ('../../etc') after

Ok, I will try to be more careful next time. Thanks for the 'heads up'. I am not 100% certain, but I think it might have been VS Code that did this automatically. Funny enough, that import is no longer needed.

sedghi · 2023-01-25T02:11:45Z

packages/tools/test/LengthTool_test.js

+
+        doMouseDownAndUp(
+          element,
+          evt,


I know evt, is mouseDown, but renaming the variable to mouseDownEvt would match the mouseUpEvt nicelu

Sure. I will go through the various unit test files and make updates.

sedghi · 2023-01-25T02:22:47Z

packages/tools/src/tools/annotation/ArrowAnnotateTool.ts

@@ -446,6 +450,10 @@ class ArrowAnnotateTool extends AnnotationTool {

    this.editData = null;
    this.isDrawing = false;
+
+    // This double click was handled and the dialogue was displayed. No need for anyone else to handle it too.
+    evt.stopImmediatePropagation();


why stopImmediatePropagation as opposed to stopPropagation? not immediately clear to me so I appreciate an inline comment above it

sedghi · 2023-01-25T02:23:34Z

packages/tools/src/eventListeners/mouse/index.ts

+  element.addEventListener('dblclick', mouseDoubleClickIgnoreListener, {
+    capture: true, // capture phase is the best way to ignore double clicks
+  });


is there any reason we are adding again the dblclick listener?

Yes there are two reasons...

If the logic of mouseDoubleClickIgnoreListener is moved into mouseDoubleClickListener, then the ignoreDoubleClick flag in mouseDownListener has to be exposed. Furthermore its mutation from true to false also has to be exposed. I do not particularly like that.

Notice that a capture phase listener is used which mouseDoubleClickListener is not. To best prevent listeners of dblclick to NOT get an ignored double click, a capture phase listener is best.

Then can we put them as inline comment?

Most certainly.

Actually I will move this logic code out of this index.ts and put it in mouseDownListener because none of it needs to be exposed outside that module.

sedghi · 2023-01-25T02:25:29Z

packages/tools/src/eventListeners/mouse/mouseDoubleClickListener.ts

@@ -35,7 +35,21 @@ function mouseDoubleClickListener(evt: MouseEvent): void {
    deltaPoints,
  };

-  triggerEvent(element, Events.MOUSE_DOUBLE_CLICK, eventDetail);
+  const preventDefault = !triggerEvent(


can we rename this to consumed or similar?

or eventDidNotPropagate

or even better const eventDidPropagate = trigger ...

Note that triggerEvent in turn calls dispatchEvent. I checked the mdn docs for dispatchEvent (https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/dispatchEvent) and its return value has NOTHING to do with propagation. In fact it is related to preventDefault. So I still prefer the name I had. However of your choices I think consumed is best.

sedghi · 2023-01-25T02:29:57Z

packages/tools/src/eventListeners/mouse/mouseDownListener.ts

+  doubleClickState.doubleClickTimeout = setTimeout(
+    _doubleClickTimeoutHandler,
+    DOUBLE_CLICK_TOLERANCE_MS
+  );


shouldn't this be the first thing right after the following?

if (doubleClickState.doubleClickTimeout) { return; }

No I don't understand why it should be. The listeners that follow have to be added. That said I can move the addListener calls before the setTimeout call and the method will return right away regardless. However, I am still wondering why the suggestion.

I moved the setting of the doubleClickTimeout to near the top of the method.

sedghi · 2023-01-25T02:32:50Z

packages/tools/src/eventListeners/mouse/mouseDownListener.ts

@@ -102,8 +134,30 @@ function mouseDownListener(evt: MouseEvent) {
  // Prevent CornerstoneToolsMouseMove while mouse is down
  state.element.removeEventListener('mousemove', mouseMoveListener);

-  const startPoints = getMouseEventPoints(evt, state.element);
-  const deltaPoints = _getDeltaPoints(startPoints, startPoints);
+  state.startPoints = getMouseEventPoints(evt, state.element);


we were doing copyPoints previously, so I'm wondering not copying will result some bug due to reference manipulation

At some point I did have it so that state.startPoints would get a copy. However, the CPU tests began to fail. The short answer to all this is that I can return to using _copyPoints here if StackViewport.canvasToWorldCPU returns [worldPos[0], worldPos[1], worldPos[2]] as Point3 instead of worldPos as Point3. Why? Because worldPos (for the CPU tests anyway) is a vec3/Float32Array which _copyPoints converts to a JSON object and NOT an array. Thus any operation expecting an array (in the case I saw it was the spread operator for copying the array) will fail.

There are other ways I could fix this. Let me know your thoughts. I suppose the safest thing might be to change _copyPoints to be...

const copy = JSON.parse(JSON.stringify(points)); copy.world = [copy.world[0], copy.world[1], copy.world[2]] as Point3; return copy;

packages/tools/src/eventListeners/mouse/mouseDownListener.ts

sedghi · 2023-01-25T02:45:25Z

packages/tools/src/eventListeners/mouse/mouseDownListener.ts

+ */
+function _isDragPastDoubleClickTolerance(delta: Types.Point2): boolean {
+  return (
+    Math.sqrt(Math.pow(delta[0], 2) + Math.pow(delta[1], 2)) >


This is too much to get calculated for each mouse move, a lot of computations. Can we make it simpler to not rely on sqrt and pow?

I understand and yes I can change it. However note that this is only for every move during the timeout. The timeout is relatively short. Also, once a drag/move goes past the tolerance the timeout is cleared. I would argue that yes it is expensive but for a very short time - likely less than the timeout if the mouse is moving any significant amount. Would you still like me to change it?

sedghi · 2023-01-25T16:25:51Z

packages/tools/src/eventListeners/mouse/mouseDownListener.ts

+  const mouseDownEvent = doubleClickState.mouseDownEvent;
+  const mouseUpEvent = doubleClickState.mouseUpEvent;
+
+  _clearDoubleClickTimeoutAndEvents();


maybe we can put ignore here

Yes we can and as such I will be removing the doubleClickTimeoutHandler function entirely since all it does is call _doStateMouseDownAndUp. Great suggestion.

sedghi · 2023-01-25T16:28:19Z

packages/tools/src/eventListeners/mouse/mouseDownListener.ts

+      state = JSON.parse(JSON.stringify(defaultState));
+    } else {
+      // this is the first mouse up during the double click timeout; we'll need it later if the timeout expires
+      doubleClickState.mouseUpEvent = evt;


switch order

- code comments added and modified for clarification - the double click listener to ignore double click events is now entirely encapsulated in the mouseDownListener module - better named variables in the unit tests - better conversion of world CPU coordinates to Point3

jbocce · 2023-01-25T21:03:17Z

packages/tools/src/eventListeners/mouse/mouseDownListener.ts

+// value will cancel the timeout and suppress any double click that might occur.
+// This tolerance is particularly important on touch devices where some movement
+// might occur between the two clicks.
+const DOUBLE_CLICK_DRAG_TOLERANCE = 3;


The last commit had this as 2. I did some further testing on my phone and thought 3 was better. I will add a TODO to revisit this value for touch devices.

…stener with other mouse events

jbocce · 2023-01-26T12:24:28Z

packages/tools/src/eventListeners/mouse/index.ts

+  // A separate double click listener for the element. Separate because...
+  // - it listens on the capture phase (and not the typical bubble phase)
+  // - the data used to ignore the double click is private to mouseDoubleClickIgnoreListener
+  element.removeEventListener('dblclick', mouseDoubleClickIgnoreListener, {


This adds the listener for every enabled viewport. It is a bit of a waste since only one (global) listener is required.

and destroy functions (respectively). Double click drag tolerance is now a projected distance.

jbocce commented Jan 23, 2023

View reviewed changes

jbocce marked this pull request as ready for review January 23, 2023 18:52

jbocce requested a review from sedghi January 23, 2023 18:52

Fixing syntax error in example-info.json.

1ae84b5

jbocce commented Jan 23, 2023

View reviewed changes

Joe Boccanfuso added 2 commits January 24, 2023 10:55

PR feedback: CS3D should not suppress a double click it has detected in

1e9850a

mouseDownListener - even during annotation creation. It is up to the any listener of the dblclick event to decide how it should be handled.

PR feedback - using a drag tolerance to help determine if a double cl…

b26eab3

…ick will not occur

sedghi reviewed Jan 25, 2023

View reviewed changes

packages/tools/src/eventListeners/mouse/mouseDownListener.ts Outdated Show resolved Hide resolved

sedghi reviewed Jan 25, 2023

View reviewed changes

PR updates:

0c22936

- code comments added and modified for clarification - the double click listener to ignore double click events is now entirely encapsulated in the mouseDownListener module - better named variables in the unit tests - better conversion of world CPU coordinates to Point3

jbocce commented Jan 25, 2023

View reviewed changes

Joe Boccanfuso added 2 commits January 25, 2023 16:04

Added a TODO for the double click tolerance for touch devices.

d25fc52

PR feedback - restored adding and removing the double click ignore li…

c69e1ec

…stener with other mouse events

jbocce mentioned this pull request Jan 26, 2023

Double click detection #375

Closed

Adding the ignore double click listener back on the element.

fa2f2d7

jbocce commented Jan 26, 2023

View reviewed changes

Adding and removing the ignore double click listener in the tools init

2af6f67

and destroy functions (respectively). Double click drag tolerance is now a projected distance.

sedghi approved these changes Jan 26, 2023

View reviewed changes

sedghi changed the title ~~fix(mouseDownListener): Double click detection (#375)~~ feat(doubleClick): Add Double click detection (#375) Jan 26, 2023

sedghi merged commit 8e4be96 into cornerstonejs:main Jan 26, 2023

jbocce deleted the fix/#375-double-click-detection branch January 27, 2023 23:24

feat(doubleClick): Add Double click detection (#375) #382

feat(doubleClick): Add Double click detection (#375) #382

Conversation

jbocce commented Jan 23, 2023 • edited Loading

netlify bot commented Jan 23, 2023 • edited Loading

✅ Deploy Preview for cornerstone-3d-docs ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbocce Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbocce commented Jan 23, 2023 •

edited

Loading

netlify bot commented Jan 23, 2023 •

edited

Loading

jbocce Jan 25, 2023 •

edited

Loading