fix: Focus nearest neighbor when deleting a focused block#9599
fix: Focus nearest neighbor when deleting a focused block#9599
Conversation
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Thanks @gonfunko! Largely LGTM but had a few thoughts--PTAL.
| setTimeout(() => focusManager.focusTree(this.workspace), 0); | ||
| const nearestNeighbour = this.getNearestNeighbour(); | ||
| if (nearestNeighbour) { | ||
| setTimeout(() => focusManager.focusNode(nearestNeighbour), 0); |
There was a problem hiding this comment.
Is there any case where there may be a sequence of deletions such that this triggers an attempt to focus the next closest node before it's deleted? Might be worth adding a test case to see if that's actually possible and, if so, make this a bit more robust against that.
| const block = this.workspace.getTopBlocks(false)[0]; | ||
| Blockly.getFocusManager().focusNode(block); | ||
| block.dispose(); | ||
| this.clock.runAll(); |
There was a problem hiding this comment.
Is this actually needed here & below?
| } | ||
|
|
||
| Blockly.getFocusManager().focusNode( | ||
| this.workspace.getTopBlocks(false)[0], |
There was a problem hiding this comment.
This relates to my other comment: perhaps double check that the nearest neighbor is still actually doing what it seems like and selecting and trying to focus a block before it's disposed. It may be the case that something else (maybe the focus system or another part of disposal) is robust against this failure case.
| }); | ||
|
|
||
| test('Bulk deleting blocks does not focus another dying block', function () { | ||
| for (let i = 0; i < 50; i++) { |
There was a problem hiding this comment.
Idea can probably be tested with fewer blocks (for perforamnce). Maybe just have something like 5?
| b.dispose(); | ||
| this.clock.runAll(); | ||
|
|
||
| assert.equal( |
There was a problem hiding this comment.
Here & elsewhere: I suggest using strictEqual instead since there can be problems with the deep equal for connected blocks (circular references are possible), and it shouldn't actually be necessary for verifying focus.
2 participants
The basics
The details
Resolves
Fixes #9585
Proposed Changes
This PR adjusts the focus behavior when a focused block is deleted. Previously, deleting a focused block would focus the workspace, which would in turn focus the first (left/topmost) block on the workspace. This could cause large jumps in scroll position. Now, when a focused block is deleted, it moves focus to the closest remaining block, resulting in a smaller (or no) workspace movement.
The tests were partially LLM-generated in response to a list of testcases I specified. I reviewed, modified them, and manually added an additional testcase.