Do not block shift-click mouse up handler on active cell #16647

EdsterG · Aug 6, 2024

References

Fixes #16633

Code changes

Don't prevent default behavior when the current cell is the active cell.

User-facing changes

shift+click in the same cell no longer swallows the mouseup event.

Backwards-incompatible changes

N/A

jupyterlab-probot · Aug 6, 2024

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link:

krassowski · Aug 7, 2024

Thank you, this looks good. I think we would also want to add a test case, somewhere here:

jupyterlab/packages/notebook/test/widget.spec.ts

Lines 1233 to 1273 in 653c226

    
           it('should extend selection if invoked with shift', () => { 
        
             widget.activeCellIndex = 3; 
        
             // shift click below 
        
             simulate(widget.widgets[4].node, 'mousedown', { shiftKey: true }); 
        
             expect(widget.activeCellIndex).toBe(4); 
        
             expect(selected(widget)).toEqual([3, 4]); 
        
             // shift click above 
        
             simulate(widget.widgets[1].node, 'mousedown', { shiftKey: true }); 
        
             expect(widget.activeCellIndex).toBe(1); 
        
             expect(selected(widget)).toEqual([1, 2, 3]); 
        
             // shift click expand 
        
             simulate(widget.widgets[0].node, 'mousedown', { shiftKey: true }); 
        
             expect(widget.activeCellIndex).toBe(0); 
        
             expect(selected(widget)).toEqual([0, 1, 2, 3]); 
        
             // shift click contract 
        
             simulate(widget.widgets[2].node, 'mousedown', { shiftKey: true }); 
        
             expect(widget.activeCellIndex).toBe(2); 
        
             expect(selected(widget)).toEqual([2, 3]); 
        
           }); 
        
           it('should not extend a selection if there is text selected in the output', () => { 
        
             const codeCellIndex = 3; 
        
             widget.activeCellIndex = codeCellIndex; 
        
             // Set a selection in the active cell outputs. 
        
             const selection = window.getSelection()!; 
        
             selection.selectAllChildren( 
        
               (widget.activeCell as CodeCell).outputArea.node 
        
             ); 
        
             // Shift click below, which should not extend cells selection. 
        
             simulate(widget.widgets[codeCellIndex + 2].node, 'mousedown', { 
        
               shiftKey: true 
        
             }); 
        
             expect(widget.activeCellIndex).toBe(codeCellIndex); 
        
             expect(selected(widget)).toEqual([]); 
        
           });

EdsterG · Aug 7, 2024

Thank you for the support.

EdsterG · Aug 7, 2024

I'm not sure why the tests are failing. After looking at this some more I think the test I added doesn't actually cover the new behavior. For the test case added in this PR, selected will be empty even without this PR.

What really needs to be tested is whether the widget is a registered mouse up event listener or whether the the mouse mode is null: expect(widget._mouseMode.toBe(null)). Also, the new test case needs to be moved to the top because shift clicking the first cell after expanding the selection is valid, and will still be listening to mouseup events.

krassowski

It looks like the parens are off

packages/notebook/test/widget.spec.ts

EdsterG · Aug 8, 2024

It looks like the parens are off

Oops, thanks! I'm currently not setup for JS dev, so no linters :(

EdsterG · Aug 8, 2024

Looks like the test linter doesn't like _mouseMode because it's private. What's the suggested work around here? Or an alternative to test the same thing.

packages/notebook/test/widget.spec.ts

krassowski · Aug 9, 2024

packages/notebook/src/widget.ts

