fix: remove marked node field #273
Conversation
Instead hide/show cursor/passive focus indicator on the relevant focus changes. Fixes RaspberryPiFoundation#263
microbit-matt-hillsdon
requested review from
cpcallen
and removed request for
a team
|
Note to self: if we merge this then the same logic needs to be run in the external toolbox scenario. |
cpcallen
left a comment
cpcallen
left a comment
There was a problem hiding this comment.
Code looks fine (though note comments below). Would like to verify that this works in practice before merging, though, as @rachel-fenichel had some concerns.
| const cursor = workspace.getCursor(); | ||
| if (cursor) { | ||
| this.passiveFocusIndicator.show(cursor.getCurNode()); | ||
| cursor.hide(); |
There was a problem hiding this comment.
It should be noted that (contrary to expectations) Marker.prototype.hide does not cause the cursor to be persistently hidden, but only hides the marker SVG, and only until the next time a render occurs.
Marker SVG is used for denoting position of cursor when it is on a connection or on the workspace (or on a whole-stack node, though these are only currently accessible via the context-out shortcut which will probably go away very soon).
So AFAICT this won't hide the cursor if it's on a block, and if it isn't on a block then it will be hidden but may reappear unexpectedly if something causes a render to occur.
Probably not a big deal, given that the existing code does exactly the same thing, though I do wonder if it might be better to hide the cursor before showing the passive focus indicator (here and in focusFlyout).
| this.passiveFocusIndicator.show(cursor.getCurNode()); | ||
| cursor.hide(); |
There was a problem hiding this comment.
Does it still work if you call cursor.hide() before passiveFocusIndicator.show? It would seem more tidy to do it in that order.
|
OK, I played with this for a while and couldn't find any situations in which the cursor was not in the expected place. LGTM. |
|
Closing in favour of discussion around #326 |
3 participants
Instead hide/show cursor/passive focus indicator on the relevant focus changes.
Fixes #263
I totally might be missing a reason why we can't do this. But if we can it seems like a clear win.
Demo: https://simplify-marked-node.blockly-keyboard-experimentation.pages.dev/