fix: Ensure selection stays when dragging blocks, comments, and bubbles #9034
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Is there a bug for "At the time of the PR being opened, this couldn't be tested in the test environment for the experimental keyboard navigation plugin since there's a navigation connection issue there that needs to be resolved to test movement"? |
core/dragging/block_drag_strategy.ts
Outdated
|
|
||
| // Since moving the block to the drag layer will cause it to lose focus, | ||
| // ensure it regains focus (to enable the block's selection highlight). | ||
| getFocusManager().focusNode(this.block); |
There was a problem hiding this comment.
Question: should this be done in moveToDragLayer (and moveOffDragLayer) instead of in the drag strategy?
There was a problem hiding this comment.
Good idea. This actually made me realize that workspace comments had the same problem, so this shift actually fixes those, too.
May address a separate block case that was missed before.
I don't think there was a bug but google/blockly-keyboard-experimentation#516 fortunately fixed the underlying issue so I was able to properly test this with keyboard navigation & fix another bug that I found: google/blockly-keyboard-experimentation#521. I've updated both the code and PR description accordingly, PTAL. Edit: Though there are failing tests--taking a look. |
tests/mocha/layering_test.js
Outdated
| @@ -80,7 +89,7 @@ suite('Layering', function () { | |||
| }); | |||
|
|
|||
| suite('dragging', function () { | |||
| test('moving an element to the drag layer adds it to the drag group', function () { | |||
| test.only('moving an element to the drag layer adds it to the drag group', function () { | |||
There was a problem hiding this comment.
Reset to test( so you're not skipping all the other tests.
There was a problem hiding this comment.
I forget this far too often until I do my self-review. :) Thanks & removed in latest.
|
Looks like tests are now both running and passing. PTAL @rachel-fenichel. |
The basics
The details
Resolves
Fixes #9027
Fixes google/blockly-keyboard-experimentation#521
Proposed Changes
Ensure that a block & workspace comment being dragged are properly focused mid-drag.
Reason for Changes
Focus seems to be lost due to the element being moved to the drag layer, so re-focusing the element ensures that it remains both actively focused and selected while dragging. This applies to both blocks and workspace comments, and theoretically bubbles (though that's only for an event basis since they don't actually have a selection highlight).
The regressions were likely caused when block and comment selection was moved to be fully synced based on active focus.
Note that there are also changes for the selection block path. It was noticed when trying to fix google/blockly-keyboard-experimentation#521 that since a selection highlight is a complete copy of a block's path it was also copying the ID and CSS state for that block. It just so happened that the block was actually receiving passive focus at the time of re-render that generated the selection highlight and thus that state took precedence in the new path object. This was likely due to rendering happening while moving the block over to the drag layer (and thus during its brief window of having lost focus). The fix is to simply ensure that all focus-related attributes are properly removed from the cloned object.
There is one other location where a block's path is cloned:
block_animations.tsfor disposing of the block. I suspect this won't actually require the additional attribute removal being done for the selection highlight, so this case hasn't been fixed. My local testing of block deletion seems to suggest it looks correct, but there could well be edge cases that I haven't considered.Test Coverage
This has been manually verified in Core's simple playground and in the keyboard navigation plugin's test environment.
It would be helpful to add a new test case for the underlying problem (i.e. ensuring that the block holds focus mid-drag) as part of resolving #8915.
Documentation
No new documentation should need to be added.
Additional Information
This was found during the development of google/blockly-keyboard-experimentation#511.