From 04da877ddae1f2f6e50402b4f5e3f8c64d045628 Mon Sep 17 00:00:00 2001 From: Aaron Dodson Date: Mon, 12 May 2025 15:22:48 -0700 Subject: [PATCH 1/3] refactor: Make INavigable extend IFocusableNode. --- core/block_svg.ts | 9 ------- core/field.ts | 14 ----------- core/flyout_button.ts | 10 -------- core/flyout_separator.ts | 25 ++++++++++++++++++- core/interfaces/i_navigable.ts | 18 +++---------- core/interfaces/i_navigation_policy.ts | 14 +++++++++++ core/keyboard_nav/block_navigation_policy.ts | 9 +++++++ .../connection_navigation_policy.ts | 9 +++++++ core/keyboard_nav/field_navigation_policy.ts | 18 +++++++++++++ .../flyout_button_navigation_policy.ts | 9 +++++++ core/keyboard_nav/flyout_navigation_policy.ts | 9 +++++++ .../flyout_separator_navigation_policy.ts | 9 +++++++ .../workspace_navigation_policy.ts | 9 +++++++ core/navigator.ts | 11 +++++--- core/rendered_connection.ts | 9 ------- core/workspace_svg.ts | 15 ++++------- 16 files changed, 125 insertions(+), 72 deletions(-) diff --git a/core/block_svg.ts b/core/block_svg.ts index e4cc27c060a..1ec4d98eafe 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -1824,15 +1824,6 @@ export class BlockSvg return true; } - /** - * Returns whether or not this block can be navigated to via the keyboard. - * - * @returns True if this block is keyboard navigable, otherwise false. - */ - isNavigable() { - return true; - } - /** * Returns this block's class. * diff --git a/core/field.ts b/core/field.ts index 589817b19d7..7aa348d52c3 100644 --- a/core/field.ts +++ b/core/field.ts @@ -1385,20 +1385,6 @@ export abstract class Field ); } - /** - * Returns whether or not this field is accessible by keyboard navigation. - * - * @returns True if this field is keyboard accessible, otherwise false. - */ - isNavigable() { - return ( - this.isClickable() && - this.isCurrentlyEditable() && - !(this.getSourceBlock()?.isSimpleReporter() && this.isFullBlockField()) && - this.getParentInput().isVisible() - ); - } - /** * Returns this field's class. * diff --git a/core/flyout_button.ts b/core/flyout_button.ts index 6790145a7b4..605a1358535 100644 --- a/core/flyout_button.ts +++ b/core/flyout_button.ts @@ -413,16 +413,6 @@ export class FlyoutButton return true; } - /** - * Returns whether or not this button is accessible through keyboard - * navigation. - * - * @returns True if this button is keyboard accessible, otherwise false. - */ - isNavigable() { - return true; - } - /** * Returns this button's class. * diff --git a/core/flyout_separator.ts b/core/flyout_separator.ts index 02737879a7c..73698b3dd6c 100644 --- a/core/flyout_separator.ts +++ b/core/flyout_separator.ts @@ -5,6 +5,8 @@ */ import type {IBoundedElement} from './interfaces/i_bounded_element.js'; +import type {IFocusableNode} from './interfaces/i_focusable_node.js'; +import type {IFocusableTree} from './interfaces/i_focusable_tree.js'; import type {INavigable} from './interfaces/i_navigable.js'; import {Rect} from './utils/rect.js'; @@ -12,7 +14,7 @@ import {Rect} from './utils/rect.js'; * Representation of a gap between elements in a flyout. */ export class FlyoutSeparator - implements IBoundedElement, INavigable + implements IBoundedElement, INavigable, IFocusableNode { private x = 0; private y = 0; @@ -75,6 +77,27 @@ export class FlyoutSeparator getClass() { return FlyoutSeparator; } + + /** See IFocusableNode.getFocusableElement. */ + getFocusableElement(): HTMLElement | SVGElement { + throw new Error('Cannot be focused'); + } + + /** See IFocusableNode.getFocusableTree. */ + getFocusableTree(): IFocusableTree { + throw new Error('Cannot be focused'); + } + + /** See IFocusableNode.onNodeFocus. */ + onNodeFocus(): void {} + + /** See IFocusableNode.onNodeBlur. */ + onNodeBlur(): void {} + + /** See IFocusableNode.canBeFocused. */ + canBeFocused(): boolean { + return false; + } } /** diff --git a/core/interfaces/i_navigable.ts b/core/interfaces/i_navigable.ts index f41e7882934..dcb8ad50fca 100644 --- a/core/interfaces/i_navigable.ts +++ b/core/interfaces/i_navigable.ts @@ -4,24 +4,12 @@ * SPDX-License-Identifier: Apache-2.0 */ +import type {IFocusableNode} from './i_focusable_node.js'; + /** * Represents a UI element which can be navigated to using the keyboard. */ -export interface INavigable { - /** - * Returns whether or not this specific instance should be reachable via - * keyboard navigation. - * - * Implementors should generally return true, unless there are circumstances - * under which this item should be skipped while using keyboard navigation. - * Common examples might include being disabled, invalid, readonly, or purely - * a visual decoration. For example, while Fields are navigable, non-editable - * fields return false, since they cannot be interacted with when focused. - * - * @returns True if this element should be included in keyboard navigation. - */ - isNavigable(): boolean; - +export interface INavigable extends IFocusableNode { /** * Returns the class of this instance. * diff --git a/core/interfaces/i_navigation_policy.ts b/core/interfaces/i_navigation_policy.ts index b263aac5987..d2b694c897a 100644 --- a/core/interfaces/i_navigation_policy.ts +++ b/core/interfaces/i_navigation_policy.ts @@ -43,4 +43,18 @@ export interface INavigationPolicy { * there is none. */ getPreviousSibling(current: T): INavigable | null; + + /** + * Returns whether or not the given instance should be reachable via keyboard + * navigation. + * + * Implementors should generally return true, unless there are circumstances + * under which this item should be skipped while using keyboard navigation. + * Common examples might include being disabled, invalid, readonly, or purely + * a visual decoration. For example, while Fields are navigable, non-editable + * fields return false, since they cannot be interacted with when focused. + * + * @returns True if this element should be included in keyboard navigation. + */ + isNavigable(current: T): boolean; } diff --git a/core/keyboard_nav/block_navigation_policy.ts b/core/keyboard_nav/block_navigation_policy.ts index b073253f98e..0a385643510 100644 --- a/core/keyboard_nav/block_navigation_policy.ts +++ b/core/keyboard_nav/block_navigation_policy.ts @@ -114,4 +114,13 @@ export class BlockNavigationPolicy implements INavigationPolicy { } return block.outputConnection; } + + /** + * Returns whether or not the given block can be navigated to. + * + * @returns True if the given block can be focused. + */ + isNavigable(current: BlockSvg): boolean { + return current.canBeFocused(); + } } diff --git a/core/keyboard_nav/connection_navigation_policy.ts b/core/keyboard_nav/connection_navigation_policy.ts index 1dfb0f64edd..5756dca37b5 100644 --- a/core/keyboard_nav/connection_navigation_policy.ts +++ b/core/keyboard_nav/connection_navigation_policy.ts @@ -166,4 +166,13 @@ export class ConnectionNavigationPolicy } return block.outputConnection; } + + /** + * Returns whether or not the given connection can be navigated to. + * + * @returns True if the given connection can be focused. + */ + isNavigable(current: RenderedConnection): boolean { + return current.canBeFocused(); + } } diff --git a/core/keyboard_nav/field_navigation_policy.ts b/core/keyboard_nav/field_navigation_policy.ts index a0e43001e4f..7a3f2a34ed8 100644 --- a/core/keyboard_nav/field_navigation_policy.ts +++ b/core/keyboard_nav/field_navigation_policy.ts @@ -88,4 +88,22 @@ export class FieldNavigationPolicy implements INavigationPolicy> { } return null; } + + /** + * Returns whether or not the given field can be navigated to. + * + * @returns True if the given field can be focused and navigated to. + */ + isNavigable(current: Field): boolean { + return ( + current.canBeFocused() && + current.isClickable() && + current.isCurrentlyEditable() && + !( + current.getSourceBlock()?.isSimpleReporter() && + current.isFullBlockField() + ) && + current.getParentInput().isVisible() + ); + } } diff --git a/core/keyboard_nav/flyout_button_navigation_policy.ts b/core/keyboard_nav/flyout_button_navigation_policy.ts index 8819bf588c6..af417eb107a 100644 --- a/core/keyboard_nav/flyout_button_navigation_policy.ts +++ b/core/keyboard_nav/flyout_button_navigation_policy.ts @@ -53,4 +53,13 @@ export class FlyoutButtonNavigationPolicy getPreviousSibling(_current: FlyoutButton): INavigable | null { return null; } + + /** + * Returns whether or not the given flyout button can be navigated to. + * + * @returns True if the given flyout button can be focused. + */ + isNavigable(current: FlyoutButton): boolean { + return current.canBeFocused(); + } } diff --git a/core/keyboard_nav/flyout_navigation_policy.ts b/core/keyboard_nav/flyout_navigation_policy.ts index 6a89a6fbdc6..1512bed7690 100644 --- a/core/keyboard_nav/flyout_navigation_policy.ts +++ b/core/keyboard_nav/flyout_navigation_policy.ts @@ -88,4 +88,13 @@ export class FlyoutNavigationPolicy implements INavigationPolicy { return flyoutContents[index].getElement(); } + + /** + * Returns whether or not the given flyout item can be navigated to. + * + * @returns True if the given flyout item can be focused. + */ + isNavigable(current: T): boolean { + return this.policy.isNavigable(current); + } } diff --git a/core/keyboard_nav/flyout_separator_navigation_policy.ts b/core/keyboard_nav/flyout_separator_navigation_policy.ts index d104c4c0074..a7636cf9302 100644 --- a/core/keyboard_nav/flyout_separator_navigation_policy.ts +++ b/core/keyboard_nav/flyout_separator_navigation_policy.ts @@ -30,4 +30,13 @@ export class FlyoutSeparatorNavigationPolicy getPreviousSibling(_current: FlyoutSeparator): INavigable | null { return null; } + + /** + * Returns whether or not the given flyout separator can be navigated to. + * + * @returns False. + */ + isNavigable(current: FlyoutSeparator): boolean { + return false; + } } diff --git a/core/keyboard_nav/workspace_navigation_policy.ts b/core/keyboard_nav/workspace_navigation_policy.ts index 91f3221b558..32fa52318ee 100644 --- a/core/keyboard_nav/workspace_navigation_policy.ts +++ b/core/keyboard_nav/workspace_navigation_policy.ts @@ -63,4 +63,13 @@ export class WorkspaceNavigationPolicy getPreviousSibling(_current: WorkspaceSvg): INavigable | null { return null; } + + /** + * Returns whether or not the given workspace can be navigated to. + * + * @returns True if the given workspace can be focused. + */ + isNavigable(current: WorkspaceSvg): boolean { + return current.canBeFocused(); + } } diff --git a/core/navigator.ts b/core/navigator.ts index 33e98c47d79..b726e77a623 100644 --- a/core/navigator.ts +++ b/core/navigator.ts @@ -59,7 +59,8 @@ export class Navigator { const result = this.get(current)?.getFirstChild(current); if (!result) return null; // If the child isn't navigable, don't traverse into it; check its peers. - if (!result.isNavigable()) return this.getNextSibling(result); + if (!this.get(result)?.isNavigable(result)) + return this.getNextSibling(result); return result; } @@ -72,7 +73,7 @@ export class Navigator { getParent>(current: T): INavigable | null { const result = this.get(current)?.getParent(current); if (!result) return null; - if (!result.isNavigable()) return this.getParent(result); + if (!this.get(result)?.isNavigable(result)) return this.getParent(result); return result; } @@ -85,7 +86,8 @@ export class Navigator { getNextSibling>(current: T): INavigable | null { const result = this.get(current)?.getNextSibling(current); if (!result) return null; - if (!result.isNavigable()) return this.getNextSibling(result); + if (!this.get(result)?.isNavigable(result)) + return this.getNextSibling(result); return result; } @@ -100,7 +102,8 @@ export class Navigator { ): INavigable | null { const result = this.get(current)?.getPreviousSibling(current); if (!result) return null; - if (!result.isNavigable()) return this.getPreviousSibling(result); + if (!this.get(result)?.isNavigable(result)) + return this.getPreviousSibling(result); return result; } } diff --git a/core/rendered_connection.ts b/core/rendered_connection.ts index ec1b4ed0a48..4298afa3f23 100644 --- a/core/rendered_connection.ts +++ b/core/rendered_connection.ts @@ -665,15 +665,6 @@ export class RenderedConnection | null as SVGElement | null; } - /** - * Returns whether or not this connection is keyboard-navigable. - * - * @returns True. - */ - isNavigable() { - return true; - } - /** * Returns this connection's class for keyboard navigation. * diff --git a/core/workspace_svg.ts b/core/workspace_svg.ts index 2dae154e05d..dd8cac242d2 100644 --- a/core/workspace_svg.ts +++ b/core/workspace_svg.ts @@ -2728,7 +2728,11 @@ export class WorkspaceSvg if (this.isFlyout && flyout) { for (const flyoutItem of flyout.getContents()) { const elem = flyoutItem.getElement(); - if (isFocusableNode(elem) && elem.getFocusableElement().id === id) { + if ( + isFocusableNode(elem) && + elem.canBeFocused() && + elem.getFocusableElement().id === id + ) { return elem; } } @@ -2817,15 +2821,6 @@ export class WorkspaceSvg return WorkspaceSvg; } - /** - * Returns whether or not this workspace is keyboard-navigable. - * - * @returns True. - */ - isNavigable() { - return true; - } - /** * Returns an object responsible for coordinating movement of focus between * items on this workspace in response to keyboard navigation commands. From 6422d95c90ce72ff45be27de888ad2855e8cea8e Mon Sep 17 00:00:00 2001 From: Aaron Dodson Date: Mon, 12 May 2025 15:25:13 -0700 Subject: [PATCH 2/3] fix: Fix JSDoc. --- core/keyboard_nav/block_navigation_policy.ts | 1 + core/keyboard_nav/connection_navigation_policy.ts | 1 + core/keyboard_nav/field_navigation_policy.ts | 1 + core/keyboard_nav/flyout_button_navigation_policy.ts | 1 + core/keyboard_nav/flyout_navigation_policy.ts | 1 + core/keyboard_nav/flyout_separator_navigation_policy.ts | 3 ++- core/keyboard_nav/workspace_navigation_policy.ts | 1 + 7 files changed, 8 insertions(+), 1 deletion(-) diff --git a/core/keyboard_nav/block_navigation_policy.ts b/core/keyboard_nav/block_navigation_policy.ts index 0a385643510..aac16c2a449 100644 --- a/core/keyboard_nav/block_navigation_policy.ts +++ b/core/keyboard_nav/block_navigation_policy.ts @@ -118,6 +118,7 @@ export class BlockNavigationPolicy implements INavigationPolicy { /** * Returns whether or not the given block can be navigated to. * + * @param current The instance to check for navigability. * @returns True if the given block can be focused. */ isNavigable(current: BlockSvg): boolean { diff --git a/core/keyboard_nav/connection_navigation_policy.ts b/core/keyboard_nav/connection_navigation_policy.ts index 5756dca37b5..8323a73e148 100644 --- a/core/keyboard_nav/connection_navigation_policy.ts +++ b/core/keyboard_nav/connection_navigation_policy.ts @@ -170,6 +170,7 @@ export class ConnectionNavigationPolicy /** * Returns whether or not the given connection can be navigated to. * + * @param current The instance to check for navigability. * @returns True if the given connection can be focused. */ isNavigable(current: RenderedConnection): boolean { diff --git a/core/keyboard_nav/field_navigation_policy.ts b/core/keyboard_nav/field_navigation_policy.ts index 7a3f2a34ed8..d4470e51788 100644 --- a/core/keyboard_nav/field_navigation_policy.ts +++ b/core/keyboard_nav/field_navigation_policy.ts @@ -92,6 +92,7 @@ export class FieldNavigationPolicy implements INavigationPolicy> { /** * Returns whether or not the given field can be navigated to. * + * @param current The instance to check for navigability. * @returns True if the given field can be focused and navigated to. */ isNavigable(current: Field): boolean { diff --git a/core/keyboard_nav/flyout_button_navigation_policy.ts b/core/keyboard_nav/flyout_button_navigation_policy.ts index af417eb107a..771a327b589 100644 --- a/core/keyboard_nav/flyout_button_navigation_policy.ts +++ b/core/keyboard_nav/flyout_button_navigation_policy.ts @@ -57,6 +57,7 @@ export class FlyoutButtonNavigationPolicy /** * Returns whether or not the given flyout button can be navigated to. * + * @param current The instance to check for navigability. * @returns True if the given flyout button can be focused. */ isNavigable(current: FlyoutButton): boolean { diff --git a/core/keyboard_nav/flyout_navigation_policy.ts b/core/keyboard_nav/flyout_navigation_policy.ts index 1512bed7690..1abdbfb6cd8 100644 --- a/core/keyboard_nav/flyout_navigation_policy.ts +++ b/core/keyboard_nav/flyout_navigation_policy.ts @@ -92,6 +92,7 @@ export class FlyoutNavigationPolicy implements INavigationPolicy { /** * Returns whether or not the given flyout item can be navigated to. * + * @param current The instance to check for navigability. * @returns True if the given flyout item can be focused. */ isNavigable(current: T): boolean { diff --git a/core/keyboard_nav/flyout_separator_navigation_policy.ts b/core/keyboard_nav/flyout_separator_navigation_policy.ts index a7636cf9302..2e69af37a66 100644 --- a/core/keyboard_nav/flyout_separator_navigation_policy.ts +++ b/core/keyboard_nav/flyout_separator_navigation_policy.ts @@ -34,9 +34,10 @@ export class FlyoutSeparatorNavigationPolicy /** * Returns whether or not the given flyout separator can be navigated to. * + * @param _current The instance to check for navigability. * @returns False. */ - isNavigable(current: FlyoutSeparator): boolean { + isNavigable(_current: FlyoutSeparator): boolean { return false; } } diff --git a/core/keyboard_nav/workspace_navigation_policy.ts b/core/keyboard_nav/workspace_navigation_policy.ts index 32fa52318ee..656b0453b86 100644 --- a/core/keyboard_nav/workspace_navigation_policy.ts +++ b/core/keyboard_nav/workspace_navigation_policy.ts @@ -67,6 +67,7 @@ export class WorkspaceNavigationPolicy /** * Returns whether or not the given workspace can be navigated to. * + * @param current The instance to check for navigability. * @returns True if the given workspace can be focused. */ isNavigable(current: WorkspaceSvg): boolean { From 89747342d81f1db4a07102d2c51b3e5741123772 Mon Sep 17 00:00:00 2001 From: Aaron Dodson Date: Mon, 12 May 2025 15:41:04 -0700 Subject: [PATCH 3/3] chore: Add braces. --- core/navigator.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/core/navigator.ts b/core/navigator.ts index b726e77a623..ffff2507c3a 100644 --- a/core/navigator.ts +++ b/core/navigator.ts @@ -59,8 +59,9 @@ export class Navigator { const result = this.get(current)?.getFirstChild(current); if (!result) return null; // If the child isn't navigable, don't traverse into it; check its peers. - if (!this.get(result)?.isNavigable(result)) + if (!this.get(result)?.isNavigable(result)) { return this.getNextSibling(result); + } return result; } @@ -86,8 +87,9 @@ export class Navigator { getNextSibling>(current: T): INavigable | null { const result = this.get(current)?.getNextSibling(current); if (!result) return null; - if (!this.get(result)?.isNavigable(result)) + if (!this.get(result)?.isNavigable(result)) { return this.getNextSibling(result); + } return result; } @@ -102,8 +104,9 @@ export class Navigator { ): INavigable | null { const result = this.get(current)?.getPreviousSibling(current); if (!result) return null; - if (!this.get(result)?.isNavigable(result)) + if (!this.get(result)?.isNavigable(result)) { return this.getPreviousSibling(result); + } return result; } }