fix: right click menu issues #331
Conversation
PR RaspberryPiFoundation#326 introduced an issue with stale current node when showing the context menu by mouse. It's only possible to fix this by reinstating the updating of curNode from the selection. We also attempt to avoid showing passive focus when using the context menu and widgets (via mouse and keyboard) for consistency with the normal Blockly experience. Fixes an issue clicking on shadow blocks which could previously show an outline around the shadow and parent block.
microbit-matt-hillsdon
requested review from
BenHenning
and removed request for
a team
…erimentation into fix-right-click-2
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Thanks @microbit-matt-hillsdon! I think the fixes make sense. I'm happy to approve the PR, though I did have a few comments on clarity. Feel free to self-resolve and merge, or assign me back if you'd like me to take another review pass.
Due to an unintentially commented out line, clicking the workspace wouldn't clear the current node so e.g. delete would act on a block that didn't have a visual selection. Re-enabling that required revisiting shadow blocks to prevent getCurNode unexpectedly clearing based on a null selection. We now always set/use the selection for shadow blocks and take care not to move the cursor unexpectedly as it can be in two positions for a shadow block selection.
src/line_cursor.ts
Outdated
| curNode?.getType() === Blockly.ASTNode.types.BLOCK | ||
| ) { | ||
| // Selection says our curNode is not selected anymore. | ||
| // this.setCurNode(null as never, true); |
There was a problem hiding this comment.
Oops, this is definitely a problem. Re-enabling it threw up a shadow block issue addressed in 1233f57
There was a problem hiding this comment.
I actually missed this commented out bit the first time around--great catch and thanks for fixing it in the follow-up!
|
Thanks for the review @BenHenning. Assigning back as it would be good to take another look (and I can't merge it in any case). |
BenHenning
left a comment
BenHenning
left a comment
There was a problem hiding this comment.
Thanks @microbit-matt-hillsdon. Erm, good point on the merging bit. This looks good to me!
src/line_cursor.ts
Outdated
| curNode?.getType() === Blockly.ASTNode.types.BLOCK | ||
| ) { | ||
| // Selection says our curNode is not selected anymore. | ||
| // this.setCurNode(null as never, true); |
There was a problem hiding this comment.
I actually missed this commented out bit the first time around--great catch and thanks for fixing it in the follow-up!
3 participants
PR #326 introduced an issue with stale current node when showing the context menu by mouse. It's only possible to fix this by reinstating the updating of curNode from the selection in
getCurNode.We also attempt to avoid showing passive focus when using the context menu and widgets (via mouse and keyboard) for consistency with the normal Blockly experience.
You can now no longer get this scenario:
Fixes an issue clicking on shadow blocks which could previously show an outline around the shadow and parent block. You can now no longer get these scenarios:
Demo: https://fix-right-click-2.blockly-keyboard-experimentation.pages.dev/