Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 7 additions & 3 deletions core/keyboard_nav/block_navigation_policy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -181,20 +181,24 @@ function getBlockNavigationCandidates(
* `delta` relative to the current element's stack when navigating backwards.
*/
export function navigateStacks(current: ISelectable, delta: number) {
const stacks: IFocusableNode[] = (current.workspace as WorkspaceSvg)
const workspace = current.workspace as WorkspaceSvg;
const stacks: IFocusableNode[] = workspace
.getTopBoundedElements(true)
.filter((element: IBoundedElement) => isFocusableNode(element));
const currentIndex = stacks.indexOf(
current instanceof BlockSvg ? current.getRootBlock() : current,
);
const targetIndex = currentIndex + delta;
let result: IFocusableNode | null = null;
const loop = workspace.getCursor().getNavigationLoops();
if (targetIndex >= 0 && targetIndex < stacks.length) {
result = stacks[targetIndex];
} else if (targetIndex < 0) {
} else if (loop && targetIndex < 0) {
result = stacks[stacks.length - 1];
} else if (targetIndex >= stacks.length) {
} else if (loop && targetIndex >= stacks.length) {
result = stacks[0];
} else {
return null;
}

// When navigating to a previous block stack, our previous sibling is the last
Expand Down
63 changes: 53 additions & 10 deletions core/keyboard_nav/line_cursor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ export class LineCursor extends Marker {
/** Locations to try moving the cursor to after a deletion. */
private potentialNodes: IFocusableNode[] | null = null;

/** Whether or not navigation loops around when reaching the end. */
private navigationLoops = true;

/**
* @param workspace The workspace this cursor belongs to.
*/
Expand All @@ -64,7 +67,7 @@ export class LineCursor extends Marker {
const newNode = this.getNextNode(
curNode,
this.getValidationFunction(NavigationDirection.NEXT),
true,
this.getNavigationLoops(),
);

if (newNode) {
Expand All @@ -89,7 +92,7 @@ export class LineCursor extends Marker {
const newNode = this.getNextNode(
curNode,
this.getValidationFunction(NavigationDirection.IN),
true,
this.getNavigationLoops(),
);

if (newNode) {
Expand All @@ -112,7 +115,7 @@ export class LineCursor extends Marker {
const newNode = this.getPreviousNode(
curNode,
this.getValidationFunction(NavigationDirection.PREVIOUS),
true,
this.getNavigationLoops(),
);

if (newNode) {
Expand All @@ -137,7 +140,7 @@ export class LineCursor extends Marker {
const newNode = this.getPreviousNode(
curNode,
this.getValidationFunction(NavigationDirection.OUT),
true,
this.getNavigationLoops(),
);

if (newNode) {
Expand All @@ -158,12 +161,12 @@ export class LineCursor extends Marker {
const inNode = this.getNextNode(
curNode,
this.getValidationFunction(NavigationDirection.IN),
true,
this.getNavigationLoops(),
);
const nextNode = this.getNextNode(
curNode,
this.getValidationFunction(NavigationDirection.NEXT),
true,
this.getNavigationLoops(),
);

return inNode === nextNode;
Expand Down Expand Up @@ -219,11 +222,22 @@ export class LineCursor extends Marker {
getNextNode(
node: IFocusableNode | null,
isValid: (p1: IFocusableNode | null) => boolean,
// TODO: Consider deprecating and removing this argument.
loop: boolean,
): IFocusableNode | null {
if (!node || (!loop && this.getLastNode() === node)) return null;
const originalLoop = this.getNavigationLoops();
this.setNavigationLoops(loop);
Copy link
Contributor

Choose a reason for hiding this comment

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

this whole rigamarole is unfortunate, is the loop parameter in getNextNode already released? should we just deprecate it? or potentially should get navigationLoops policy be made part of the navigation policy instead and it can pass that value into here? (This suggestion may not make sense, I forget which order these calls happen in, let me know if you want me to dig in further to find a proposal that might make more sense)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I don't super love it, but yes that argument is public already, hence this stack-y thing. I think the cursor feels like the right level to control this (the navigation policies should just be given X, what's the next/previous thing? with filtering and larger-scale behavior delegated to the cursor), but the existence of this argument does complicate it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment with a TODO so we can come back to this when we're designing the eventual "real" solution? The argument should probably just be removed at that point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, and yeah if we're comfortable doing that that SGTM.


let result: IFocusableNode | null;
if (!node || (!loop && this.getLastNode() === node)) {
result = null;
} else {
result = this.getNextNodeImpl(node, isValid);
}

return this.getNextNodeImpl(node, isValid);
this.setNavigationLoops(originalLoop);

return result;
}

/**
Expand Down Expand Up @@ -273,11 +287,22 @@ export class LineCursor extends Marker {
getPreviousNode(
node: IFocusableNode | null,
isValid: (p1: IFocusableNode | null) => boolean,
// TODO: Consider deprecating and removing this argument.
loop: boolean,
): IFocusableNode | null {
if (!node || (!loop && this.getFirstNode() === node)) return null;
const originalLoop = this.getNavigationLoops();
this.setNavigationLoops(loop);

let result: IFocusableNode | null;
if (!node || (!loop && this.getFirstNode() === node)) {
result = null;
} else {
result = this.getPreviousNodeImpl(node, isValid);
}

return this.getPreviousNodeImpl(node, isValid);
this.setNavigationLoops(originalLoop);

return result;
}

/**
Expand Down Expand Up @@ -538,6 +563,24 @@ export class LineCursor extends Marker {
const first = this.getFirstNode();
return this.getPreviousNode(first, () => true, true);
}

/**
* Sets whether or not navigation should loop around when reaching the end
* of the workspace.
*
* @param loops True if navigation should loop around, otherwise false.
*/
setNavigationLoops(loops: boolean) {
this.navigationLoops = loops;
}

/**
* Returns whether or not navigation loops around when reaching the end of
* the workspace.
*/
getNavigationLoops(): boolean {
return this.navigationLoops;
}
}

registry.register(registry.Type.CURSOR, registry.DEFAULT, LineCursor);
Loading