fix(react-keytips): create alias nodes for persisted keytips

microsoft · Dec 13, 2024 · 9c3b4e8 · 9c3b4e8
1 parent c60982c
commit 9c3b4e8
Show file tree

Hide file tree

Showing 16 changed files with 134 additions and 124 deletions.
diff --git a/packages/react-keytips/src/components/Keytip/Keytip.types.ts b/packages/react-keytips/src/components/Keytip/Keytip.types.ts
@@ -60,10 +60,10 @@ export type KeytipProps = ComponentProps<KeytipSlots> & {
   /**
    * Whether this Keytip can be accessed at the root level.
    */
-  shortcut?: boolean;
+  isShortcut?: boolean;
   /**
-   * Whether or not this Keytip belongs to a component that has a menu Keytip mode will stay on when a menu is opened,
-   * even if the items in that menu have no keytips.
+   * Whether or not this Keytip belongs to a component that has a menu. Keytip mode will stay on when a menu is opened,
+   * even if the items in that menu have no keytips. If this is
    */
   hasMenu?: boolean;
 };

diff --git a/packages/react-keytips/src/components/Keytips/useKeytips.tsx b/packages/react-keytips/src/components/Keytips/useKeytips.tsx
@@ -2,7 +2,6 @@ import * as React from 'react';
 import {
   getIntrinsicElementProps,
   slot,
-  useIsomorphicLayoutEffect,
   useFluent,
   useTimeout,
 } from '@fluentui/react-components';
@@ -63,7 +62,7 @@ export const useKeytips_unstable = (props: KeytipsProps): KeytipsState => {
     },
     [state.inKeytipMode, onEnterKeytipsMode]
   );
-  //
+
   const handleExitKeytipMode = React.useCallback(
     (ev: KeyboardEvent) => {
       if (state.inKeytipMode) {
@@ -81,6 +80,7 @@ export const useKeytips_unstable = (props: KeytipsProps): KeytipsState => {
     (ev: KeyboardEvent) => {
       if (!state.inKeytipMode) return;
       const currentKeytip = tree.currentKeytip.current;
+
       if (currentKeytip && currentKeytip.target) {
         currentKeytip?.onReturn?.(ev, {
           event: ev,
@@ -114,21 +114,14 @@ export const useKeytips_unstable = (props: KeytipsProps): KeytipsState => {
     invokeEvent
   );
 
-  useIsomorphicLayoutEffect(() => {
+  React.useEffect(() => {
     const handleKeytipAdded = (keytip: KeytipWithId) => {
       tree.addNode(keytip);
 
-      if (keytip.isShortcut) {
-        dispatch({
-          type: ACTIONS.ADD_SHORTCUT,
-          shortcut: keytip,
-        });
-      } else {
-        dispatch({
-          type: ACTIONS.ADD_KEYTIP,
-          keytip,
-        });
-      }
+      dispatch({
+        type: ACTIONS.ADD_KEYTIP,
+        keytip,
+      });
 
       if (state.inKeytipMode && tree.isCurrentKeytipParent(keytip)) {
         showKeytips(tree.getChildren());
@@ -138,11 +131,7 @@ export const useKeytips_unstable = (props: KeytipsProps): KeytipsState => {
     const handleKeytipRemoved = (keytip: KeytipWithId) => {
       tree.removeNode(keytip.uniqueId);
 
-      if (keytip.isShortcut) {
-        dispatch({ type: ACTIONS.REMOVE_SHORTCUT, id: keytip.uniqueId });
-      } else {
-        dispatch({ type: ACTIONS.REMOVE_KEYTIP, id: keytip.uniqueId });
-      }
+      dispatch({ type: ACTIONS.REMOVE_KEYTIP, id: keytip.uniqueId });
     };
 
     const handleKeytipUpdated = (keytip: KeytipWithId) => {
@@ -152,17 +141,15 @@ export const useKeytips_unstable = (props: KeytipsProps): KeytipsState => {
     };
 
     subscribe(EVENTS.KEYTIP_ADDED, handleKeytipAdded);
-    subscribe(EVENTS.SHORTCUT_ADDED, handleKeytipAdded);
     subscribe(EVENTS.KEYTIP_UPDATED, handleKeytipUpdated);
     subscribe(EVENTS.KEYTIP_REMOVED, handleKeytipRemoved);
-    subscribe(EVENTS.SHORTCUT_REMOVED, handleKeytipRemoved);
 
     return () => {
       reset();
     };
   }, [state.inKeytipMode]);
 
-  useIsomorphicLayoutEffect(() => {
+  React.useEffect(() => {
     const controller = new AbortController();
     const { signal } = controller;
 
@@ -200,6 +187,7 @@ export const useKeytips_unstable = (props: KeytipsProps): KeytipsState => {
 
         dispatchEvent(EVENTS.KEYTIP_EXECUTED, node);
       }
+
       const currentChildren = tree.getChildren(node);
       const shouldExitKeytipMode =
         currentChildren.length === 0 && !node.dynamic;
@@ -219,13 +207,11 @@ export const useKeytips_unstable = (props: KeytipsProps): KeytipsState => {
   // executes keytip that was triggered via shortcut
   const handleShortcutExecution = React.useCallback(
     async (ev: KeyboardEvent, node: KeytipTreeNode) => {
-      const { dependentKeys, keySequences } = node;
+      const { keySequences } = node;
 
       if (!targetDocument) return;
 
-      const fullPath = [...dependentKeys, ...node.keySequences].reduce<
-        string[]
-      >((acc, key, idx) => {
+      const fullPath = keySequences.reduce<string[]>((acc, key, idx) => {
         if (idx === 0) acc.push(sequencesToID([key]));
         else
           acc.push(
@@ -234,7 +220,7 @@ export const useKeytips_unstable = (props: KeytipsProps): KeytipsState => {
         return acc;
       }, []);
 
-      const nodeId = sequencesToID([...dependentKeys, ...keySequences]);
+      const nodeId = sequencesToID(keySequences);
       const treeNode = tree.getNode(nodeId);
 
       // if the node has menu, trigger overflow keytip and current keytip to show the menu
@@ -286,7 +272,7 @@ export const useKeytips_unstable = (props: KeytipsProps): KeytipsState => {
     }
   }, []);
 
-  useIsomorphicLayoutEffect(() => {
+  React.useEffect(() => {
     if (!targetDocument) return;
 
     const handleInvokeEvent = (ev: KeyboardEvent) => {
@@ -330,15 +316,16 @@ export const useKeytips_unstable = (props: KeytipsProps): KeytipsState => {
       <Keytip key={keytipId} {...keytipProps} />
     ));
 
-  const hiddenKeytips = Object.values(state.keytips).map(({ keySequences }) => (
-    <span
-      key={sequencesToID(keySequences)}
-      id={sequencesToID(keySequences)}
-      style={VISUALLY_HIDDEN_STYLES}
-    >
-      {keySequences.join(', ')}
-    </span>
-  ));
+  const hiddenKeytips = Object.values(state.keytips).map(
+    ({ keySequences, uniqueId }) => {
+      const id = sequencesToID(keySequences);
+      return (
+        <span key={uniqueId} id={id} style={VISUALLY_HIDDEN_STYLES}>
+          {keySequences.join(', ')}
+        </span>
+      );
+    }
+  );
 
   return {
     components: {

diff --git a/packages/react-keytips/src/components/Keytips/useKeytipsState.ts b/packages/react-keytips/src/components/Keytips/useKeytipsState.ts
@@ -3,7 +3,13 @@ import type { KeytipProps, KeytipWithId } from '../Keytip';
 import { isTargetVisible, omit } from '../../utilities';
 import { ACTIONS } from '../../constants';
 
-type Keytips = Record<string, KeytipProps & { visibleInternal?: boolean }>;
+type Keytips = Record<
+  string,
+  KeytipProps & {
+    visibleInternal?: boolean;
+    uniqueId: string;
+  }
+>;
 
 type KeytipsState = {
   inKeytipMode: boolean;
@@ -17,8 +23,6 @@ type KeytipsAction =
   | { type: typeof ACTIONS.ADD_KEYTIP; keytip: KeytipWithId }
   | { type: typeof ACTIONS.UPDATE_KEYTIP; keytip: KeytipWithId }
   | { type: typeof ACTIONS.REMOVE_KEYTIP; id: string }
-  | { type: typeof ACTIONS.ADD_SHORTCUT; shortcut: KeytipWithId }
-  | { type: typeof ACTIONS.REMOVE_SHORTCUT; id: string }
   | {
       type: typeof ACTIONS.SET_VISIBLE_KEYTIPS;
       ids: string[];

diff --git a/packages/react-keytips/src/constants.ts b/packages/react-keytips/src/constants.ts
@@ -2,7 +2,6 @@ import {
   ArrowLeft,
   Enter,
   Space,
-  Tab,
   ArrowUp,
   ArrowDown,
   ArrowRight,
@@ -21,8 +20,6 @@ export const EVENTS = {
   KEYTIP_REMOVED: 'fui-keytip-removed',
   KEYTIP_UPDATED: 'fui-keytip-updated',
   KEYTIP_EXECUTED: 'fui-keytip-executed',
-  SHORTCUT_ADDED: 'fui-shortcut-added',
-  SHORTCUT_REMOVED: 'fui-shortcut-removed',
   SHORTCUT_EXECUTED: 'fui-shortcut-executed',
   ENTER_KEYTIP_MODE: 'fui-enter-keytip-mode',
   EXIT_KEYTIP_MODE: 'fui-exit-keytip-mode',
@@ -51,9 +48,7 @@ export const ACTIONS = {
   ENTER_KEYTIP_MODE: 'ENTER_KEYTIP_MODE',
   EXIT_KEYTIP_MODE: 'EXIT_KEYTIP_MODE',
   ADD_KEYTIP: 'ADD_KEYTIP',
-  ADD_SHORTCUT: 'ADD_SHORTCUT',
   REMOVE_KEYTIP: 'REMOVE_KEYTIP',
-  REMOVE_SHORTCUT: 'REMOVE_SHORTCUT',
   UPDATE_KEYTIP: 'UPDATE_KEYTIP',
   SET_VISIBLE_KEYTIPS: 'SET_VISIBLE_KEYTIPS',
   SET_SEQUENCE: 'SET_SEQUENCE',

diff --git a/packages/react-keytips/src/hooks/useEventService.ts b/packages/react-keytips/src/hooks/useEventService.ts
@@ -1,4 +1,4 @@
-import { useCallback, useRef, useEffect } from 'react';
+import * as React from 'react';
 import { useFluent } from '@fluentui/react-components';
 import { EVENTS } from '../constants';
 import type { KeytipWithId } from '../components/Keytip';
@@ -13,8 +13,6 @@ type PayloadDefinition = {
   [EVENTS.KEYTIP_ADDED]: KeytipWithId;
   [EVENTS.KEYTIP_REMOVED]: KeytipWithId;
   [EVENTS.KEYTIP_EXECUTED]: KeytipTreeNode;
-  [EVENTS.SHORTCUT_ADDED]: KeytipWithId;
-  [EVENTS.SHORTCUT_REMOVED]: KeytipWithId;
   [EVENTS.SHORTCUT_EXECUTED]: KeytipTreeNode;
 };
 
@@ -35,9 +33,11 @@ function createEventHandler<T>(
 
 export function useEventService() {
   const { targetDocument } = useFluent();
-  const controller = useRef<AbortController | null>(new AbortController());
+  const controller = React.useRef<AbortController | null>(
+    new AbortController()
+  );
 
-  const dispatch = useCallback(
+  const dispatch = React.useCallback(
     <T extends EventType>(eventName: T, payload?: PayloadDefinition[T]) => {
       const event = payload
         ? new CustomEvent(eventName, { detail: payload })
@@ -47,7 +47,7 @@ export function useEventService() {
     [targetDocument]
   );
 
-  const subscribe = useCallback(
+  const subscribe = React.useCallback(
     <T extends EventType>(
       event: T,
       handler: (payload: PayloadDefinition[T]) => void
@@ -69,14 +69,14 @@ export function useEventService() {
     [targetDocument]
   );
 
-  const reset = useCallback(() => {
+  const reset = React.useCallback(() => {
     if (controller.current) {
       controller.current.abort();
       controller.current = null;
     }
   }, []);
 
-  useEffect(() => {
+  React.useEffect(() => {
     return () => {
       reset();
     };

diff --git a/packages/react-keytips/src/hooks/useHotkeys.ts b/packages/react-keytips/src/hooks/useHotkeys.ts
@@ -1,10 +1,5 @@
 import * as React from 'react';
-
-import {
-  useIsomorphicLayoutEffect,
-  useFluent,
-  useTimeout,
-} from '@fluentui/react-components';
+import { useFluent, useTimeout } from '@fluentui/react-components';
 import { KeytipsProps } from '../components/Keytips/Keytips.types';
 
 type Options = {
@@ -73,7 +68,7 @@ export const useHotkeys = (
   const doc = target ?? targetDocument;
   const activeKeys = React.useRef<Set<string>>(new Set());
 
-  useIsomorphicLayoutEffect(() => {
+  React.useEffect(() => {
     const handleInvokeEvent = (ev: KeyboardEvent) => {
       hotkeys.forEach(
         ([hotkey, handler, options = { preventDefault: true }]) => {

diff --git a/packages/react-keytips/src/hooks/useKeytipRef.ts b/packages/react-keytips/src/hooks/useKeytipRef.ts
@@ -11,19 +11,22 @@ const isEqualArray = (a: string[], b: string[]) => {
 
 export const useKeytipRef = <
   T extends HTMLElement = HTMLButtonElement | HTMLAnchorElement
->(
-  keytip: Omit<KeytipProps, 'uniqueId'>
-): React.Dispatch<React.SetStateAction<T | null>> => {
-  const ktpId = React.useId();
-  const shortcutId = React.useId();
+>({
+  ...keytip
+}: KeytipProps): React.Dispatch<React.SetStateAction<T | null>> => {
   const [node, setNode] = React.useState<T | null>(null);
   const { dispatch } = useEventService();
 
+  const uniqueId = React.useId();
+  const keySequences = keytip.keySequences.map((k) => k.toLowerCase());
+  const id = sequencesToID(keySequences);
+
   const ktp = React.useMemo(
     () => ({
       ...keytip,
-      uniqueId: ktpId,
-      id: sequencesToID(keytip.keySequences),
+      id,
+      uniqueId,
+      keySequences,
       positioning: {
         target: node,
         ...keytip.positioning,
@@ -32,18 +35,7 @@ export const useKeytipRef = <
     [keytip, node]
   );
 
-  const shortcut = React.useMemo(
-    () => ({
-      uniqueId: shortcutId,
-      dependentKeys: keytip.keySequences.slice(0, -1),
-      keySequences: keytip.keySequences.slice(-1),
-      isShortcut: true,
-      content: '',
-    }),
-    [keytip]
-  );
-
-  const prevKeytip = usePrevious(ktp);
+  const prevKeytip = usePrevious(keytip);
 
   // this will run on every render, in order to update the keytip if the keySequences change
   React.useEffect(() => {
@@ -56,11 +48,9 @@ export const useKeytipRef = <
 
   React.useEffect(() => {
     dispatch(EVENTS.KEYTIP_ADDED, ktp);
-    if (ktp.shortcut) dispatch(EVENTS.SHORTCUT_ADDED, shortcut);
 
     return () => {
       dispatch(EVENTS.KEYTIP_REMOVED, ktp);
-      if (ktp.shortcut) dispatch(EVENTS.SHORTCUT_REMOVED, shortcut);
     };
   }, [node]);
 

diff --git a/packages/react-keytips/src/hooks/useTree.test.ts b/packages/react-keytips/src/hooks/useTree.test.ts
@@ -117,7 +117,7 @@ describe('useTree', () => {
       expect(result.current.root).toEqual({
         id: KTP_ROOT_ID,
         children: new Set(),
-        dependentKeys: [],
+        isShortcut: false,
         target: null,
         hasMenu: false,
         parent: '',