Skip to content

feat: active/passive focus styling #511

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 15, 2025
Merged

feat: active/passive focus styling #511

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6e78753
feat: active/passive focus styling
microbit-matt-hillsdon May 12, 2025
77f7549
fix: ensure valid rect width/height
microbit-matt-hillsdon May 15, 2025
d94d55f
fix: whitespace
microbit-matt-hillsdon May 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import * as Blockly from 'blockly/core';
import {NavigationController} from './navigation_controller';
import {enableBlocksOnDrag} from './disabled_blocks';
import {InputModeTracker} from './input_mode_tracker';

/** Plugin for keyboard navigation. */
export class KeyboardNavigation {
Expand All @@ -25,6 +26,27 @@ export class KeyboardNavigation {
*/
private originalTheme: Blockly.Theme;

/**
Copy link
Collaborator

@rachel-fenichel rachel-fenichel May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@BenHenning BenHenning May 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

@microbit-matt-hillsdon microbit-matt-hillsdon May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@microbit-matt-hillsdon microbit-matt-hillsdon May 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@BenHenning BenHenning May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 RaspberryPiFoundation/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).

* Input mode tracking.
*/
private inputModeTracker: InputModeTracker;

/**
* Focus ring in the workspace.
*/
private workspaceFocusRing: Element | null = null;
/**
* Selection ring inside the workspace.
*/
private workspaceSelectionRing: Element | null = null;

/**
* Used to restore monkey patch.
*/
private oldWorkspaceResize:
| InstanceType<typeof Blockly.WorkspaceSvg>['resize']
| null = null;

/**
* Constructs the keyboard navigation.
*
Expand All @@ -37,6 +59,7 @@ export class KeyboardNavigation {
this.navigationController.init();
this.navigationController.addWorkspace(workspace);
this.navigationController.enable(workspace);
this.inputModeTracker = new InputModeTracker(workspace);

this.originalTheme = workspace.getTheme();
this.setGlowTheme();
Expand All @@ -57,18 +80,62 @@ export class KeyboardNavigation {
workspace.getParentSvg(),
);
}

this.oldWorkspaceResize = workspace.resize;
workspace.resize = () => {
this.oldWorkspaceResize?.call(this.workspace);
this.resizeWorkspaceRings();
};
this.workspaceSelectionRing = Blockly.utils.dom.createSvgElement('rect', {
fill: 'none',
class: 'blocklyWorkspaceSelectionRing',
});
workspace.getSvgGroup().appendChild(this.workspaceSelectionRing);
this.workspaceFocusRing = Blockly.utils.dom.createSvgElement('rect', {
fill: 'none',
class: 'blocklyWorkspaceFocusRing',
});
workspace.getSvgGroup().appendChild(this.workspaceFocusRing);
this.resizeWorkspaceRings();
}

private resizeWorkspaceRings() {
if (!this.workspaceFocusRing || !this.workspaceSelectionRing) return;
this.resizeFocusRingInternal(this.workspaceSelectionRing, 5);
this.resizeFocusRingInternal(this.workspaceFocusRing, 0);
}

private resizeFocusRingInternal(ring: Element, inset: number) {
const metrics = this.workspace.getMetrics();
ring.setAttribute('x', (metrics.absoluteLeft + inset).toString());
ring.setAttribute('y', (metrics.absoluteTop + inset).toString());
ring.setAttribute(
'width',
Math.max(0, metrics.viewWidth - inset * 2).toString(),
);
ring.setAttribute(
'height',
Math.max(0, metrics.svgHeight - inset * 2).toString(),
);
}

/**
* Disables keyboard navigation for this navigator's workspace.
*/
dispose() {
this.workspaceFocusRing?.remove();
this.workspaceSelectionRing?.remove();
if (this.oldWorkspaceResize) {
this.workspace.resize = this.oldWorkspaceResize;
}

// Remove the event listener that enables blocks on drag
this.workspace.removeChangeListener(enableBlocksOnDrag);

this.workspace.setTheme(this.originalTheme);

this.navigationController.dispose();
this.inputModeTracker.dispose();
}

/**
Expand Down
48 changes: 48 additions & 0 deletions src/input_mode_tracker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import {WorkspaceSvg} from 'blockly';

/**
* Types of user input.
*/
const enum InputMode {
Keyboard,
Pointer,
}

/**
* Tracks the most recent input mode and sets a class indicating we're in
* keyboard nav mode.
*/
export class InputModeTracker {
private lastEventMode: InputMode | null = null;

private pointerEventHandler = () => {
this.lastEventMode = InputMode.Pointer;
};
private keyboardEventHandler = () => {
this.lastEventMode = InputMode.Keyboard;
};
private focusChangeHandler = () => {
const isKeyboard = this.lastEventMode === InputMode.Keyboard;
const classList = this.workspace.getInjectionDiv().classList;
const className = 'blocklyKeyboardNavigation';
if (isKeyboard) {
classList.add(className);
} else {
classList.remove(className);
}
};

constructor(private workspace: WorkspaceSvg) {
document.addEventListener('pointerdown', this.pointerEventHandler, true);
document.addEventListener('keydown', this.keyboardEventHandler, true);
document.addEventListener('focusout', this.focusChangeHandler, true);
document.addEventListener('focusin', this.focusChangeHandler, true);
}

dispose() {
document.removeEventListener('pointerdown', this.pointerEventHandler, true);
document.removeEventListener('keydown', this.keyboardEventHandler, true);
document.removeEventListener('focusout', this.focusChangeHandler, true);
document.removeEventListener('focusin', this.focusChangeHandler, true);
}
}
109 changes: 60 additions & 49 deletions test/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,6 @@
width: 100%;
max-height: 100%;
position: relative;
--outline-width: 5px;
}

.blocklyFlyout {
top: var(--outline-width);
left: var(--outline-width);
height: calc(100% - calc(var(--outline-width) * 2));
}

.blocklyToolboxDiv ~ .blocklyFlyout:focus {
Expand Down Expand Up @@ -97,52 +90,70 @@
font-weight: bold;
}

.blocklyActiveFocus:is(
.blocklyField,
.blocklyPath,
.blocklyHighlightedConnectionPath
) {
stroke: #ffa200;
stroke-width: 3px;
}
.blocklyActiveFocus > .blocklyFlyoutBackground,
.blocklyActiveFocus > .blocklyMainBackground {
stroke: #ffa200;
stroke-width: 3px;
}
.blocklyActiveFocus:is(
.blocklyToolbox,
.blocklyToolboxCategoryContainer
) {
outline: 3px solid #ffa200;
}
.blocklyPassiveFocus:is(
.blocklyField,
.blocklyPath,
.blocklyHighlightedConnectionPath
) {
stroke: #ffa200;
stroke-dasharray: 5px 3px;
stroke-width: 3px;
html {
--blockly-active-node-color: #ffa200;
--blockly-active-tree-color: #60a5fa;
--blockly-selection-width: 3px;
}
.blocklyPassiveFocus > .blocklyFlyoutBackground,
.blocklyPassiveFocus > .blocklyMainBackground {
stroke: #ffa200;
stroke-dasharray: 5px 3px;
stroke-width: 3px;
* {
Copy link
Collaborator

@BenHenning BenHenning May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@microbit-matt-hillsdon microbit-matt-hillsdon May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

box-sizing: border-box;
}
.blocklyPassiveFocus:is(
.blocklyToolbox,
.blocklyToolboxCategoryContainer
) {
border: 3px dashed #ffa200;

/* Blocks, connections and fields. */
.blocklyKeyboardNavigation
.blocklyActiveFocus:is(.blocklyPath, .blocklyHighlightedConnectionPath),
.blocklyKeyboardNavigation
.blocklyActiveFocus.blocklyField
> .blocklyFieldRect {
Comment on lines +103 to +107
Copy link
Collaborator

@BenHenning BenHenning May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@microbit-matt-hillsdon microbit-matt-hillsdon May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@BenHenning BenHenning May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

stroke: var(--blockly-active-node-color);
stroke-width: var(--blockly-selection-width);
}
.blocklyKeyboardNavigation
.blocklyPassiveFocus:is(
.blocklyPath:not(.blocklyFlyout .blocklyPath),
.blocklyHighlightedConnectionPath
),
.blocklyKeyboardNavigation
.blocklyPassiveFocus.blocklyField
> .blocklyFieldRect {
stroke: var(--blockly-active-node-color);
stroke-dasharray: 5px 3px;
stroke-width: var(--blockly-selection-width);
}
.blocklySelected:is(.blocklyPath) {
stroke: #ffa200;
stroke-width: 5;
.blocklyKeyboardNavigation
.blocklyPassiveFocus.blocklyHighlightedConnectionPath {
/* The connection path is being unexpectedly hidden in core */
Copy link
Collaborator

@BenHenning BenHenning May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 RaspberryPiFoundation/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 RaspberryPiFoundation/blockly#8942 since CSS should make this a lot easier.

Copy link
Contributor Author

@microbit-matt-hillsdon microbit-matt-hillsdon May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@BenHenning BenHenning May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

display: unset !important;
}
.blocklySelected > .blocklyPathLight {
display: none;

/* Toolbox and flyout. */
.blocklyKeyboardNavigation .blocklyFlyout:has(.blocklyActiveFocus),
.blocklyKeyboardNavigation .blocklyToolbox:has(.blocklyActiveFocus),
.blocklyKeyboardNavigation
.blocklyActiveFocus:is(.blocklyFlyout, .blocklyToolbox) {
outline-offset: calc(var(--blockly-selection-width) * -1);
outline: var(--blockly-selection-width) solid
var(--blockly-active-tree-color);
}
/* Workspace */
.blocklyKeyboardNavigation
.blocklyWorkspace:has(.blocklyActiveFocus)
.blocklyWorkspaceFocusRing,
.blocklyKeyboardNavigation
.blocklyWorkspace.blocklyActiveFocus
.blocklyWorkspaceFocusRing {
stroke: var(--blockly-active-tree-color);
stroke-width: calc(var(--blockly-selection-width) * 2);
}
.blocklyKeyboardNavigation
.blocklyWorkspace.blocklyActiveFocus
.blocklyWorkspaceSelectionRing {
stroke: var(--blockly-active-node-color);
stroke-width: var(--blockly-selection-width);
}
.blocklyKeyboardNavigation
.blocklyToolboxCategoryContainer:focus-visible {
outline: none;
}
</style>
</head>
Expand Down
Loading