fix(react-keytips): do not show keytips for disabled targets (#256)

Co-authored-by: Dmytro Kirpa <[email protected]>
microsoft · Nov 5, 2024 · fe0e588 · fe0e588
1 parent 61dab23
commit fe0e588
Show file tree

Hide file tree

Showing 13 changed files with 152 additions and 17 deletions.
diff --git a/change/@fluentui-contrib-react-keytips-5580747c-3c9f-4750-9854-00b3d534e82b.json b/change/@fluentui-contrib-react-keytips-5580747c-3c9f-4750-9854-00b3d534e82b.json
@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "fix: do not show keytips for disabled targets",
+  "packageName": "@fluentui-contrib/react-keytips",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}
diff --git a/packages/react-keytips/package.json b/packages/react-keytips/package.json
@@ -13,7 +13,7 @@
   "dependencies": {
     "@fluentui/react-jsx-runtime": "^9.0.29",
     "@fluentui/react-utilities": "^9.16.0",
-    "@fluentui/react-positioning": "^9.16.0",
+    "@fluentui/react-positioning": "^9.15.0",
     "@fluentui/keyboard-keys": "~9.0.6",
     "@swc/helpers": "~0.5.2"
   }

diff --git a/packages/react-keytips/src/components/Keytip/Keytip.types.ts b/packages/react-keytips/src/components/Keytip/Keytip.types.ts
@@ -5,6 +5,7 @@ import type {
   ComponentState,
   Slot,
 } from '@fluentui/react-components';
+import type { InvokeEvent } from '../Keytips/Keytips.types';
 
 /**
  * Slot properties for Keytip
@@ -17,13 +18,13 @@ export type KeytipSlots = {
 };
 
 export type ExecuteKeytipEventHandler<E = HTMLElement> = EventHandler<
-  EventData<'keydown', KeyboardEvent> & {
+  EventData<InvokeEvent, KeyboardEvent> & {
     targetElement: E;
   }
 >;
 
 export type ReturnKeytipEventHandler<E = HTMLElement> = EventHandler<
-  EventData<'keydown', KeyboardEvent> & {
+  EventData<InvokeEvent, KeyboardEvent> & {
     targetElement: E;
   }
 >;

diff --git a/packages/react-keytips/src/components/Keytips/Keytips.component-browser-spec.tsx b/packages/react-keytips/src/components/Keytips/Keytips.component-browser-spec.tsx
@@ -5,6 +5,8 @@ import {
   KeytipsTabsExample,
   KeytipsOverflowMenuExample,
   KeytipsDynamicExample,
+  KeytipsDisabledTargetExample,
+  KeytipsDisabledMenuTargetExample,
 } from './KeytipsExamples.component-browser-spec';
 
 test.use({ viewport: { width: 500, height: 500 } });
@@ -20,6 +22,36 @@ test.describe('enter and exit from keytip mode interactions', () => {
     await expect(tooltip).toBeHidden();
   });
 
+  test('should not show keytip for disabled target', async ({
+    mount,
+    page,
+  }) => {
+    const component = await mount(<KeytipsDisabledTargetExample />);
+    const tooltip = page.getByRole('tooltip');
+    await expect(tooltip).toBeHidden();
+    await component.press('Alt+Meta');
+
+    await expect(page.getByRole('tooltip', { name: 'B' })).toBeVisible();
+    await expect(page.getByRole('tooltip', { name: 'A' })).toBeHidden();
+  });
+
+  test('should not show keytip for disabled target in menu', async ({
+    mount,
+    page,
+  }) => {
+    const component = await mount(<KeytipsDisabledMenuTargetExample />);
+    const tooltip = page.getByRole('tooltip');
+    await expect(tooltip).toBeHidden();
+    await component.press('Alt+Meta');
+
+    await expect(page.getByRole('tooltip', { name: 'A' })).toBeVisible();
+
+    await page.keyboard.press('a');
+
+    await expect(page.getByRole('tooltip', { name: 'C' })).toBeVisible();
+    await expect(page.getByRole('tooltip', { name: 'B' })).toBeHidden();
+  });
+
   test('should enter and exit with custom start and exit sequences', async ({
     mount,
     page,
@@ -57,7 +89,7 @@ test.describe('enter and exit from keytip mode interactions', () => {
 test.describe('test keytip navigation', () => {
   test('should work with tabs', async ({ mount, page }) => {
     const component = await mount(<KeytipsTabsExample />);
-    // should shoow first level of keytips
+    // should show first level of keytips
     await component.press('Alt+Meta');
 
     expect(await page.getByRole('tooltip').count()).toBe(2);
@@ -87,7 +119,7 @@ test.describe('test keytip navigation', () => {
 
     await expect(page.getByRole('tooltip', { name: 'A' })).toBeVisible();
     // should open nested menus and show next keytip
-    await component.press('a');
+    await page.keyboard.press('a');
     await expect(page.getByRole('menuitem', { name: 'Item 7' })).toBeVisible();
     await expect(page.getByRole('tooltip', { name: 'B' })).toBeVisible();
 

diff --git a/packages/react-keytips/src/components/Keytips/Keytips.types.ts b/packages/react-keytips/src/components/Keytips/Keytips.types.ts
@@ -11,6 +11,8 @@ export type KeytipsSlots = {
   root: Slot<'div'>;
 };
 
+export type InvokeEvent = 'keydown' | 'keyup';
+
 type OnExitKeytipsModeData = EventData<'keydown', KeyboardEvent>;
 type OnEnterKeytipsModeData = EventData<'keydown', KeyboardEvent>;
 
@@ -57,7 +59,7 @@ export type KeytipsProps = ComponentProps<KeytipsSlots> &
      * Event responsible for invoking keytips.
      * @default 'keydown'
      * */
