feat!: Make everything ISelectable focusable #9004
Conversation
Now everything is based on focus with selection only being used as a proxy.
Now setSelected() is only for quasi-external use.
|
This still needs to be tested & expanded into a ready-to-merge PR, but the thoughts here are:
I haven't changed cases where Edit: updated the description--this will NOT fix #9002 as it doesn't provide a transition pathway for mutators. It will allow the bubble to be focused, though. |
Needed new common-level helper.
|
This looks directionally correct to me. |
| */ | ||
| let selected: ISelectable | null = null; | ||
| export function getSelected(): ISelectable | null { | ||
| const focused = getFocusManager().getFocusedNode(); |
There was a problem hiding this comment.
I am curious about how well this will work.
One scenario is that in context menu items sometimes the callback would use getSelected, because the block is still selected when the menu is open. But the block won’t be focused when the menu is open. So we would need to call this out as a breaking change. I am curious if there are other situations where a block would have been selected but not (active) focused and I’m not sure I can think of any, maybe in some widget div scenarios
There was a problem hiding this comment.
I think the scenario you describe would be broken as of #8938 not this change, but it's equally relevant. It's definitely the case now that right clicking will not maintain block selection since the selection highlight is cleared on focus loss.
It's something worth discussing, I think. Does something really have selection if it doesn't have input focus? My assumption is no which is why the change moved in this direction. If the answer should be yes, then we need to approach the auto-selection differently.
There was a problem hiding this comment.
I think logically it's fine, but it is a big behavior change that the block isn't selected anymore while you're interacting with non-Blockly parts of the UI (e.g. context menu or dev console) so we just need to be super clear about that in our communication.
There was a problem hiding this comment.
What's the best way for me to indicate that bit (per being super clear)?
There was a problem hiding this comment.
Update the PR description of this PR (and/or any others you've made that are breaking) to include a ## Breaking Changes section that explains 1) who is broken by the change 2) what they need to do in response to the breaking change.
So for the issue about the block not being selected anymore, provide workarounds like using the (now passively-) focused block instead of selected block, using the scope for context menu items and shortcuts instead of selected, etc.
For this PR, you might need other details like what new methods need to be implemented if any for bubbles, etc.
Then remind me to call this out in the handwritten release notes we'll attach to the github release/forum announcement as well.
Conflicts: core/keyboard_nav/line_cursor.ts
This fixed then regressed a circular dependency causing the node and advanced compilation steps to fail. This investigation is ongoing.
This ensures the node and advanced compilation test steps now pass.
BenHenning
left a comment
There was a problem hiding this comment.
Self-reviewed all changes so far.
| // If either block being connected was selected, visually un- and reselect | ||
| // it. This has the effect of moving the selection path to the end of the | ||
| // list of child nodes in the DOM. Since SVG z-order is determined by node | ||
| // order in the DOM, this works around an issue where the selection outline | ||
| // path could be partially obscured by a new block inserted after it in the | ||
| // DOM. | ||
| const selection = common.getSelected(); | ||
| const selectedBlock = | ||
| (selection === parentBlock && parentBlock) || | ||
| (selection === childBlock && childBlock); | ||
| if (selectedBlock) { | ||
| selectedBlock.removeSelect(); | ||
| selectedBlock.addSelect(); | ||
| } |
There was a problem hiding this comment.
Check whatever cases this can occur to see if it still works as expected.
There was a problem hiding this comment.
Note that this testing will include verifying #9004 (comment) as well to keep things in one location for RenderedConnection.
There was a problem hiding this comment.
Chatting with Rachel about this a bit to try and figure out repro cases, and why this logic is even needed.
For connections, it's needed specifically in Zelos due to selections being a separate path object (where SVG rendering is always back-to-front based on the DOM element order). An easy repro is to open the advanced playground with Zelos enabled, add and connect 2 print blocks, then drag the bottom block to the top of the stack and observe whether the selection highlight gets cut off on the bottom of the block.
I can confirm that this works correctly in this branch even without these manual changes. I will confirm more specifically against develop, but I suspect why this is no longer needed is due to behavior changes in #8981 in how often a block is brought to front. I'm not 100% certain of that yet as nothing in the PR is obviously the reason why this would be different.
For bumping, my repro was a bit trickier to figure out. Here's what I did:
- Open advanced playground (also with Zelos since that's the only renderer that should be affected by these selection changes).
- Add a "do something" with return block.
- Add an empty text block as the return.
- Add a "create text with" block.
- Try to drag the "do something" block over the "create text with" block such that the mutator icon is underneath the empty space for the text block (i.e. the white square). This is a nice reference to use since otherwise getting the block underneath to bump can be a bit finicky position-wise.
This will generally trigger a small bump of the "create text with" block.
However, this doesn't trigger the interesting case because the block underneath being bumped is the 'inferior' one and thus not selected. Still trying to find a repro with selection.
There was a problem hiding this comment.
Ah, another suggestion from Rachel: this can be triggered by forcing the 'create text with' block to be immovable by using:
Blockly.getMainWorkspace().getBlockById(id).setMovable(false)In the developer console with the 'create text' block's ID being used in place of 'id' above (using the advanced playground JSON output to find it).
This...unfortunately still seems to not demonstrate the problem. On develop with the selection code removed and this scenario used, the outer block (being dragged) is retaining its selection order without a problem (which makes sense because it's being clicked on and thus being brought to front for the drag).
There was a problem hiding this comment.
I've dug back to 02b2c21 as the original introducing of the selection correction logic. I'm not sure what the original bug's repro steps were, but this fix is very old and predates gestures plus bring-to-front logic (at least in its current form). I'm now suspecting that the selection bits for bumping have been made obsolete with other bring-to-front mechanisms such that this hasn't been needed for some time. No amount of bumping seems to get in the way of the selected block or the selection itself (both always seem to stay above the bumped block), so I do think it's safe to remove this logic now.
There was a problem hiding this comment.
Going back to connect_, I think I know what's happening here and verifying would be tricky, but I think this fine to logic out based on the observation that everything is working fine without the selection bits in.
With the latest changes, the data flow roughly looks like this:
- User sends pointer event to click on a block.
- Gestures mark that block as focused.
- The block, upon receiving focus, auto select themselves (and if a previous block was focused, it removes its selection).
- The process of removing/adding a selection bumps it back to the top in the same way as the old logic here did.
Thus, this shouldn't be needed since the various efforts to focus the block being moved should generally always result with its selection appearing on top.
Addresses reviewer comment.
This addresses a bunch of review comments, and fixes selecting workspace comments.
BenHenning
left a comment
There was a problem hiding this comment.
Self-reviewed latest changes and working to close open conversation threads that have had their trivial fixes addressed.
|
This is a comment documenting all of the testing things I did (or, rather, linking to where I describe them):
|
| * later on when defocused (since it was previously considered focusable at | ||
| * the time of being focused). | ||
| */ | ||
| canBeFocused(): boolean; |
There was a problem hiding this comment.
I think I would probably remove "disabled, read-only" from the list of examples for why you might want this to return false, as those cases should still be focusable for screenreader users most likely. I think it really is just the purely visual decoration / no visual representation case.
4074cee
into
RaspberryPiFoundation:rc/v12.0.0
## The basics - [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change) ## The details ### Resolves Fixes RaspberryPiFoundation/blockly-keyboard-experimentation#499 ### Proposed Changes This ensures that non-blocks which hold active focus correctly update `LineCursor`'s internal state. ### Reason for Changes This is outright a correction in how `LineCursor` has worked up until now, and is now possible after several recent changes (most notably #9004). #9004 updated selection to be more explicitly generic (and based on `IFocusableNode`) which means `LineCursor` should also properly support more than just blocks when synchronizing with focus (in place of selection), particularly since lots of non-block things can be focusable. What's interesting is that this change isn't strictly necessary, even if it is a reasonable correction and improvement in the robustness of `LineCursor`. Essentially everywhere navigation is handled results in a call to `setCurNode` which correctly sets the cursor's internal state (with no specific correction from focus since only blocks were checked and we already ensure that selecting a block correctly focuses it). ### Test Coverage It would be nice to add test coverage specifically for the cursor cases, but it seems largely unnecessary since: 1. The main failure cases are test-specific (as mentioned above). 2. These flows are better left tested as part of broader accessibility testing (per #8915). This has been tested with a cursory playthrough of some basic scenarios (block movement, insertion, deletion, copy & paste, context menus, and interacting with fields). ### Documentation No new documentation should be needed here. ### Additional Information This is expected to only affect keyboard navigation plugin behaviors, particularly plugin tests. It may be worth updating `LineCursor` to completely reflect current focus state rather than holding an internal variable. This, in turn, may end up simplifying solving issues like #8793 (but not necessarily).
This appears to have inadvertently been omitted in PR RaspberryPiFoundation#9004.
This appears to have inadvertently been omitted in PR RaspberryPiFoundation#9004.
* refactor(interfaces): Use typeof ... === 'function' to test for methods
Testing for
'name' in object
or
obj.name !== undefined
only checks for the existence of the property (and in the latter
case that the property is not set to undefined). That's fine if
the interface specifies a property of indeterminate type, but in
the usual case that the interface member is a method we can do
one better and check to make sure the property's value is
callable.
* refactor(interfaces): Always check obj is not null/undefined
Since most type predicates take an argument of type any but then
check for the existence of certain properties, explicitly check
that the argument is not null or undefined (or check implicitly
by calling another type predicate that does so first, which
necessitates adding a few casts because tsc infers the type of
the argument too narrowly).
* fix(interfaces): Add missing check to hasBubble type predicate
This appears to have inadvertently been omitted in PR #9004.
* fix(interfaces): Fix misplaced typeof
* fix: Fix typos in JSDocs
* fix(tests): Make Mocks conform to corresponding interfaces
Introduce a new MockFocusable, and add methods to MockIcon,
MockBubbleIcon and MockComment, so that they fulfil the
IFocusableNode, IIcon, IHasBubble and ICommentIcon interfaces
respectively.
* chore(tests): Add assertions verifying mocks conform to predicates
Add (test) runtime assertions that:
- isFocusableNode(MockFocusable) returns true
- isIcon(MockIcon) returns true
- hasBubble(MockBubbleIcon) returns true
- isCommentIcon(MockCommentIcon) returns true
(The latter is currently failing because Blockly is undefined when
isCommentIcon calls the MockCommentIcon's getType method.)
* fix(tests): Don't rely on Blockly being set in Mock methods
For some reason the global Blockly binding is not visible at the
time when isCommentIcon calls MockCommentIcon's getType method,
and presumably this problem would apply to getBubbleSize too,
so directly import the required items.
* refactor(tests): Make MockCommentIcon a MockBubbleIcon
This slightly simplifies it and makes it less likely to accidentally
stop conforming to IHasBubble.
* fix(interfaces): Fix incorrect check in isSelectable
Fix an error which caused ISelectable instances to fail
isSelectable() checks, one of the results of which is that
Blockly.common.getSelected() would generally return null.
Whoops!
The basics
The details
Resolves
Fixes #8820
Fixes #9000
Fixes #9001
Fixes #9003
Fixes #8998
Fixes part of #8771
Proposed Changes
At a high level, this PR makes everything that's selectable (that is, implements
ISelectable) now focusable by makingISelectableextendIFocusableNode. The brings with it a bunch of new possibilities, also introduced in this PR:commonto be completely derived from focus (since we now guarantee that selection auto-updates forISelectables when they are focused).IIconandIBubbleextensions ofIFocusableNode. Both imply having visual elements which is why these root interfaces have been updated (unlike in other cases likeBlockandConnection).common.setSelected()calls to now usegetFocusManager().focusNode(). Since selection is represented by focus, focusing with auto-selection is logically equivalent to the oldcommon.setSelected()call (except for clearing a selection state).LineCursorno longer needs to fall back to selection since it's completely covered by focus cases.WorkspaceSvghas been done somewhat generically to provide a reasonable pathway for custom implementations to properly hook into workspace focus.Reason for Changes
Fundamentally, these changes are needed to simplify selection logic as well as add missing components to the focus system (which will then allow them to become navigable and fully functional for keyboard navigation).
Some key design decisions that were made:
ISelectableandcommon.setSelected()/common.getSelected()have been kept even though they are trivially removable currently. This is because we still want to maintain selectable state and an API to represent it for when multi selection can be designed into core Blockly.common.setSelected()is still marked as@internal.canBeFocusedmethod has been added toIFocusableNode.INavigable.isNavigable().SELECTEDevent firing logic has been pulled into a newcommonmethod. It's now called directly byISelectableimplementations and, being that it's@internal, shouldn't be used outside of core Blockly. There may well be a better location for this to live in the long-term, but that's a design decision better settled as part of multi-select.BlockSvgselection is now directly attached to shadow blocks rather than being forced to delegate to the block's parent. Instead, this is being checked at the places where it could matter (namely for gestures).BlockSvg's dispose logic no longer needs to care about selections since they're tied to focus, and focus should automatically adjust when a block is removed from the workspace.Bubblehas a bit of tricky changes to its constructor as a result of needs fromTextInputBubble. Specifically, for reasons I am not 100% sure of it seems thatTextInputBubble'stextareamust be focused in order to receive input (this is not the case forRenderedWorkspaceCommentdespite very similar implementations). Thus,Bubbleprovides a way to customize the focusable element that it uses (but otherwise defaults to its root group).bringToFront()logic was simplified since it can be tied to focus being received, now.TextInputBubbleis no longer necessary.FocusManagerprovides fairly strong guarantees for restoring focus for certain DOM operations (though this can get complicated as is seen forBlockSvg.bringToFront()).IHasBubblehas been updated to require being able to return the current bubble (which is used byWorkspaceSvgto find a focusable bubble in response to DOM changes).RenderedWorkspaceCommentnow being focusable (and thus unfocusable to return focus to the workspace itself for gesture management). Note that there was one change inLayerManagerto avoid unnecessary appends (which can cause a cycle inFocusManagerfor focused elements when the operation is called inonFocusNode). Some effort could be made to break the cycle inFocusManageritself (which is tricky), but this approach has a net benefit to avoid an operation that simply isn't needed.WorkspaceDraggerhas had its selection/focus logic on drag start removed due to it being unnecessary. See: feat!: Make everything ISelectable focusable #9004 (comment).Caveats:
BlockSvg'saddSelect/removeSelectare used directly. It's assumed these are still needed, but they may be incidentally obsolete if there's any part of the dataflow that forces focus (since that will then force selection state, anyway).BlockSvgwas updated to auto-select on focus) and is new for other selectables.common.getSelected()will return null for cases when there was previously a selection but not active focus (such as right clicking on a block).RenderedConnectiontechnically depends on the shadow block selection logic that was removed, however it's actually quite challenging to implement that there.RenderedConnectioncannot importBlockSvgdirectly, and fixing this import ordering could potentially be quite complex. That being said, it seems that the selection management there is actually no longer needed due to focus automatically driving selection states. See feat!: Make everything ISelectable focusable #9004 (comment) for much more details on why.WorkspaceSvgin order to ensure cross-workspace hand-off works correctly, though this might partly come down to a usability and navigation question that should be solved outside this PR.Test Coverage
Tests have not been added here, but there are legitimate cases for future testing:
FocusManagerbehaviors should be tested as part of Shore up testing coverage for FocusManager #8910.Manually testing has been detailed in this comment: #9004 (comment).
Documentation
As part of the broader focus management documentation, we will likely want to describe how to account for focus support when implementing custom icons and bubbles (though the base classes are meant to mostly solve these as-is) when wanting to maintain keyboard navigation support.
Additional Information
None currently.