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.
packages/blockly/core/block_svg.ts
Outdated
| 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.
There was a problem hiding this comment.
That is a possibility, and is why getNearestNeighbour() filters the blocks by isDeadOrDying(), so that it only returns a block that isn't about to be disposed of. This is also the purpose of the test that makes 50 blocks and clears the workspace.
| 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?
There was a problem hiding this comment.
Yes, this test file mocks the clock and this gets the various async focus things to happen.
| } | ||
|
|
||
| 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.
There was a problem hiding this comment.
Adjusted this is a bit to verify that it does not focus dying blocks.
| }); | ||
|
|
||
| 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.
There was a problem hiding this comment.
Oh nice, that's a big improvement. We should probably take a pass to update all the tests to do this where relevant to avoid locking things up when there's a failure.
|
This also fixes #9601 |
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.