-    invokeEvent?: 'keydown' | 'keyup';
+    invokeEvent?: InvokeEvent;
   };
 
 /**

diff --git a/packages/react-keytips/src/components/Keytips/KeytipsExamples.component-browser-spec.tsx b/packages/react-keytips/src/components/Keytips/KeytipsExamples.component-browser-spec.tsx
@@ -95,6 +95,78 @@ export const KeytipsBasicExample = (props: KeytipsProps) => {
   );
 };
 
+export const KeytipsDisabledTargetExample = (props: KeytipsProps) => {
+  const onExecute: ExecuteKeytipEventHandler = (_, { targetElement }) => {
+    if (targetElement) targetElement.click();
+  };
+
+  const disabledKeytipTarget = useKeytipRef({
+    keySequences: ['a'],
+    content: 'A',
+    onExecute,
+  });
+
+  const keytipTarget = useKeytipRef({
+    keySequences: ['b'],
+    content: 'B',
+    onExecute,
+  });
+
+  return (
+    <>
+      <Keytips {...props} />
+      <Button ref={disabledKeytipTarget} disabled>
+        Disabled Button
+      </Button>
+      <Button ref={keytipTarget}>Normal Button</Button>
+    </>
+  );
+};
+
+export const KeytipsDisabledMenuTargetExample = (props: KeytipsProps) => {
+  const onExecute: ExecuteKeytipEventHandler = (_, { targetElement }) => {
+    if (targetElement) targetElement.click();
+  };
+
+  const keytipMenu = useKeytipRef({
+    keySequences: ['a'],
+    content: 'A',
+    onExecute,
+  });
+
+  const keytipMenuDisabled = useKeytipRef<HTMLDivElement>({
+    keySequences: ['a', 'b'],
+    content: 'B',
+    onExecute,
+  });
+
+  const keytipMenuEnabled = useKeytipRef<HTMLDivElement>({
+    keySequences: ['a', 'c'],
+    content: 'C',
+    onExecute,
+  });
+
+  return (
+    <>
+      <Keytips {...props} />
+      <Menu>
+        <MenuTrigger disableButtonEnhancement>
+          <Button ref={keytipMenu}>Toggle menu</Button>
+        </MenuTrigger>
+
+        <MenuPopover>
+          <MenuList>
+            <MenuItem ref={keytipMenuEnabled}>New </MenuItem>
+            <MenuItem disabled ref={keytipMenuDisabled}>
+              Open Folder
+            </MenuItem>
+          </MenuList>
+        </MenuPopover>
+      </Menu>
+    </>
+  );
+};
+
 export const KeytipsTabsExample = (props: KeytipsProps) => {
   const [selectedValue, setSelectedValue] = React.useState<TabValue>('1');
 

diff --git a/packages/react-keytips/src/components/Keytips/useKeytips.tsx b/packages/react-keytips/src/components/Keytips/useKeytips.tsx
@@ -10,7 +10,7 @@ import { EVENTS, VISUALLY_HIDDEN_STYLES, ACTIONS } from '../../constants';
 import type { KeytipWithId } from '../Keytip';
 import { Keytip } from '../Keytip';
 import { useEventService } from '../../hooks/useEventService';
-import { sequencesToID } from '../../utilities/index';
+import { sequencesToID, isDisabled } from '../../utilities/index';
 import { useTree } from '../../hooks/useTree';
 import type { KeytipTreeNode } from '../../hooks/useTree';
 import type { Hotkey } from '../../hooks/useHotkeys';
@@ -77,7 +77,7 @@ export const useKeytips_unstable = (props: KeytipsProps): KeytipsState => {
         if (currentKeytip.target) {
           currentKeytip?.onReturn?.(ev, {
             event: ev,
-            type: 'keydown',
+            type: invokeEvent,
             targetElement: currentKeytip.target,
           });
         }
@@ -165,10 +165,10 @@ export const useKeytips_unstable = (props: KeytipsProps): KeytipsState => {
     (ev: KeyboardEvent, node: KeytipTreeNode) => {
       tree.currentKeytip.current = node;
 
-      if (node.target) {
+      if (node.target && !isDisabled(node.target)) {
         node.onExecute?.(ev, {
           event: ev,
-          type: 'keydown',
+          type: invokeEvent,
           targetElement: node.target,
         });
       }

diff --git a/packages/react-keytips/src/components/Keytips/useKeytipsState.ts b/packages/react-keytips/src/components/Keytips/useKeytipsState.ts
@@ -63,8 +63,9 @@ const stateReducer: React.Reducer<KeytipsState, KeytipsAction> = (
         ...state,
         keytips: Object.keys(state.keytips).reduce((acc, key) => {
           const keytip = state.keytips[key];
+
           const isVisibleInDocument = isTargetVisible(
-            keytip.positioning?.target,
+            keytip.positioning?.target as HTMLElement,
             action.targetDocument?.defaultView
           );
 

diff --git a/packages/react-keytips/src/utilities/index.ts b/packages/react-keytips/src/utilities/index.ts
@@ -1,4 +1,5 @@
 export * from './sequencesToID';
 export * from './createNode';
 export * from './isTargetVisible';
+export * from './isDisabled';
 export * from './omit';
diff --git a/packages/react-keytips/src/utilities/isDisabled.ts b/packages/react-keytips/src/utilities/isDisabled.ts
@@ -0,0 +1,8 @@
+export const isDisabled = (target?: HTMLElement): boolean => {
+  if (!target) return false;
+
+  return (
+    target.hasAttribute('disabled') ||
+    target.getAttribute('aria-disabled')?.toLowerCase() === 'true'
+  );
+};
diff --git a/packages/react-keytips/src/utilities/isTargetVisible.ts b/packages/react-keytips/src/utilities/isTargetVisible.ts
@@ -1,10 +1,12 @@
-import type { PositioningProps } from '@fluentui/react-positioning';
+import { isDisabled } from './isDisabled';
 
 export const isTargetVisible = (
-  target?: PositioningProps['target'],
+  target?: HTMLElement,
   win?: Document['defaultView']
 ): boolean => {
   if (!target || !win) return false;
-  const style = win.getComputedStyle(target as HTMLElement);
+  if (isDisabled(target)) return false;
+
+  const style = win.getComputedStyle(target);
   return style.display !== 'none' && style.visibility !== 'hidden';
 };
diff --git a/packages/react-keytips/stories/Default.stories.tsx b/packages/react-keytips/stories/Default.stories.tsx
@@ -39,7 +39,9 @@ const useStyles = makeStyles({
 });
 
 const onExecute: ExecuteKeytipEventHandler = (_, { targetElement }) => {
-  if (targetElement) targetElement.click();
+  if (targetElement) {
+    targetElement.click();
+  }
 };
 
 const SplitButtonComponent = () => {
@@ -124,6 +126,12 @@ const MenuButtonComponent = () => {
 export const DefaultStory = () => {
   const classes = useStyles();
 
+  const disabledButton = useKeytipRef({
+    keySequences: ['b0'],
+    content: 'B0',
+    onExecute,
+  });
+
   const normalButton = useKeytipRef({
     keySequences: ['b1'],
     content: 'B1',
@@ -147,6 +155,9 @@ export const DefaultStory = () => {
     <>
       <div className={classes.column}>
         <div className={classes.row}>
+          <Button ref={disabledButton} disabled>
+            Disabled Button
+          </Button>
           <Button ref={normalButton}>Button</Button>
           <CompoundButton
             ref={compoundButton}

diff --git a/packages/react-keytips/stories/OverflowMenu.stories.tsx b/packages/react-keytips/stories/OverflowMenu.stories.tsx
@@ -1,6 +1,5 @@
 import * as React from 'react';
 import {
-  Keytips,
   ExecuteKeytipEventHandler,
   useKeytipRef,
 } from '@fluentui-contrib/react-keytips';
@@ -164,7 +163,6 @@ export const OverflowStory = () => {
 
   return (
     <>
-      <Keytips content="Alt Control" />
       <Overflow>
         <div className={mergeClasses(styles.container, styles.resizableArea)}>
           {itemIds.map((i) => (