+      // We don't want to block the shift-click mouse up handler
+      // when the current cell is the active cell.
+      const isActiveCell = index == this.activeCellIndex;
      if (
        button === 0 &&
        shiftKey &&
        !hasSelection &&
+        !isActiveCell &&


Giving it a bit more thought I think this has to be more subtle. The idea is that user can press Shift, press mouse down and start dragging cursor to select cells. Instead of short-circuiting here I think we should store the activeCellIndex if starts shift + mousedown on active cell and skip preventing default on _evtDocumentMouseup() if the cell index has not changed.

I will push a commit

packages/notebook/test/widget.spec.ts

krassowski

Thank you @EdsterG! Please let me know once you had a chance to look at the changes I've made and whether you have any comments.

packages/notebook/test/widget.spec.ts

EdsterG · Aug 9, 2024

Yay, thanks for pushing the changes. LGTM.

krassowski · Aug 9, 2024

@meeseeksdev please backport to 4.2.x

…er on active cell

…ve cell (#16663) Co-authored-by: Eddie Groshev <eddie_g_89@hotmail.com>

…16647) * check if current cell is the active cell * add unit test * update unit test * fix typo in unit tests * Do not use private variables in tests, use events * Do not prevent default on mouse + shift up if in the same cell * Fix the test * Adjust comment --------- Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>

check if current cell is the active cell

95de8e2

github-actions bot assigned EdsterG Aug 6, 2024

github-actions bot added the pkg:notebook label Aug 6, 2024

krassowski added the bug label Aug 7, 2024

krassowski added this to the 4.2.x milestone Aug 7, 2024

krassowski changed the title ~~check if current cell is the active cell~~ Do not block shift-click mouse up handler on active cell Aug 7, 2024

add unit test

c2b39cc

update unit test

6188b4b

krassowski reviewed Aug 8, 2024

View reviewed changes

packages/notebook/test/widget.spec.ts Outdated Show resolved Hide resolved

packages/notebook/test/widget.spec.ts Outdated Show resolved Hide resolved

fix typo in unit tests

86dcdc9

krassowski reviewed Aug 9, 2024

View reviewed changes

Do not use private variables in tests, use events

c0388e8

github-advanced-security bot found potential problems Aug 9, 2024

View reviewed changes

packages/notebook/test/widget.spec.ts Fixed Show fixed Hide fixed

packages/notebook/test/widget.spec.ts Fixed Show fixed Hide fixed

krassowski added 2 commits August 9, 2024 10:41

Do not prevent default on mouse + shift up if in the same cell

c275d54

Fix the test

8a9d879

krassowski approved these changes Aug 9, 2024

View reviewed changes

krassowski reviewed Aug 9, 2024

View reviewed changes

packages/notebook/test/widget.spec.ts Outdated Show resolved Hide resolved

Adjust comment

3bf72c6

krassowski merged commit 165d589 into jupyterlab:main Aug 9, 2024
83 checks passed

EdsterG deleted the fix_shift_click branch August 9, 2024 17:24

meeseeksmachine pushed a commit to meeseeksmachine/jupyterlab that referenced this pull request Aug 9, 2024

Backport PR jupyterlab#16647: Do not block shift-click mouse up handl…

0878fca

…er on active cell

meeseeksmachine mentioned this pull request Aug 9, 2024

Backport PR #16647 on branch 4.2.x (Do not block shift-click mouse up handler on active cell) #16663

Merged

krassowski pushed a commit that referenced this pull request Aug 9, 2024

Backport PR #16647: Do not block shift-click mouse up handler on acti…

1fa4474

…ve cell (#16663) Co-authored-by: Eddie Groshev <eddie_g_89@hotmail.com>

Search code, repositories, users, issues, pull requests...

Uh oh!

Do not block shift-click mouse up handler on active cell #16647

Do not block shift-click mouse up handler on active cell #16647

Uh oh!

Conversation

EdsterG commented Aug 6, 2024

References

Code changes

User-facing changes

Backwards-incompatible changes

Uh oh!

jupyterlab-probot bot commented Aug 6, 2024

Uh oh!

krassowski commented Aug 7, 2024

Uh oh!

EdsterG commented Aug 7, 2024

Uh oh!

EdsterG commented Aug 7, 2024

Uh oh!

krassowski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

EdsterG commented Aug 8, 2024

Uh oh!

EdsterG commented Aug 8, 2024

Uh oh!

Uh oh!

Uh oh!

krassowski Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

krassowski Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

krassowski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

EdsterG commented Aug 9, 2024

Uh oh!

Uh oh!

krassowski commented Aug 9, 2024

Uh oh!

Uh oh!