Backport PR #16647: Do not block shift-click mouse up handler on active cell (#16663)

meeseeksmachine · EdsterG · web-flow · commit 1fa44740bb80 · 2024-08-09T21:51:27.000+01:00
Co-authored-by: Eddie Groshev &lt;eddie_g_89@hotmail.com&gt;
diff --git a/packages/notebook/src/widget.ts b/packages/notebook/src/widget.ts
@@ -2487,6 +2487,13 @@ export class Notebook extends StaticNotebook {
         }
         // Enter selecting mode
         this._mouseMode = 'select';
+
+        // We don't want to block the shift-click mouse up handler
+        // when the current cell is (and remains) the active cell.
+        this._selectData = {
+          startedOnActiveCell: index == this.activeCellIndex,
+          startingCellIndex: this.activeCellIndex
+        };
         document.addEventListener('mouseup', this, true);
         document.addEventListener('mousemove', this, true);
       } else if (button === 0 && !shiftKey) {
@@ -2532,8 +2539,21 @@ export class Notebook extends StaticNotebook {
    * Handle the `'mouseup'` event on the document.
    */
   private _evtDocumentMouseup(event: MouseEvent): void {
-    event.preventDefault();
-    event.stopPropagation();
+    const [, index] = this._findEventTargetAndCell(event);
+
+    let shouldPreventDefault = true;
+    if (this._mouseMode === 'select' && this._selectData) {
+      // User did not move the mouse over to a difference cell, so there was no selection
+      const { startedOnActiveCell, startingCellIndex } = this._selectData;
+      if (startedOnActiveCell && index === startingCellIndex) {
+        shouldPreventDefault = false;
+      }
+      this._selectData = null;
+    }
+    if (shouldPreventDefault) {
+      event.preventDefault();
+      event.stopPropagation();
+    }
 
     // Remove the event listeners we put on the document
     document.removeEventListener('mousemove', this, true);
@@ -2542,8 +2562,6 @@ export class Notebook extends StaticNotebook {
     if (this._mouseMode === 'couldDrag') {
       // We didn't end up dragging if we are here, so treat it as a click event.
 
-      const [, index] = this._findEventTargetAndCell(event);
-
       this.deselectAll();
       this.activeCellIndex = index;
       // Focus notebook if active cell changes but does not have focus.
@@ -2955,6 +2973,10 @@ export class Notebook extends StaticNotebook {
     pressY: number;
     index: number;
   } | null = null;
+  private _selectData: {
+    startedOnActiveCell: boolean;
+    startingCellIndex: number;
+  } | null = null;
   private _mouseMode: 'select' | 'couldDrag' | null = null;
   private _activeCellChanged = new Signal<this, Cell | null>(this);
   private _stateChanged = new Signal<this, IChangedArgs<any>>(this);
diff --git a/packages/notebook/test/widget.spec.ts b/packages/notebook/test/widget.spec.ts
@@ -1233,6 +1233,21 @@ describe('@jupyter/notebook', () => {
         it('should extend selection if invoked with shift', () => {
           widget.activeCellIndex = 3;
 
+          // shift click no-op
+          simulate(widget.widgets[3].node, 'mousedown', { shiftKey: true });
+          expect(widget.activeCellIndex).toBe(3);
+          expect(selected(widget)).toEqual([]);
+          // test that selecting mode handler does not prevent default
+          // if no cells were selected; in the selecting mode we listen
+          // to the `mouseup` event to stop selecting when the mouse button gets
+          // released; this event gets default prevented as handled by the notebook.
+          const mouseUpEvent = new MouseEvent('mouseup', {
+            bubbles: true,
+            cancelable: true
+          });
+          widget.widgets[3].node.dispatchEvent(mouseUpEvent);
+          expect(mouseUpEvent.defaultPrevented).toBe(false);
+
           // shift click below
           simulate(widget.widgets[4].node, 'mousedown', { shiftKey: true });
           expect(widget.activeCellIndex).toBe(4);
@@ -1252,6 +1267,27 @@ describe('@jupyter/notebook', () => {
           simulate(widget.widgets[2].node, 'mousedown', { shiftKey: true });
           expect(widget.activeCellIndex).toBe(2);
           expect(selected(widget)).toEqual([2, 3]);
+
+          // shift click deselect
+          simulate(widget.widgets[3].node, 'mousedown', { shiftKey: true });
+          expect(widget.activeCellIndex).toBe(3);
+          expect(selected(widget)).toEqual([]);
+
+          // shift click select by dragging from active cell
+          expect(widget.activeCellIndex).toBe(3);
+          simulate(widget.widgets[3].node, 'mousedown', { shiftKey: true });
+          expect(widget.activeCellIndex).toBe(3);
+          simulate(widget.widgets[4].node, 'mousemove', { shiftKey: true });
+          expect(selected(widget)).toEqual([3, 4]);
+          // test that selecting mode mouse up handler prevents default;
+          // in selecting mode we listen to the `mouseup` event to stop
+          // selecting when the mouse button gets released
+          const blockedMouseUpEvent = new MouseEvent('mouseup', {
+            bubbles: true,
+            cancelable: true
+          });
+          widget.widgets[4].node.dispatchEvent(blockedMouseUpEvent);
+          expect(blockedMouseUpEvent.defaultPrevented).toBe(true);
         });
 
         it('should not extend a selection if there is text selected in the output', () => {
