feat: active/passive focus styling

[microbit-matt-hillsdon]

Depends on #520 so it can build against latest v12. Clean diff vs that branch.

Demo: https://passive-focus.blockly-keyboard-experimentation.pages.dev/ (built against latest v12)

Closes #507
Closes google/blockly#8966
Closes google/blockly#8964

Obviously all the CSS is still in the index.html so we should consider whether it should move for a plugin release or whether each integrator needs to copy/paste/tweak.

Key changes:

• Avoid setting stroke on field text, target the rect instead
• Hack on passive focus for connections (unexpectedly hidden in core). I still think these are going away but just in case.
• Dial down passive focus to only be used in the workspace
• Use focus outlines around focus trees to indicate active area (flyout, toolbox).
• Added rect outlines to the workspace for active area and also workspace selection
  • These need to resize like scrollbars so I've added a monkey patch for resize. Could move into core. See also relevant discussion on Shift+Tab after keyboard navigating to a block doesn’t open flyout as expected blockly#8965
• Removed the gap between the toolbox and the flyout (added long ago to make an outline visible but I don't think we're proposing everyone does this)
• Set box-sizing: border-box. We think most apps will do this and it avoids the toolbox's default padding moving it slightly off the bottom of the screen (noticeable when adding an outline).
• Remove active styling from toolbox. I think we have conflicting internal views on this one and will discuss further. If we do keep it it needs to move to using "outline" so it doesn't cause glitchy jumping around.
• Remove styles that related to selection as they don't do anything
• Add heuristic input mode tracking that avoids keyboard-nav-related focus styles when you're clicking. A simple approach seems to work surprisingly well here.

Serious outstanding issues that can feel broken or confusing:

• Move mode is quite broken with a passive and active selection showing at once
• Mouse drags show passive focus but that feels wrong when you're interacting with the passive thing. We think this behaviour should not change from current Blockly.
• Extra tab stop issue for the workspace - now discussed on Shift+Tab after keyboard navigating to a block doesn’t open flyout as expected blockly#8965

Other outstanding issues

• In Firefox/Safari it looks like more outline resets might be needed for the newly focusable blocks
• We'd prefer not to have passive focus on fields (and full block fields) when the field editor is showing. Don't show passive focus for fields #508
  • I don't know an easy way to fix this in the plugin other than tracking focus of widget/dropdown divs ourselves. Can we have a class on the field when the editor is open? There doesn't seem to be one.
• We'd prefer to have the passive focus only show when you were navigating to or within the toolbox/flyout via keyboard, where it serves a purpose as a reference point. Otherwise it doesn't justify the visual noise.
• We'd prefer the focus outlines for the areas (and passive focus in general) to only show for keyboard interaction, like focus-visible (otherwise this is a significant visual change to Blockly). This might be hard to achieve.
  • Since added input mode tracking which seems viable.
• I've made liberal use of CSS "has". If that's an issue for browser support it can likely be fixed by having the focus manger set some classes on focus trees indicating the focus within.
  • Intend to fix via new classes in Blockly as separate PR

[rachel-fenichel]

@BenHenning to take a look at the rest of "other outstanding issues with no obvious plugin-only fix" to make sure there are no required breaking changes.

[rachel-fenichel]

@BenHenning also to check whether this needs to happen in core.

[BenHenning]

@microbit-matt-hillsdon a few questions:

