Help needed to implement IFocusableTree for multiselect plugin

[Songlin Jiang]

Hi there,

I'm reaching out to get advice from the community on best practices for integrating multiselect plugin support in Blockly v12. With the introduction of IFocusableTree, the multiselect plugin isn’t working now. We're considering implementing IFocusableTree on top of MultiselectDraggable, but we're unsure about the best approach to do this on our side. So now I'm checking with you first to see if there is any possibility of handling this directly within the Blockly core to streamline the implementation? Any support or guidance you can provide would be greatly appreciated.

Best regards,

Songlin

[feni...@google.com]

Hi Songlin,

Thanks for getting touch about this!

I think you're correct that implementing `IFocusableTree` would resolve this. I suggest starting with a close read of the interfaces for `IFocusableTree` and `IFocusableNode`, and posting any questions here for the team to follow up on. 

>So now I'm checking with you first to see if there is any possibility of handling this directly within the Blockly core to streamline the implementation?

Long term, my goal is to get multiselection as a core feature--that's actually why we didn't completely combine the concepts of "focus" and "selection". 

We don't have anyone assigned to work on multiselection in Q3, but can support you as you work on it. If you find missing APIs or incompatibilities as you work on this, please do let us know so we can make changes in core to support this feature.

Thanks,

Rachel

[Songlin Jiang]

Hi Rachel,

Thanks for your response!

Finally got some free time to further look into this today. The PR for workspace-multiselect plugin to update to Blockly v12 is at: https://github.com/mit-cml/workspace-multiselect/pull/106. However, some changes are necessary to get Blockly working properly with the plugin, so I submitted a PR as well to Blockly: https://github.com/google/blockly/pull/9261.

The fundamental issue here is that Blockly v12 is buggy at this stage to properly support a custom and dummy IDraggable (MultiselectDraggable for the plugin's case), as Blockly constantly wants to change the focus to the actual blockSvg via hardcoded getFocusManager().focusNode calls that are scattered all around the codebase. Setting MultiselectDraggable directly via Blockly.common.setSelected doesn't work as well, and we will need to call Blockly.getFocusManager().updateFocusedNode directly.

I'm sure the PR for changes to Blockly will break some native Blockly features here and there. Feel free to modify on top of this PR directly, and I'll help test it out to see if it still works with the plugin. Right now, the plugin can only work when you multiselect and drag the blocks. Many other features, such as multi-manipulation using context menu, as well as selection highlighting in some cases, are still broken because of the unintended focus change, but I guess it might be a good idea to discuss with you first before I implement these hard-coded fixes, in case you come up with a better focus management system to properly resolve this. If you have any suggestions, feel free to comment under that PR. Thanks!

Best regards,

Songlin

[Christopher Allen]

Hi Songlin,

I've only had a quick look at your PR (an not at all to the changes you've made to the multiselect plugin), and most likely Rachel and/or my colleague Ben will be better placed to comment definitively on FocusManager-related topics, but I did have a few observations which might be useful.

On the topic of Blockly support for selectability:

What we have done in v12 is replace the ability to select one ISelectable with the ability to focus one IFocusableNode, and then (so as to not completely break the old selection API) reimplement the selection API in terms of focus—essentially unifying the two temporarily.

In the long term we hope to implement a new selection API that will allow one or more ISelectables to be selected.  It may be that the selection will be entirely independent of focus, but most probably it will usually be the case that focus would remain on one of the selected item(s), and most most actions that previously operated on the (single) selected node, and which currently operate on the (single) focused node, will instead operate on the (one or more) selected item(s).

