Deleting a variable throws an exception

[BenHenning]

Check for duplicates

• I have searched for similar issues before opening a new one.

Description

When deleting a variable, a console error is thrown.

Reproduction steps

1. Create new variable from the toolbox.
2. Add the variable to the workspace.
3. Right click on the variable and click to delete it.
4. Open the developer console to see the stack trace thrown.

Stack trace

Uncaught Error: Attempting to focus node which isn't recognized by its parent tree: ???.
    at FocusManager.focusNode (focus_manager.ts:254:13)
    at focus_manager.ts:347:18

Screenshots

No response

Browsers

Chrome desktop

[BenHenning]

This was caused by the attempt at making ephemeral focus more robust in #8981.

I'm happy to revert that portion to fix the issue, but it actually speaks to a larger problem that both ephemeral focus and broader Blockly can sometimes run into problems where it's try to focus a node that is invalid to focus. As I see it, there are a number of issues trying to fix this:

• Remove the check and treat trying to focus an invalid node as essentially a "de-focus" situation. This would be the most robust, but is likely to run into unintended side effects of focus being on a blockly element that's no longer attached. We could try to leverage the tree's default fallback instead of leaving focus arbitrarily on that element, though this will result in those cases turning into a default thing being focused. This may be okay.
• Try to improve robustness detection. This isn't easy. I tried adding a simple "is this element still valid" check to fix this issue, and it doesn't because the element actually still is in the DOM but the node (in this case a created variable) is no longer in the block structure. I'm not sure if this is a timing thing or the DOM just isn't being cleaned up fully.

I think I actually lean toward the second solution in option (1) above, that is, fall back to a reasonable default when focus manager is given an invalid node to focus rather than throwing. I think this system is so central to Blockly now that we should try our best to recover rather than throwing (except when things are really obviously broken). I worry otherwise that we're just going to keep hitting failures with this exception in unexpected ways as we've seen versions of it come up repeatedly.

One note on that last sentence, though: it has actually helped detect a number of real logical failures that needed fixing elsewhere.

[rachel-fenichel]

I agree with falling back to a reasonable default if possible and treating it as a defocus if not possible--it matches Blockly's behaviour of trying to fail into a usable state.

[BenHenning]

Fixed in #8997.