Deleting a variable throws an exception #8994
Comments
|
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:
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. |
|
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. |
## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes #8994 ### Proposed Changes This removes an error that was previously thrown by `FocusManager` when attempting to focus an invalid node (such as one that's been removed from its parent). ### Reason for Changes #8994 (comment) goes into more detail. While this error did cover legitimately wrong cases to try and focus things (and helped to catch some real problems), fixing this 'properly' may become a leaky boat problem where we have to track down every possible asynchronous scenario that could produce such a case. One class of this is ephemeral focus which had robustness improvements itself in #8981 that, by effect, caused this issue in the first place. Holistically fixing this with enforced API contracts alone isn't simple due to the nature of how these components interact. This change ensures that there's a sane default to fall back on if an invalid node is passed in. Note that `FocusManager` was designed specifically to disallow defocusing a node (since fallbacks can get messy and introduce unpredictable user experiences), and this sort of allows that now. However, this seems like a reasonable approach as it defaults to the behavior when focusing a tree explicitly which allows the tree to fallback to a more suitable default (such as the first item to select in the toolbox for that particular tree). In many cases this will default back to the tree's root node (such as the workspace root group) since sometimes the removed node is still the "last focused node" of the tree (and is considered valid for the purpose of determining a fallback; tree implementations could further specialize by checking whether that node is still valid). ### Test Coverage Some new tests were added to cover this case, but more may be useful to add as part of #8910. ### Documentation No documentation needs to be added or updated as part of this (beyond code documentation changes). ### Additional Information This original issue was found by @RoboErikG when testing #8995. I also verified this against the keyboard navigation plugin repository.
|
Fixed in #8997. |
Check for duplicates
Description
When deleting a variable, a console error is thrown.
Reproduction steps
Stack trace
Screenshots
No response
Browsers
Chrome desktop
The text was updated successfully, but these errors were encountered: