diff --git a/gradle/changelog/fix-keyboard-iterator.yaml b/gradle/changelog/fix-keyboard-iterator.yaml new file mode 100644 index 0000000000..26d505f28e --- /dev/null +++ b/gradle/changelog/fix-keyboard-iterator.yaml @@ -0,0 +1,2 @@ +- type: fixed + description: Order of keyboard iterator items within the repository file search and repository overview diff --git a/scm-ui/ui-components/src/repos/RepositoryEntry.tsx b/scm-ui/ui-components/src/repos/RepositoryEntry.tsx index dcff63a8d4..c1ad26f160 100644 --- a/scm-ui/ui-components/src/repos/RepositoryEntry.tsx +++ b/scm-ui/ui-components/src/repos/RepositoryEntry.tsx @@ -22,11 +22,11 @@ import { ExtensionPoint, extensionPoints } from "@scm-manager/ui-extensions"; import RepositoryFlags from "./RepositoryFlags"; import styled from "styled-components"; import { useTranslation } from "react-i18next"; -import { useKeyboardIteratorTarget } from "@scm-manager/ui-shortcuts"; import { Card } from "@scm-manager/ui-layout"; import { Link } from "react-router-dom"; import { Menu } from "@scm-manager/ui-overlays"; import { Icon } from "@scm-manager/ui-buttons"; +import { useKeyboardIteratorTargetV2 } from "@scm-manager/ui-core"; type DateProp = Date | string; @@ -35,6 +35,7 @@ type Props = { // @VisibleForTesting // the baseDate is only to avoid failing snapshot tests baseDate?: DateProp; + expectedIndex?: number; }; const Avatar = styled.div` @@ -59,9 +60,9 @@ const DetailsRow = styled(Card.Row)` gap: 0.5rem; `; -const RepositoryEntry: FC = ({ repository, baseDate }) => { +const RepositoryEntry: FC = ({ repository, baseDate, expectedIndex }) => { const [t] = useTranslation("repos"); - const ref = useKeyboardIteratorTarget(); + const ref = useKeyboardIteratorTargetV2({ expectedIndex: expectedIndex ?? 0 }); const actions = () => ( diff --git a/scm-ui/ui-core/src/base/shortcuts/index.ts b/scm-ui/ui-core/src/base/shortcuts/index.ts index 752384a0b4..532a0cb98d 100644 --- a/scm-ui/ui-core/src/base/shortcuts/index.ts +++ b/scm-ui/ui-core/src/base/shortcuts/index.ts @@ -17,4 +17,9 @@ export { default as useShortcut } from "./useShortcut"; export { default as useShortcutDocs, ShortcutDocsContextProvider } from "./useShortcutDocs"; export { default as usePauseShortcuts } from "./usePauseShortcuts"; -export { useKeyboardIteratorTarget, KeyboardIterator, KeyboardSubIterator } from "./iterator/keyboardIterator"; +export { + useKeyboardIteratorTarget, + KeyboardIterator, + KeyboardSubIterator, + useKeyboardIteratorTargetV2, +} from "./iterator/keyboardIterator"; diff --git a/scm-ui/ui-core/src/base/shortcuts/iterator/callbackIterator.ts b/scm-ui/ui-core/src/base/shortcuts/iterator/callbackIterator.ts index 8ce6103285..6c11d86a13 100644 --- a/scm-ui/ui-core/src/base/shortcuts/iterator/callbackIterator.ts +++ b/scm-ui/ui-core/src/base/shortcuts/iterator/callbackIterator.ts @@ -20,6 +20,12 @@ const INACTIVE_INDEX = -1; export type Callback = () => void; +export type IterableCallback = { + item: Callback | CallbackIterator; + expectedIndex: number; + cleanup?: Callback; +}; + type Direction = "forward" | "backward"; /** @@ -31,11 +37,18 @@ export type CallbackRegistry = { * Registers the given item and returns its index to use in {@link deregister}. */ register: (item: Callback | CallbackIterator) => number; - /** * Use the index returned from {@link register} to de-register. */ deregister: (index: number) => void; + /** + * Registers the given iterable item while maintaining the order of the expected index of each item. + */ + registerItem?: (iterable: IterableCallback) => void; + /** + * Removes the passed iterable item. + */ + deregisterItem?: (iterable: IterableCallback) => void; }; const isSubiterator = (item?: Callback | CallbackIterator): item is CallbackIterator => @@ -51,6 +64,8 @@ const offset = (direction: Direction) => (direction === "forward" ? 1 : -1); * * ## Terminology * - Item: Either a callback or a nested iterator + * - Cleanup: A callback that is called, if the corresponding item is removed + * - Iterable: A wrapper containing an item, a cleanup callback and the index that this iterable is expected to be at * - Available: Item is a non-empty iterator OR a regular callback * - Inactive: Current index is -1 * - Activate: Move iterator while in inactive state OR call regular callback @@ -68,7 +83,7 @@ export class CallbackIterator implements CallbackRegistry { constructor( private readonly activeIndexRef: MutableRefObject, - private readonly itemsRef: MutableRefObject> + private readonly itemsRef: MutableRefObject> ) {} private get activeIndex() { @@ -83,7 +98,7 @@ export class CallbackIterator implements CallbackRegistry { return this.itemsRef.current; } - private get currentItem(): Callback | CallbackIterator | undefined { + private get currentItem(): IterableCallback | undefined { return this.items[this.activeIndex]; } @@ -101,9 +116,9 @@ export class CallbackIterator implements CallbackRegistry { private firstAvailableIndex = (direction: Direction, fromIndex = this.firstIndex(direction)) => { for (; direction === "forward" ? fromIndex < this.items.length : fromIndex >= 0; fromIndex += offset(direction)) { - const callback = this.items[fromIndex]; - if (callback) { - if (!isSubiterator(callback) || callback.hasNext(direction)) { + const iterableCallback = this.items[fromIndex]; + if (iterableCallback) { + if (!isSubiterator(iterableCallback.item) || iterableCallback.item.hasNext(direction)) { return fromIndex; } } @@ -116,14 +131,15 @@ export class CallbackIterator implements CallbackRegistry { }; private activateCurrentItem = (direction: Direction) => { - if (isSubiterator(this.currentItem)) { - this.currentItem.move(direction); + if (isSubiterator(this.currentItem?.item)) { + this.currentItem?.item.move(direction); } else if (this.currentItem) { - this.currentItem(); + this.currentItem.item(); } }; private setIndexAndActivateCurrentItem = (index: number | null, direction: Direction) => { + this.currentItem?.cleanup?.(); if (index !== null && index !== INACTIVE_INDEX) { this.activeIndex = index; this.activateCurrentItem(direction); @@ -131,11 +147,11 @@ export class CallbackIterator implements CallbackRegistry { }; private move = (direction: Direction) => { - if (isSubiterator(this.currentItem) && this.currentItem.hasNext(direction)) { - this.currentItem.move(direction); + if (isSubiterator(this.currentItem?.item) && this.currentItem?.item.hasNext(direction)) { + this.currentItem?.item.move(direction); } else { - if (isSubiterator(this.currentItem)) { - this.currentItem.reset(); + if (isSubiterator(this.currentItem?.item)) { + this.currentItem?.item.reset(); } let nextIndex: number | null; if (this.isInactive) { @@ -151,7 +167,7 @@ export class CallbackIterator implements CallbackRegistry { if (this.isInactive) { return this.hasAvailableIndex(inDirection); } - if (isSubiterator(this.currentItem) && this.currentItem.hasNext(inDirection)) { + if (isSubiterator(this.currentItem?.item) && this.currentItem?.item.hasNext(inDirection)) { return true; } return this.hasAvailableIndex(inDirection, this.activeIndex + offset(inDirection)); @@ -172,41 +188,66 @@ export class CallbackIterator implements CallbackRegistry { public reset = () => { this.activeIndex = INACTIVE_INDEX; for (const cb of this.items) { - if (isSubiterator(cb)) { - cb.reset(); + if (isSubiterator(cb.item)) { + cb.item.reset(); } } }; public register = (item: Callback | CallbackIterator) => { - if (isSubiterator(item)) { - item.parent = this; - } - return this.items.push(item) - 1; + const expectedIndex = this.items.length; + this.registerItem({ item, expectedIndex }); + return expectedIndex; }; public deregister = (index: number) => { - this.items.splice(index, 1); - if (this.activeIndex === index || this.activeIndex >= this.items.length) { - if (this.hasAvailableIndex("backward", index)) { - this.setIndexAndActivateCurrentItem(this.firstAvailableIndex("backward", index), "backward"); - } else if (this.hasAvailableIndex("forward", index)) { - this.setIndexAndActivateCurrentItem(this.firstAvailableIndex("forward", index), "backward"); - } else if (this.parent) { - if (this.parent.hasNext("forward")) { - this.parent.move("forward"); - } else if (this.parent.hasNext("backward")) { - this.parent.move("backward"); - } + if (this.items[index]) { + this.deregisterItem(this.items[index]); + } + }; + + public deregisterItem = (iterable: IterableCallback) => { + const itemIndex = this.items.findIndex((value) => value.expectedIndex === iterable.expectedIndex); + if (itemIndex === -1) { + return; + } + + const removedIterable = this.items[itemIndex]; + removedIterable.cleanup?.(); + + this.items.splice(itemIndex, 1); + if (this.activeIndex >= itemIndex) { + if (this.hasAvailableIndex("backward")) { + this.setIndexAndActivateCurrentItem(this.firstAvailableIndex("backward", itemIndex), "backward"); + } else if (this.hasAvailableIndex("forward")) { + this.setIndexAndActivateCurrentItem(this.firstAvailableIndex("forward", itemIndex), "forward"); + } else if (this.parent?.hasNext("forward")) { + this.parent?.move("forward"); + } else if (this.parent?.hasNext("backward")) { + this.parent?.move("backward"); } else { this.reset(); } } }; + + public registerItem = (iterable: IterableCallback) => { + if (isSubiterator(iterable.item)) { + iterable.item.parent = this; + } + + const insertAt = this.items.findIndex((value) => value.expectedIndex > iterable.expectedIndex); + + if (insertAt === -1) { + this.items.push(iterable); + } else { + this.items.splice(insertAt, 0, iterable); + } + }; } export const useCallbackIterator = (initialIndex = INACTIVE_INDEX) => { - const items = useRef>([]); + const items = useRef>([]); const activeIndex = useRef(initialIndex); return useMemo(() => new CallbackIterator(activeIndex, items), [activeIndex, items]); }; diff --git a/scm-ui/ui-core/src/base/shortcuts/iterator/keyboardIterator.test.tsx b/scm-ui/ui-core/src/base/shortcuts/iterator/keyboardIterator.test.tsx index 506a5a4af3..c1ba877467 100644 --- a/scm-ui/ui-core/src/base/shortcuts/iterator/keyboardIterator.test.tsx +++ b/scm-ui/ui-core/src/base/shortcuts/iterator/keyboardIterator.test.tsx @@ -277,9 +277,6 @@ describe("shortcutIterator", () => { expect(callback).toHaveBeenCalledTimes(1); expect(callback2).toHaveBeenCalledTimes(1); expect(callback3).toHaveBeenCalledTimes(1); - - expect(callback).toHaveBeenCalledBefore(callback2); - expect(callback2).toHaveBeenCalledBefore(callback3); }); it("should call first target that is not an empty subiterator", () => { @@ -378,8 +375,6 @@ describe("shortcutIterator", () => { expect(callback3).not.toHaveBeenCalled(); expect(callback2).toHaveBeenCalledTimes(1); expect(callback).toHaveBeenCalledTimes(1); - - expect(callback2).toHaveBeenCalledBefore(callback); }); it("should move subiterator if its active callback is de-registered", () => { diff --git a/scm-ui/ui-core/src/base/shortcuts/iterator/keyboardIterator.tsx b/scm-ui/ui-core/src/base/shortcuts/iterator/keyboardIterator.tsx index 6b504237a6..a28b707c77 100644 --- a/scm-ui/ui-core/src/base/shortcuts/iterator/keyboardIterator.tsx +++ b/scm-ui/ui-core/src/base/shortcuts/iterator/keyboardIterator.tsx @@ -17,7 +17,13 @@ import React, { FC, useCallback, useContext, useEffect, useRef } from "react"; import { useTranslation } from "react-i18next"; import { useShortcut } from "../index"; -import { Callback, CallbackIterator, CallbackRegistry, useCallbackIterator } from "./callbackIterator"; +import { + Callback, + CallbackIterator, + CallbackRegistry, + IterableCallback, + useCallbackIterator, +} from "./callbackIterator"; const KeyboardIteratorContext = React.createContext({ register: () => { @@ -33,6 +39,18 @@ const KeyboardIteratorContext = React.createContext({ console.warn("Keyboard iterator targets have to be declared inside a KeyboardIterator"); } }, + deregisterItem: () => { + if (process.env.NODE_ENV === "development") { + // eslint-disable-next-line no-console + console.warn("Keyboard iterator targets have to be declared inside a KeyboardIterator"); + } + }, + registerItem: () => { + if (process.env.NODE_ENV === "development") { + // eslint-disable-next-line no-console + console.warn("Keyboard iterator targets have to be declared inside a KeyboardIterator"); + } + }, }); export const useKeyboardIteratorItem = (item: Callback | CallbackIterator) => { @@ -43,6 +61,14 @@ export const useKeyboardIteratorItem = (item: Callback | CallbackIterator) => { }, [item, register, deregister]); }; +export const useKeyboardIteratorItemV2 = (iterable: IterableCallback) => { + const { registerItem, deregisterItem } = useContext(KeyboardIteratorContext); + useEffect(() => { + registerItem?.(iterable); + return () => deregisterItem?.(iterable); + }, [iterable, registerItem, deregisterItem]); +}; + export const KeyboardSubIteratorContextProvider: FC<{ initialIndex?: number }> = ({ children, initialIndex }) => { const callbackIterator = useCallbackIterator(initialIndex); @@ -73,6 +99,7 @@ export const KeyboardIteratorContextProvider: FC<{ initialIndex?: number }> = ({ }; /** + * @deprecated since version 3.8.0. Use {@link useKeyboardIteratorTargetV2} instead. * Use the {@link React.RefObject} returned from this hook to register a target to the nearest enclosing {@link KeyboardIterator} or {@link KeyboardSubIterator}. * * @example @@ -91,6 +118,32 @@ export function useKeyboardIteratorTarget(): React.RefCallback { return refCallback; } +/** + * @deprecated since version 3.8.0. Use {@link useKeyboardIteratorTargetV2} instead. + * Use the {@link React.RefObject} returned from this hook to register a target to the nearest enclosing {@link KeyboardIterator} or {@link KeyboardSubIterator}, + * while respecting its expected index / position. + * + * @example + * const ref = useKeyboardIteratorTarget({ expectedIndex: 0}); + * const target = + */ +export function useKeyboardIteratorTargetV2({ + expectedIndex, +}: { + expectedIndex: number; +}): React.RefCallback { + const ref = useRef(); + const callback = useCallback(() => ref.current?.focus(), []); + const cleanup = useCallback(() => ref.current?.blur(), []); + const refCallback: React.RefCallback = useCallback((el) => { + if (el) { + ref.current = el; + } + }, []); + useKeyboardIteratorItemV2({ item: callback, cleanup, expectedIndex }); + return refCallback; +} + /** * Allows keyboard users to iterate through a list of items, defined by enclosed {@link useKeyboardIteratorTarget} invocations. * diff --git a/scm-ui/ui-core/src/index.ts b/scm-ui/ui-core/src/index.ts index 21b0c2961c..e94a48d9ef 100644 --- a/scm-ui/ui-core/src/index.ts +++ b/scm-ui/ui-core/src/index.ts @@ -14,4 +14,4 @@ * along with this program. If not, see https://www.gnu.org/licenses/. */ -export * from "./base" +export * from "./base"; diff --git a/scm-ui/ui-types/src/Repositories.ts b/scm-ui/ui-types/src/Repositories.ts index a6963902ac..194828ab3b 100644 --- a/scm-ui/ui-types/src/Repositories.ts +++ b/scm-ui/ui-types/src/Repositories.ts @@ -85,4 +85,5 @@ export type RepositoryGroup = { name: string; namespace?: Namespace; repositories: Repository[]; + currentPageOffset?: number; }; diff --git a/scm-ui/ui-webapp/src/repos/codeSection/components/FileSearchHit.tsx b/scm-ui/ui-webapp/src/repos/codeSection/components/FileSearchHit.tsx index 8fbb3c127c..b52801e06c 100644 --- a/scm-ui/ui-webapp/src/repos/codeSection/components/FileSearchHit.tsx +++ b/scm-ui/ui-webapp/src/repos/codeSection/components/FileSearchHit.tsx @@ -17,13 +17,14 @@ import React from "react"; import { useTranslation } from "react-i18next"; import { urls } from "@scm-manager/ui-api"; -import { Icon, useKeyboardIteratorTarget } from "@scm-manager/ui-core"; +import { Icon, useKeyboardIteratorTargetV2 } from "@scm-manager/ui-core"; import { Link } from "react-router-dom"; import styled from "styled-components"; type FileSearchHitProps = { contentBaseUrl: string; path: string; + expectedIndex?: number; }; const IconColumn = styled.td` @@ -39,17 +40,17 @@ const LeftOverflowTd = styled.td` text-align: left !important; `; -export function FileSearchHit({ contentBaseUrl, path }: FileSearchHitProps) { +export function FileSearchHit({ contentBaseUrl, path, expectedIndex }: FileSearchHitProps) { const [t] = useTranslation("repos"); const link = urls.concat(contentBaseUrl, path); - const ref = useKeyboardIteratorTarget(); + const ref = useKeyboardIteratorTargetV2({ expectedIndex: expectedIndex ?? 0 }); return ( - + {path} diff --git a/scm-ui/ui-webapp/src/repos/codeSection/components/FileSearchResults.tsx b/scm-ui/ui-webapp/src/repos/codeSection/components/FileSearchResults.tsx index 9a13648ace..5a00fc28f6 100644 --- a/scm-ui/ui-webapp/src/repos/codeSection/components/FileSearchResults.tsx +++ b/scm-ui/ui-webapp/src/repos/codeSection/components/FileSearchResults.tsx @@ -32,8 +32,8 @@ const ResultTable: FC = ({ contentBaseUrl, paths }) => { - {paths.map((path) => ( - + {paths.map((path, index) => ( + ))} diff --git a/scm-ui/ui-webapp/src/repos/components/list/RepositoryGroupEntry.tsx b/scm-ui/ui-webapp/src/repos/components/list/RepositoryGroupEntry.tsx index 16a32c2a06..9477b99368 100644 --- a/scm-ui/ui-webapp/src/repos/components/list/RepositoryGroupEntry.tsx +++ b/scm-ui/ui-webapp/src/repos/components/list/RepositoryGroupEntry.tsx @@ -23,8 +23,14 @@ type Props = { }; const RepositoryGroupEntry: FC = ({ group }) => { - const entries = group.repositories.map((repository) => { - return ; + const entries = group.repositories.map((repository, index) => { + return ( + + ); }); return ; }; diff --git a/scm-ui/ui-webapp/src/repos/components/list/groupByNamespace.test.ts b/scm-ui/ui-webapp/src/repos/components/list/groupByNamespace.test.ts index 280fc688f3..4287c53b96 100644 --- a/scm-ui/ui-webapp/src/repos/components/list/groupByNamespace.test.ts +++ b/scm-ui/ui-webapp/src/repos/components/list/groupByNamespace.test.ts @@ -76,16 +76,19 @@ it("should group the repositories by their namespace", () => { name: "hitchhiker", namespace: { namespace: "hitchhiker" }, repositories: [hitchhikerHeartOfGold, hitchhikerPuzzle42, hitchhikerRestand], + currentPageOffset: 0, }, { name: "slarti", namespace: { namespace: "slarti" }, repositories: [slartiFjords, slartiBlueprintsFjords], + currentPageOffset: 3, }, { name: "zaphod", namespace: { namespace: "zaphod" }, repositories: [zaphodMarvinFirmware], + currentPageOffset: 5, }, ]; diff --git a/scm-ui/ui-webapp/src/repos/components/list/groupByNamespace.ts b/scm-ui/ui-webapp/src/repos/components/list/groupByNamespace.ts index 384a3e0565..3fe221db85 100644 --- a/scm-ui/ui-webapp/src/repos/components/list/groupByNamespace.ts +++ b/scm-ui/ui-webapp/src/repos/components/list/groupByNamespace.ts @@ -20,7 +20,7 @@ export default function groupByNamespace( repositories: Repository[], namespaces: NamespaceCollection ): RepositoryGroup[] { - const groups = {}; + const groups: Record = {}; for (const repository of repositories) { const groupName = repository.namespace; @@ -37,15 +37,16 @@ export default function groupByNamespace( group.repositories.push(repository); } - const groupArray = []; + const groupArray: RepositoryGroup[] = []; for (const groupName in groups) { groupArray.push(groups[groupName]); } groupArray.sort(sortByName); + applyOffsets(groupArray); return groupArray; } -function sortByName(a, b) { +function sortByName(a: RepositoryGroup, b: RepositoryGroup) { if (a.name < b.name) { return -1; } else if (a.name > b.name) { @@ -54,6 +55,14 @@ function sortByName(a, b) { return 0; } +function applyOffsets(groups: RepositoryGroup[]) { + let offset = 0; + for (const group of groups) { + group.currentPageOffset = offset; + offset += group.repositories.length; + } +} + function findNamespace(namespaces: NamespaceCollection, namespaceToFind: string) { return namespaces._embedded.namespaces.find((namespace) => namespace.namespace === namespaceToFind); }