• Does the ring div have to be a child of the injection div or can it be a child of workspace?
• If the latter, is it possible to use styling to enable/disable the ring focus if the parent (i.e. workspace) has active focus? That would eliminate the need for the additional focus listeners (which would be preferred since we're trying to route all focus management through FocusManager).

If the above can be done, this won't require an API breaking change in Core.

If this style cannot be achieved without overlaying a div on top of the workspace g element, then this does get more complicated. Perhaps we can use a group element inside workspace with customized dimensions that subtract the toolbox + flyout so that it's just the main work area that's highlighted, and add that to the bottom of the workspace so that it always renders above everything? Still not a breaking change, I think, but perhaps a bit complicated.

If it truly needs to be a div then we can still tweak it's on/off behavior in WorkspaceSvg's own onFocusNode/onBlurNode callbacks. In fact, a quick proof-of-concept would be to swap those methods out here in place of custom focusin/focusout listeners. If that works, the change in Core for a managed div is pretty straightforward.

So long as we don't need to figure out a way to hook in this new sibling to workspace into focus, I don't expect any breaking API changes to be needed.

[microbit-matt-hillsdon]

Thanks, makes sense to try other options. Ran out of time today but will pick this up tomorrow.

[microbit-matt-hillsdon]

It cannot be achieved within the SVG, which cannot draw over the toolbox. However, recent discussions around global navigation led me to rethink whether this was desirable vs outlining the same area the workspace scrollbars are drawn in (i.e. workspace minus toolbox area). I've updated the PR to do that with a rect in the workspace SVG. That corresponds better to the global nav areas in MakeCode. Also removed the focus listeners.

[BenHenning]

I really like this latest solution. It's pretty much exactly what I was envisioning, and should be really straightforward to move into core when we get to that point.

There's a bit of a bigger open question around InputModeTracker as part of solving google/blockly#8852 but that can be worked on after this PR is merged. InputModeTracker seems quite sensible at this point and for the needs of this PR.

I also asked @maribethb to take a quick pass on the new tracker and her thoughts were:

I think the approach is fine enough but has problems, because one of the things with tracking is that doing something like copying with ctrl+c doesn't mean that a user is using keyboard navigation, so doing this with global event listeners is not going to be the right approach in the long term. but if we use the same class name, this is easy enough to update because we wouldn't have to update any styles, and then we can just use a more sophisticated way to track the mode, and delete the global event listeners

I think if these edge cases around copying and pasting (as specific examples) are fine with you @microbit-matt-hillsdon then it seems completely reasonable to merge this as-is and we can later, as Maribeth mentioned, figure out a better tracking system that interoperates with the new CSS classes (or some variation on them).

[BenHenning]

@microbit-matt-hillsdon could you provide a bit more context on the breakage you found here? Connections are hidden until they're selected. Is it the case that they're highlighting but then moving them to passive focus causes them to disappear again?

That would be a miss on my part, if so, and would make complete sense since some of the selection bits have changed yet again in google/blockly#9004.

This would need a fix in Core but I wouldn't expect it to be an API breakage. We need to maybe use something other than display to enable/disable the connection highlighting, instead. This might actually be best fixed as part of google/blockly#8942 since CSS should make this a lot easier.

[microbit-matt-hillsdon]

Prior to this change we found there was no passive focus connection path at all as it had display: none set by core. I suppose this might be about to go away due to other work but I wanted to see it and check it worked as expected if styled.

[BenHenning]

Got it. I'm not opposed to the !important bit, but we'll of course need to fix this properly when it comes time to move this all over to core. This discussion thread will probably be helpful as part of that work.

[BenHenning]

Thanks @microbit-matt-hillsdon! This is a very nice set of fixes over the rough current state of passive/active focus. I've left a few thoughts on code directly, otherwise I have in-line replies to various concerns in your PR description below:

• Remove styles that related to selection as they don't seem to do anything (please clarify)

This is probably a remnant of test changes that I had around selection that I unintentionally included in the Core plugin (but I'd have to do some code archaeology to confirm). I think removing it seems completely reasonable as we haven't yet fully moved selection over to CSS.

Serious outstanding issues that can feel broken or confusing:

• Move mode is quite broken with a passive and active selection showing at once
• Mouse drags show passive focus but that feels wrong when you're interacting with the passive thing. We think this behaviour should not change from current Blockly.

The specific behavior with selection disappearing is a regression and has been filed in google/blockly#9027.

There's a separate question about passive focus here. I think we can probably disable passive focus for blocks if they are selected, but that's supposed to be impossible. If we find a clean way to fix the selection issue while dragging then the passive focus problem should go away, too.

• Extra tab stop issue for the workspace

Just to check, this is only for shift tabbing once in the workspace, correct? If so, this will require more discussion as there may be an explicit need to be able to tab to the workspace (to open the workspace context menu with redundancy/consistency with regular keyboard navigation).

There's a small chance a proper long-term fix here would require breaking API changes. The unknown is how to let WorkspaceSvg signal to FocusManager what to do when the root is focused (which we have no control over--this is a DOM event). In some contexts we want to auto-focus a node, but in others we want to either focus the workspace or skip it. This could get complicated.

Other outstanding issues with no obvious plugin-only fix (many previously noted):

• We'd prefer not to have passive focus on fields (and full block fields) when the field editor is showing. Don't show passive focus for fields #508

So from local testing it seems removing .blocklyField for .blocklyPassiveFocus removes passive focus as expected for fields that don't have a shadow block. For shadow blocks I was able to create a more specific specifier. Specifically, here's the code I changed off the latest main branch:

https://gist.github.com/BenHenning/b6c734d7b0c2d293a56a54590d3badf8/revisions#diff-0eb547304658805aad788d320f10bf1f292797b5e6d745a3bf617584da017051'

Edit: This isn't a perfect fix as it will break the insertion flow (i.e. there's no indicator where a block will be added during insertion). I suspect this could be fixed more specifically by checking if the editor is open, but that could become rather field-specific.

I think this depends a lot on what the desired behavior is. Once insertion starts, selection takes over and you properly see where the block will be added. It's only an issue for losing context while the toolbox is open (which perhaps is something we don't actually care about, anyway).