The fundamental issue here is that Blockly v12 is buggy at this stage to properly support a custom and dummy IDraggable (MultiselectDraggable for the plugin's case), as Blockly constantly wants to change the focus to the actual blockSvg via hardcoded getFocusManager().focusNode calls that are scattered all around the codebase.

It's possible that some of these "hardcoded getFocusManager().focusNode calls" might be a little overzealous—for instance, I am surprised to see that BlockSvg.prototype.setParent calls .focusNode()—but overall I would expect that a MultiselectDraggable that correctly implements IFocusableNode should be compatible with the existing focus implementation.

 

Setting MultiselectDraggable directly via Blockly.common.setSelected doesn't work as well, and we will need to call Blockly.getFocusManager().updateFocusedNode directly.

Can you explain a bit more about why calling setSelected does not work?

 

I'm sure the PR for changes to Blockly will break some native Blockly features here and there. Feel free to modify on top of this PR directly, and I'll help test it out to see if it still works with the plugin. Right now, the plugin can only work when you multiselect and drag the blocks. Many other features, such as multi-manipulation using context menu, as well as selection highlighting in some cases, are still broken because of the unintended focus change, but I guess it might be a good idea to discuss with you first before I implement these hard-coded fixes, in case you come up with a better focus management system to properly resolve this.

Broadly speaking I do not expect we will be making any major changes to the focus management system (i.e., FocusManager and associated IFocusable* interfaces) in the near future.  It's possible that there is some cleanup to be done about when and where we make explicit focus changes—but on both counts my colleague Ben Henning will be better placed to comment, and I have asked that he take a look at this thread and your changes when he has a chance to.

Best wishes,

Christopher

[Ben Henning]

Hi Songlin,

Thanks for starting this work. My reply is going to be twofold: how I think we should approach this for the long-term, and how I think we should approach this for the short-term.

Long-term considerations

I've admittedly only thought tangentially about multiselect, and I'm not familiar with how the plugin actually works. I actually want us to step back and consider what multiselect means in the world of v12+ Blockly where we need to consider accessibility implications of selection. For some background context, FocusManager exists to ensure that we synchronize DOM focus with what the user expects is 'focused' (or being currently interacted with) since screen readers rely on the current DOM focus state for determining what to read out (where ARIA and other factors determine the actual text read out). We found during development that this carefully managed state actually simplifies a bunch of other things, including the keyboard navigation cursor and selection. In fact, it simplified selection so much that we completely removed the internal tracking for it and just instead defer to FocusManager (selection now means whatever is currently focused if it's selectable). That was sufficient for single selection, but I had assumed this wouldn't be fully compatible with multiselect.

Thinking about how things should work from a user perspective, there are at least two different ways this could be approached:

Approach 1

Using a spreadsheet metaphor: only one cell in a spreadsheet can ever be typed into which means only one cell can have focus. Multiple cells can be selected, but always exactly one will hold current focus. This makes sense from a modeling perspective, but it may be confusing from a user perspective: it could lead to situations where a user uses ctrl+enter to open a context menu in multiselect and perform an operation only on whichever block current holds focus. This also makes keyboard block movement confusing: is it on the multiselect or on an individual block?

That parallel here probably applies, but there are (at least) three distinct challenges that need to be solved to make it work:

• How do we visually distinguish selection from focus? Today, they are the same. We use the selection highlight to indicate what we deem 'active' focus (that is, the element currently interactive to the user) largely because selection highlighting already existed. We intend for selection and active highlighting to be individually customized, but that's not quite possible yet today. That being said, we don't know how we would handle the multiselect case where it becomes clear that there needs to be a visual distinction.
• How do we actually model the domain logic to make this work?
• Similar to the visual element, how does screen reader output work for multiselect? Screen reader work is very early right now in Blockly, but the entirety of the focus system was designed with it in mind, so the question definitely needs to be asked.

Approach 2

Considering cases of selection more than 1 thing to be a case where the 'selection' itself has 'focus' (e.g. as a transient construct). This could simplify a number of things:

• This makes it easier to ensure consistency between keyboard and mouse usage.
• We no longer have to visually distinguish between active focus and multiselect: just make multiselect the thing with focus. We can specialize the styling to make it more obvious that multiple elements hold 'focus'.
• It simplifies screen reader support: the multiselection would be an element that could hold DOM focus and thus have its own ARIA labels/context set up to provide an auditory context for the group of selectables.
• The domain logic may be closer to what's implemented in the plugin today (though I only looked briefly), and will probably be more interoperative with v12+ Blockly. We would need to adjust the focusNode logic in places to instead focus on the multiselection if present rather than a single node. This might require some finnickiness in ways I don't fully know the implications of: we would need to support representing a contextual object in dragging and other operations rather than a singular focusable node. This may make multiselect in plugin form prohibitively difficult vs. adding it to core (which is something we've wanted to do at some point, regardless).

Conclusion

My suggestion: We should centralize on what the user experience should be from mouse, keyboard, and screen reader perspectives. After that we can determine what the ideal solution is within core Blockly, and then consider how we might support that implementation in the plugin. I suggest this approach because I can't be sure the current way the multiselect plugin works will even be compatible with the focus system or screen reader support--given how much of core Blockly doesn't "just work" for these use cases, I'm very much expecting multiselect to be in a similar situation. Also not discussed here but equally important: approaches 1 and 2 above will have implications for keyboard navigation since we will need to consider how multiselect operates both for initiating/changing a multiselect and for operating on a multiselect (which requires some level of dynamic detection for navigation).

Short-term considerations

The biggest unknown to me is how much we will be able to make this work via a plugin, or if we're in a situation where multiselect can only reasonably be made to work with core Blockly changes (which would mean possibly targeting the v13 release next year). I really hope we can at least introduce a stopgap for v12 so that we're not breaking multiselect for all v12 users--that is not what we want. It may be the case that the idea here of using a FocusableNode to represent the multiselection may actually work correctly, and could provide an actual basis for approach 2 above. The main challenge that I expect to happen here is around all the focusNode() calls which are very much assuming a block (in many cases) is the thing to focus.

I will need to look into the multiselect plugin in more detail to provide more concrete recommendations and to properly think about your existing changes, but I might not have time to do that for a few weeks.

Regards,

Ben

--
You received this message because you are subscribed to the Google Groups "Blockly" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blockly+u...@googlegroups.com.
To view this discussion visit https://groups.google.com/d/msgid/blockly/CAN0w5ebJmns6vaCuyYAdpAiioVxX4GCk-eoJEzr8qT%2B4xkxu5g%40mail.gmail.com.

[Songlin Jiang]

Thank you for your reply, Christopher and Ben! I now understand more about the design of the focus management system. Here is a more detailed and high-level explanation for the conflict between the focus management system and the multiselect plugin:

Currently, the workspace-multiselect plugin acts as an adapter. It maintains its own multiple selection set (state), which keeps track of currently selected blocks. At the Blockly side, this is implemented as if there's a global `MultiselectDraggable` (which implements IDraggable) for each workspace, and current design for the plugin is that this instance of `MultiselectDraggable` should always be selected (in a dummy way as it won't create any new DOM object) when we have multiple blocks selected, so that the plugin can receive corresponding actions and pass all the actions to the blocks in the multiple selection set (state). It was working well in previous versions, but now in v12, the current code for `getFocusManager().focusNode` seems to break this completely, as it keeps unselecting `MultiselectDraggable`.

I'm not sure if there's a better way so that we can pass users' actions to the plugin, maybe somehow something can be implemented on Blockly/multiselect plugin side, so that the focus management system keeps `MultiselectDraggable` selected when users select multiple nodes, but I'm not completely sure how to properly implement this. Feel free to let me know your thoughts.

Best regards,

Songlin

[Christopher Allen]

Hi Songlin,

Currently, the workspace-multiselect plugin acts as an adapter. It maintains its own multiple selection set (state), which keeps track of currently selected blocks. At the Blockly side, this is implemented as if there's a global `MultiselectDraggable` (which implements IDraggable) for each workspace, and current design for the plugin is that this instance of `MultiselectDraggable` should always be selected (in a dummy way as it won't create any new DOM object) when we have multiple blocks selected, so that the plugin can receive corresponding actions and pass all the actions to the blocks in the multiple selection set (state).

That makes sense—and sounds quite a lot like Ben's proposed approach #2, at least in principle.

It was working well in previous versions, but now in v12, the current code for `getFocusManager().focusNode` seems to break this completely, as it keeps unselecting `MultiselectDraggable`.

I'm not sure if there's a better way so that we can pass users' actions to the plugin, maybe somehow something can be implemented on Blockly/multiselect plugin side, so that the focus management system keeps `MultiselectDraggable` selected when users select multiple nodes, but I'm not completely sure how to properly implement this. Feel free to let me know your thoughts.

If MultiselectDraggable implements IFocusableNode—which would mean it would need to have at least one DOM element that could hold browser focus—then there's no immediately obvious reason why the focus manager should not allow it to remain selected.  I'm not even sure it's necessary that the DOM element be visible, but it could be both visible and useful: e.g. a rectangular outline around all the selected items, as is seen in other 2D editors like Google Slides.

Best wishes,

Christopher

[Songlin Jiang]

Hi Christopher,

Thanks for replying!

> If MultiselectDraggable implements IFocusableNode—which would mean it would need to have at least one DOM element that could hold browser focus—then there's no immediately obvious reason why the focus manager should not allow it to remain selected.  I'm not even sure it's necessary that the DOM element be visible, but it could be both visible and useful: e.g. a rectangular outline around all the selected items, as is seen in other 2D editors like Google Slides.

I'm not sure if this is enough to hold the focus under the current v12 design, but at least some changes at the Blockly side would still be needed, e.g., we shouldn't call `getFocusManager().focusNode` to make it focus on the actual block during dragging: https://github.com/google/blockly/pull/9261/files#diff-c39a1ccc62b5bfd52e145668560e0a34b10d6eb4ce4de60623832f2852885ad2R106-R113 I'll investigate this more if I get any free time.

Best regards,

Songlin

[Ben Henning]

I think if we want to solve this properly in v12, we still need to consider how to represent selected state (which is largely the problem, I think). I strongly suspect that approach (1) that I mentioned above won't be possible without breaking changes, but approach (2) may be.

I like Christopher's suggestion to try augmenting the multiselect plugin to introduce an actual DOM element (like a highlight) on which to focus. To add one important thing: this element does need to be visible. In order for something to be focusable in the DOM it must both be rendered and have a tabindex defined.

As for fixing the focusNode problem, there are maybe two ways of approaching it:

1. Not recommended right now: we could consider designing a new system (perhaps a SelectionManager?) that can properly interface between FocusManager and the existing selection API (to avoid breaking v12 compatibility). This needs to be considered very carefully, however, since we would want to both implement it with backward and forward compatibility in mind (i.e. it would be a good opportunity to establish the baseline for multiselect in core Blockly in the future). I suspect this will be easier to do after solving (2) first, so it's not recommended.
2. Recommended: test and figure out the different focusNode() cases that are causing problems. For each one, we probably need to check if the thing prior to the operation that held focus was selectable and, if it was, restore focus to the selection rather than changing focus to a new node. This, combined with a custom IFocusableNode implementation for the selection itself, should largely work.

An important caveat to the suggested approach: the multiselect node will not be correctly hooked into the WorkspaceSvg focusable tree. This can have some ramifications, but I expect it won't actually introduce any issues (it mainly can cause desyncing problems between FocusManager and DOM focus, but multiselect shouldn't allow for direct DOM focus changing since the highlight itself should never be clickable or tab navigable).

Happy to provide more specific guidance as needed if this is the approach you end up taking, Songlin.

Best,

Ben

--
You received this message because you are subscribed to the Google Groups "Blockly" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blockly+u...@googlegroups.com.

To view this discussion visit https://groups.google.com/d/msgid/blockly/3f9a4c3b-1be1-45c2-b63a-d3527b8666e5n%40googlegroups.com.