• We'd prefer to have the passive focus only show when you were navigating to or within the toolbox/flyout via keyboard, where it serves a purpose as a reference point. Otherwise it doesn't justify the visual noise.
• We'd prefer the focus outlines for the areas (and passive focus in general) to only show for keyboard interaction, like focus-visible (otherwise this is a significant visual change to Blockly). This might be hard to achieve.

These seem like the same problems (mostly), and I see three different pathways here:

• We could add some sort of CSS class to INavigables (which we're considering making IFocusableNode, anyway) when they're navigated via keyboard navigation. This is tricky, however, since it's not as simple as adding the class to the last navigated navigable since passive focus can exist on multiple nodes simultaneously. /cc @gonfunko in case he has any thoughts on this one.
• Condition passive focus on whenever keyboard navigation is detected as enabled (a la Attempt to track if the user is using keyboard navigation blockly#8852 /cc @maribethb). If we were able to associate that state with a CSS variable then we could tweak passive focus showing accordingly.
• Do nothing. I can see the visual noise argument being a problem, but perhaps additional testing would provide a case on if it is indeed a problem or distraction that needs to be fixed.

• I've made liberal use of CSS "has". If that's an issue for browser support it can likely be fixed by having the focus manger set some classes on focus trees indicating the focus within.

I suspect that the compatibility question is that has has been around long enough that this should be fine. However if we find that this isn't the case, the suggestion to update FocusManager to add some sort of indicator class is very straightforward and a non-API breaking change (though it is an API behavioral change).

[BenHenning]

Also for bookkeeping, as far as I can tell this PR could fix all of the following:

• Don't show passive focus on flyout #507
• Don't show passive focus for fields #508
• Workspace outline is not showing when a block is selected in workspace. blockly#8966
• Flyout does not get focus outline when navigating between toolbox and flyout via left and right arrows blockly#8964 (if I understand that issue correctly)

Plus perhaps several other issues mentioned in the findings document that haven't had corresponding issues filed.

[BenHenning]

NB: The gist also fixes google/blockly#8969.

[microbit-matt-hillsdon]

• Extra tab stop issue for the workspace

Just to check, this is only for shift tabbing once in the workspace, correct? If so, this will require more discussion as there may be an explicit need to be able to tab to the workspace (to open the workspace context menu with redundancy/consistency with regular keyboard navigation).

There's a small chance a proper long-term fix here would require breaking API changes. The unknown is how to let WorkspaceSvg signal to FocusManager what to do when the root is focused (which we have no control over--this is a DOM event). In some contexts we want to auto-focus a node, but in others we want to either focus the workspace or skip it. This could get complicated.

Yes, let's move discussion to google/blockly#8965

• We'd prefer not to have passive focus on fields (and full block fields) when the field editor is showing. Don't show passive focus for fields #508

So from local testing it seems removing .blocklyField for .blocklyPassiveFocus removes passive focus as expected for fields that don't have a shadow block. For shadow blocks I was able to create a more specific specifier. Specifically, here's the code I changed off the latest main branch:

https://gist.github.com/BenHenning/b6c734d7b0c2d293a56a54590d3badf8/revisions#diff-0eb547304658805aad788d320f10bf1f292797b5e6d745a3bf617584da017051'

Edit: This isn't a perfect fix as it will break the insertion flow (i.e. there's no indicator where a block will be added during insertion). I suspect this could be fixed more specifically by checking if the editor is open, but that could become rather field-specific.

I think this depends a lot on what the desired behavior is. Once insertion starts, selection takes over and you properly see where the block will be added. It's only an issue for losing context while the toolbox is open (which perhaps is something we don't actually care about, anyway).

I think it's useful context for the insertion flow. Made a suggestion on #508 and won't apply your change here for now (I confirmed it works except for this though). I'll get other input on the value of the context though and follow up separately.

• We'd prefer to have the passive focus only show when you were navigating to or within the toolbox/flyout via keyboard, where it serves a purpose as a reference point. Otherwise it doesn't justify the visual noise.
• We'd prefer the focus outlines for the areas (and passive focus in general) to only show for keyboard interaction, like focus-visible (otherwise this is a significant visual change to Blockly). This might be hard to achieve.

These seem like the same problems (mostly), and I see three different pathways here:

• We could add some sort of CSS class to INavigables (which we're considering making IFocusableNode, anyway) when they're navigated via keyboard navigation. This is tricky, however, since it's not as simple as adding the class to the last navigated navigable since passive focus can exist on multiple nodes simultaneously. /cc @gonfunko in case he has any thoughts on this one.
• Condition passive focus on whenever keyboard navigation is detected as enabled (a la Attempt to track if the user is using keyboard navigation blockly#8852 /cc @maribethb). If we were able to associate that state with a CSS variable then we could tweak passive focus showing accordingly.
• Do nothing. I can see the visual noise argument being a problem, but perhaps additional testing would provide a case on if it is indeed a problem or distraction that needs to be fixed.

Do nothing might be OK in the short term where MakeCode plan to ship keyboard nav as a setting, but not sure it's a good plan for an on-by-default future. Other clients may have different plans, would be good to get more input. Leaning towards your second option but I think it's one of those things you have to try to decide whether it's too weird.

• I've made liberal use of CSS "has". If that's an issue for browser support it can likely be fixed by having the focus manger set some classes on focus trees indicating the focus within.

I suspect that the compatibility question is that has has been around long enough that this should be fine. However if we find that this isn't the case, the suggestion to update FocusManager to add some sort of indicator class is very straightforward and a non-API breaking change (though it is an API behavioral change).

I think we might need a fix for this but can split it out. :has is a Safari 15.4+ thing and I think you aim for Safari 14 based on a chat with @rachel-fenichel. MakeCode's Safari min version is unclear (docs say 9 but I'd bet heavily against it).

[BenHenning]

Per your last point @microbit-matt-hillsdon I will send out a PR today that adds a bunch of new classes for trees. I'm specifically thinking:

• blocklyTreeHasActiveFocus
• blocklyTreeHasPassiveFocus
• blocklySubtreeHasActiveFocus
• blocklySubtreeHasPassiveFocus

I think this gives maximum flexibility to solve some of the more complex styling edge cases.

[BenHenning]

• Move mode is quite broken with a passive and active selection showing at once
• Mouse drags show passive focus but that feels wrong when you're interacting with the passive thing. We think this behaviour should not change from current Blockly.

Both of these should now be fixed in the latest v12 per #521 (comment).

[BenHenning]

Per your last point @microbit-matt-hillsdon I will send out a PR today that adds a bunch of new classes for trees. I'm specifically thinking:

• blocklyTreeHasActiveFocus
• blocklyTreeHasPassiveFocus
• blocklySubtreeHasActiveFocus
• blocklySubtreeHasPassiveFocus

I think this gives maximum flexibility to solve some of the more complex styling edge cases.

Following up to this: I created google/blockly#9046. It doesn't cover subtree focus, and I thought of a different approach for field editing (by latching onto ephemeral focus). The ephemeral focus bit especially needs to be tested in this plugin since I'm not sure if it'll work.

Separately, this PR has a low chance of making it into v12 tomorrow since I'm not going to be comfortable merging it without thorough focus manager tests. I'll try to get it done, but it's my lowest priority PR to try and get in before the release.

[microbit-matt-hillsdon]

Rebased and updated the PR comment. A key change to highlight is I've added tracking of the most recent input method and used it to hide keyboard-oriented styles.

I think I'd call this ready for review if it wasn't for the dependency on #520. Not the last word in styling for sure but a good step forward.

[rachel-fenichel]

#520 now merged!

[BenHenning]

Thanks @microbit-matt-hillsdon! I think the solutions here are really solid, and they feel quite a bit better than current main tip-of-tree when I played around with it locally.

I left some follow-up comments, but I largely think this can be merged as-is. As noted there's still some follow-up work needed (and some design questions for when we eventually move these changes into core).

FYI that the failing tests are probably due to #525 which I'll be looking into (though note I didn't actually look at the failures on this branch closely, I'm just assuming that it's due to this).

[BenHenning]

Do we really always want to do this for everything, or should this be done specifically to toolbox and other related cases?

[microbit-matt-hillsdon]

I think it's the right thing for the demo page, but a more nuanced discussion when moving into core or even CSS applied automatically by plugin. Something like this is in every modern CSS reset going, so it's likely to be present in many environments Blockly operates in.

[BenHenning]

Suggested change

	.blocklyKeyboardNavigation
	.blocklyActiveFocus:is(.blocklyPath, .blocklyHighlightedConnectionPath),
	.blocklyKeyboardNavigation
	.blocklyActiveFocus.blocklyField
	> .blocklyFieldRect {
	.blocklyKeyboardNavigation
	.blocklyActiveFocus:is(.blocklyPath, .blocklyHighlightedConnectionPath, .blocklyField > .blocklyFieldRect) {

(Wrapping needed)

Does this work, by any chance? Maybe a small simplification.

Ditto for passive focus, as well.

[microbit-matt-hillsdon]

I don't think this works because the second selector in the original needs to select the .blocklyFieldRect which unlike the others, isn't the element with .blocklyActiveFocus but a child. Tested locally.

[BenHenning]

Ah I was hoping nesting the child selector in the is() would work, but it looks like it doesn't then. I appreciate you double checking.

[BenHenning]

Got it. I'm not opposed to the !important bit, but we'll of course need to fix this properly when it comes time to move this all over to core. This discussion thread will probably be helpful as part of that work.

[BenHenning]

I really like this latest solution. It's pretty much exactly what I was envisioning, and should be really straightforward to move into core when we get to that point.

There's a bit of a bigger open question around InputModeTracker as part of solving google/blockly#8852 but that can be worked on after this PR is merged. InputModeTracker seems quite sensible at this point and for the needs of this PR.

I also asked @maribethb to take a quick pass on the new tracker and her thoughts were:

I think the approach is fine enough but has problems, because one of the things with tracking is that doing something like copying with ctrl+c doesn't mean that a user is using keyboard navigation, so doing this with global event listeners is not going to be the right approach in the long term. but if we use the same class name, this is easy enough to update because we wouldn't have to update any styles, and then we can just use a more sophisticated way to track the mode, and delete the global event listeners

I think if these edge cases around copying and pasting (as specific examples) are fine with you @microbit-matt-hillsdon then it seems completely reasonable to merge this as-is and we can later, as Maribeth mentioned, figure out a better tracking system that interoperates with the new CSS classes (or some variation on them).

[microbit-matt-hillsdon]

Re input mode tracking, I think copy itself behaves well. It's paste and similar cases (if any?) where we programatically move focus in response to a keypress that wasn't a traditional focus move key press or an action we added that is keyboard oriented (like 't' for toolbox) rather than mixed-mode like paste. Perhaps just filtering keydown events to those that have the right intent (tab/shift-tab/t/w?) would be enough to address these cases. This likely needs action integration for the latter two. But I think this is a step forward at least and will help us enumerate the awkward cases.

[microbit-matt-hillsdon]

Thanks for the review @BenHenning. From my point of view it would be good to merge now and then tweak as needed with smaller changes.

[BenHenning]

Thanks for the review @BenHenning. From my point of view it would be good to merge now and then tweak as needed with smaller changes.

Sounds good @microbit-matt-hillsdon.

Will take care of merging this soon. Just working on fixing #525 so that I can ensure that these tests will pass once that's addressed (at which point I'm happy to merge this even with broken CI).

[BenHenning]

When merging in #527 and linking against google/blockly#9063 all tests seem to pass (there was a slight scare where I thought one was failing but I think this was just a caching fluke when changing branches & re-linking since everything passed in subsequent attempts).

Going ahead and merging this while we sort out the CI failures in #527.