Remove KeytipData usage from Link, Checkbox, Toggle (react-next) (#13742

) * export useKeytipData * remove KeytipData usage from Checkbox, add withKeytipData HOC * remove KeytipData for toggle * Remove keytipData for link * update example * cleanups * Change files * resolve comments * avoid adding keytipData prop * fixes * fix keytip set to invisible if addKeytipQueue is called multiple time * resolve comment
microsoft · Jul 10, 2020 · d07846f · d07846f
1 parent 8df4df3
commit d07846f
Show file tree

Hide file tree

Showing 32 changed files with 640 additions and 113 deletions.
diff --git a/change/@fluentui-react-next-2020-06-22-11-18-19-xgao-useKeytipData.json b/change/@fluentui-react-next-2020-06-22-11-18-19-xgao-useKeytipData.json
@@ -0,0 +1,8 @@
+{
+  "type": "prerelease",
+  "comment": "Remove KeytipData for Checkbox, Toggle, and Link.",
+  "packageName": "@fluentui/react-next",
+  "email": "xgao@microsoft.com",
+  "dependentChangeType": "patch",
+  "date": "2020-06-22T18:18:19.664Z"
+}
diff --git a/change/office-ui-fabric-react-2020-06-22-11-18-19-xgao-useKeytipData.json b/change/office-ui-fabric-react-2020-06-22-11-18-19-xgao-useKeytipData.json
@@ -0,0 +1,8 @@
+{
+  "type": "patch",
+  "comment": "Keytip: add useKeytipRef hook.",
+  "packageName": "office-ui-fabric-react",
+  "email": "xgao@microsoft.com",
+  "dependentChangeType": "patch",
+  "date": "2020-06-22T18:18:04.940Z"
+}
diff --git a/packages/office-ui-fabric-react/etc/office-ui-fabric-react.api.md b/packages/office-ui-fabric-react/etc/office-ui-fabric-react.api.md
@@ -8028,6 +8028,9 @@ export class Keytip extends React.Component<IKeytipProps, {}> {
 // @public
 export const KeytipData: React.FunctionComponent<IKeytipDataProps & IRenderComponent<{}>>;
 
+// @public (undocumented)
+export type KeytipDataOptions = IKeytipDataProps;
+
 // @public (undocumented)
 export const KeytipLayer: React.FunctionComponent<IKeytipLayerProps>;
 
@@ -9582,6 +9585,9 @@ export function updateSV(color: IColor, s: number, v: number): IColor;
 // @public
 export function updateT(color: IColor, t: number): IColor;
 
+// @public
+export function useKeytipRef<TElement extends HTMLElement = HTMLElement>(options: KeytipDataOptions): React.Ref<TElement>;
+
 // @public
 export enum ValidationState {
     invalid = 2,

diff --git a/packages/office-ui-fabric-react/src/components/KeytipData/KeytipData.tsx b/packages/office-ui-fabric-react/src/components/KeytipData/KeytipData.tsx
@@ -1,5 +1,6 @@
 import * as React from 'react';
 import { IRenderComponent } from '../../Utilities';
+import { DATAKTP_TARGET, DATAKTP_EXECUTE_TARGET } from '../../utilities/keytips/index';
 import { IKeytipDataProps } from './KeytipData.types';
 import { useKeytipData } from './useKeytipData';
 
@@ -9,7 +10,11 @@ import { useKeytipData } from './useKeytipData';
  */
 export const KeytipData: React.FunctionComponent<IKeytipDataProps & IRenderComponent<{}>> = props => {
   const { children, ...keytipDataProps } = props;
-  const { targetElementAttributes, executeElementAttributes, ariaDescribedBy } = useKeytipData(keytipDataProps);
+  const { keytipId, ariaDescribedBy } = useKeytipData(keytipDataProps);
 
-  return children({ ...targetElementAttributes, ...executeElementAttributes, 'aria-describedby': ariaDescribedBy });
+  return children({
+    [DATAKTP_TARGET]: keytipId,
+    [DATAKTP_EXECUTE_TARGET]: keytipId,
+    'aria-describedby': ariaDescribedBy,
+  });
 };
diff --git a/packages/office-ui-fabric-react/src/components/KeytipData/KeytipData.types.ts b/packages/office-ui-fabric-react/src/components/KeytipData/KeytipData.types.ts
@@ -18,3 +18,5 @@ export interface IKeytipDataProps {
    */
   disabled?: boolean;
 }
+
+export type KeytipDataOptions = IKeytipDataProps;
diff --git a/packages/office-ui-fabric-react/src/components/KeytipData/index.ts b/packages/office-ui-fabric-react/src/components/KeytipData/index.ts
@@ -1 +1,3 @@
 export * from './KeytipData';
+export { KeytipDataOptions } from './KeytipData.types';
+export * from './useKeytipRef';
diff --git a/packages/office-ui-fabric-react/src/components/KeytipData/useKeytipData.test.tsx b/packages/office-ui-fabric-react/src/components/KeytipData/useKeytipData.test.tsx
@@ -1,6 +1,7 @@
 import * as React from 'react';
 import { mount } from 'enzyme';
-import { useKeytipData, KeytipDataOptions } from './useKeytipData';
+import { KeytipDataOptions } from './KeytipData.types';
+import { useKeytipData } from './useKeytipData';
 import { KeytipManager } from '../../utilities/keytips/KeytipManager';
 
 describe('usePrevious', () => {
@@ -26,8 +27,7 @@ describe('usePrevious', () => {
     mount(<TestComponent />);
     expect(keytipData).toEqual({
       ariaDescribedBy: undefined,
-      executeElementAttributes: {},
-      targetElementAttributes: {},
+      keytipId: undefined,
     });
 
     expect(keytipManagerRegisterSpy).toBeCalledTimes(0);
@@ -49,12 +49,7 @@ describe('usePrevious', () => {
     mount(<TestComponent />);
     expect(keytipData).toEqual({
       ariaDescribedBy: 'ktp-layer-id ktp-a-1',
-      executeElementAttributes: {
-        'data-ktp-execute-target': 'ktp-a-1',
-      },
-      targetElementAttributes: {
-        'data-ktp-target': 'ktp-a-1',
-      },
+      keytipId: 'ktp-a-1',
     });
 
     expect(keytipManagerRegisterSpy).toBeCalledTimes(1);
@@ -78,12 +73,7 @@ describe('usePrevious', () => {
 
     expect(keytipData).toEqual({
       ariaDescribedBy: 'ktp-layer-id ktp-a-1',
-      executeElementAttributes: {
-        'data-ktp-execute-target': 'ktp-a-1',
-      },
-      targetElementAttributes: {
-        'data-ktp-target': 'ktp-a-1',
-      },
+      keytipId: 'ktp-a-1',
     });
 
     expect(keytipManagerRegisterSpy).toBeCalledTimes(1);
@@ -99,12 +89,7 @@ describe('usePrevious', () => {
 
     expect(keytipData).toEqual({
       ariaDescribedBy: 'ktp-layer-id ktp-b-1',
-      executeElementAttributes: {
-        'data-ktp-execute-target': 'ktp-b-1',
-      },
-      targetElementAttributes: {
-        'data-ktp-target': 'ktp-b-1',
-      },
+      keytipId: 'ktp-b-1',
     });
 
     expect(keytipManagerRegisterSpy).toBeCalledTimes(1);

diff --git a/packages/office-ui-fabric-react/src/components/KeytipData/useKeytipData.ts b/packages/office-ui-fabric-react/src/components/KeytipData/useKeytipData.ts
@@ -1,18 +1,18 @@
 import * as React from 'react';
 import { useConst, usePrevious } from '@uifabric/react-hooks';
 import { mergeAriaAttributeValues } from '../../Utilities';
-import { IKeytipDataProps } from './KeytipData.types';
+import { KeytipDataOptions } from './KeytipData.types';
 import { IKeytipProps } from '../../Keytip';
 import { KeytipManager, mergeOverflows, sequencesToID, getAriaDescribedBy } from '../../utilities/keytips/index';
 
-export type KeytipDataOptions = IKeytipDataProps;
-
 export interface IKeytipData {
   ariaDescribedBy: string | undefined;
-  targetElementAttributes: { [key: string]: string | undefined };
-  executeElementAttributes: { [key: string]: string | undefined };
+  keytipId: string | undefined;
 }
 
+/**
+ * Hook that creates attributes for components which are enabled with Keytip.
+ */
 export function useKeytipData(options: KeytipDataOptions): IKeytipData {
   const uniqueId = React.useRef<string>();
   const keytipProps: IKeytipProps | undefined = options.keytipProps
@@ -48,12 +48,11 @@ export function useKeytipData(options: KeytipDataOptions): IKeytipData {
 
   let nativeKeytipProps: IKeytipData = {
     ariaDescribedBy: undefined,
-    targetElementAttributes: {},
-    executeElementAttributes: {},
+    keytipId: undefined,
   };
 
   if (keytipProps) {
-    nativeKeytipProps = getKtpAttrs(keytipManager, keytipProps, options.ariaDescribedBy);
+    nativeKeytipProps = getKeytipData(keytipManager, keytipProps, options.ariaDescribedBy);
   }
 
   return nativeKeytipProps;
@@ -64,7 +63,7 @@ export function useKeytipData(options: KeytipDataOptions): IKeytipData {
  * @param keytipProps - options for Keytip
  * @param describedByPrepend - ariaDescribedBy value to prepend
  */
-function getKtpAttrs(
+function getKeytipData(
   keytipManager: KeytipManager,
   keytipProps: IKeytipProps,
   describedByPrepend?: string,
@@ -82,15 +81,10 @@ function getKtpAttrs(
   if (newKeytipProps.overflowSetSequence) {
     keySequences = mergeOverflows(keySequences, newKeytipProps.overflowSetSequence);
   }
-  const ktpId = sequencesToID(keySequences);
+  const keytipId = sequencesToID(keySequences);
 
   return {
     ariaDescribedBy,
-    targetElementAttributes: {
-      'data-ktp-target': ktpId,
-    },
-    executeElementAttributes: {
-      'data-ktp-execute-target': ktpId,
-    },
+    keytipId,
   };
 }
diff --git a/packages/office-ui-fabric-react/src/components/KeytipData/useKeytipRef.ts b/packages/office-ui-fabric-react/src/components/KeytipData/useKeytipRef.ts
@@ -0,0 +1,53 @@
+import * as React from 'react';
+import { KeytipDataOptions } from './KeytipData.types';
+import { DATAKTP_TARGET, DATAKTP_EXECUTE_TARGET, DATAKTP_ARIA_TARGET } from '../../utilities/keytips/index';
+import { useKeytipData } from './useKeytipData';
+
+/**
+ * Hook that creates a ref which is used for passing to Keytip target element.
+ * The ref will handle setting the attributes needed for Keytip to work.
+ */
+export function useKeytipRef<TElement extends HTMLElement = HTMLElement>(
+  options: KeytipDataOptions,
+): React.Ref<TElement> {
+  const { keytipId, ariaDescribedBy } = useKeytipData(options);
+
+  const contentRef: React.Ref<TElement> = (contentElement: TElement | null): void => {
+    if (!contentElement) {
+      return;
+    }
+
+    const targetElement = findFirstElement(contentElement, DATAKTP_TARGET) || contentElement;
+    const executeElement = findFirstElement(contentElement, DATAKTP_EXECUTE_TARGET) || targetElement;
+    const ariaElement = findFirstElement(contentElement, DATAKTP_ARIA_TARGET) || executeElement;
+
+    setAttribute(targetElement, DATAKTP_TARGET, keytipId);
+    setAttribute(executeElement, DATAKTP_EXECUTE_TARGET, keytipId);
+    setAttribute(ariaElement, 'aria-describedby', ariaDescribedBy, true);
+  };
+
+  return contentRef;
+}
+
+function setAttribute(
+  element: HTMLElement | null,
+  attributeName: string,
+  attributeValue: string | undefined,
+  append: boolean = false,
+): void {
+  if (element && attributeValue) {
+    let value = attributeValue;
+    if (append) {
+      const currentValue = element.getAttribute(attributeName);
+      if (currentValue && currentValue.indexOf(attributeName) === -1) {
+        value = `${currentValue} ${attributeValue}`;
+      }
+    }
+
+    element.setAttribute(attributeName, value);
+  }
+}
+
+function findFirstElement(rootElement: HTMLElement, dataAttribute: string): HTMLElement | null {
+  return rootElement.querySelector(`[${dataAttribute}]`);
+}
diff --git a/packages/office-ui-fabric-react/src/components/KeytipLayer/KeytipLayer.base.tsx b/packages/office-ui-fabric-react/src/components/KeytipLayer/KeytipLayer.base.tsx
@@ -471,7 +471,11 @@ export class KeytipLayerBase extends React.Component<IKeytipLayerProps, IKeytipL
 
     // Add the keytip to the queue to show later
     if (this._keytipTree.isCurrentKeytipParent(keytipProps)) {
+      // Ensure existing children are still shown.
+      this._delayedKeytipQueue = this._delayedKeytipQueue.concat(this._keytipTree.currentKeytip?.children || []);
+
       this._addKeytipToQueue(sequencesToID(keytipProps.keySequences));
+
       // Ensure the child of currentKeytip is successfully added to currentKeytip's children and update it if not.
       // Note: Added this condition because KeytipTree.addNode was not always reflecting updates made to a parent node
       // in currentKeytip when that parent is the currentKeytip.

diff --git a/packages/office-ui-fabric-react/src/utilities/keytips/KeytipConstants.ts b/packages/office-ui-fabric-react/src/utilities/keytips/KeytipConstants.ts
@@ -3,6 +3,7 @@ export const KTP_SEPARATOR = '-';
 export const KTP_FULL_PREFIX = KTP_PREFIX + KTP_SEPARATOR;
 export const DATAKTP_TARGET = 'data-ktp-target';
 export const DATAKTP_EXECUTE_TARGET = 'data-ktp-execute-target';
+export const DATAKTP_ARIA_TARGET = 'data-ktp-aria-target';
 export const KTP_LAYER_ID = 'ktp-layer-id';
 export const KTP_ARIA_SEPARATOR = ', ';
 

diff --git a/packages/react-next/etc/react-next.api.md b/packages/react-next/etc/react-next.api.md
@@ -262,6 +262,7 @@ export interface ICheckboxProps extends React.ButtonHTMLAttributes<HTMLElement |
     disabled?: boolean;
     indeterminate?: boolean;
     inputProps?: React.ButtonHTMLAttributes<HTMLElement | HTMLButtonElement>;
+    // @deprecated
     keytipProps?: IKeytipProps;
     label?: SlotProp<React.HTMLAttributes<HTMLSpanElement>>;
     onChange?: (ev?: React.FormEvent<HTMLElement | HTMLInputElement>, checked?: boolean) => void;
@@ -697,6 +698,7 @@ export interface ILinkOptions {
 export interface ILinkProps extends ILinkHTMLAttributes<HTMLAnchorElement | HTMLButtonElement | HTMLElement> {
     componentRef?: IRefObject<ILink>;
     disabled?: boolean;
+    // @deprecated
     keytipProps?: IKeytipProps;
     styles?: IStyleFunctionOrObject<ILinkStyleProps, ILinkStyles>;
     theme?: ITheme;
@@ -1512,6 +1514,7 @@ export interface IToggleProps extends React.HTMLAttributes<HTMLElement> {
     defaultChecked?: boolean;
     disabled?: boolean;
     inlineLabel?: boolean;
+    // @deprecated
     keytipProps?: IKeytipProps;
     label?: string | JSX.Element;
     // @deprecated (undocumented)
@@ -2003,13 +2006,13 @@ export const ThemeProvider: React.FunctionComponent<ThemeProviderProps & {
 export { ThemeProviderProps }
 
 // @public (undocumented)
-export const Toggle: React.FunctionComponent<IToggleProps>;
+export const Toggle: React.FunctionComponent<IToggleProps & React.RefAttributes<HTMLDivElement>>;
 
 // @public (undocumented)
 export const ToggleBase: import("@fluentui/react-compose").ComponentWithAs<"div", IToggleProps>;
 
 // @public
-export const useLink: (props: ILinkProps) => any;
+export const useLink: (props: ILinkProps, forwardedRef: React.Ref<HTMLElement>) => any;
 
 export { useTheme }
 

diff --git a/packages/react-next/src/Keytip.ts b/packages/react-next/src/Keytip.ts
@@ -1 +1 @@
-export * from 'office-ui-fabric-react/lib/Keytip';
+export * from './components/Keytip/index';
diff --git a/packages/react-next/src/components/Checkbox/Checkbox.base.tsx b/packages/react-next/src/components/Checkbox/Checkbox.base.tsx
@@ -1,8 +1,6 @@
 import * as React from 'react';
 import { compose, mergeProps } from '@fluentui/react-compose';
-import { mergeAriaAttributeValues } from '../../Utilities';
 import { ICheckboxProps, ICheckboxSlots, ICheckboxState, ICheckboxSlotProps } from './Checkbox.types';
-import { KeytipData } from '../../KeytipData';
 import { useCheckbox } from './useCheckbox';
 
 const defaultSlots: Omit<ICheckboxSlots, 'root'> = {
@@ -15,30 +13,20 @@ const defaultSlots: Omit<ICheckboxSlots, 'root'> = {
 
 export const CheckboxBase = compose<'div', ICheckboxProps, {}, ICheckboxProps, {}>(
   (props, forwardedRef, composeOptions) => {
-    const { slotProps, slots, state } = mergeProps<ICheckboxProps, ICheckboxState, ICheckboxSlots, ICheckboxSlotProps>(
+    const { slotProps, slots } = mergeProps<ICheckboxProps, ICheckboxState, ICheckboxSlots, ICheckboxSlotProps>(
       composeOptions.state,
       composeOptions,
     );
 
-    const { disabled, keytipProps } = state;
-
     const onRenderLabel = (): JSX.Element => {
       return <slots.label {...slotProps.label} />;
     };
 
-    // tslint:disable-next-line:no-any
-    const renderContent = (keytipAttributes: any = {}) => (
+    return (
       <slots.root {...slotProps.root}>
-        <slots.input
-          {...slotProps.input}
-          data-ktp-execute-target={keytipAttributes['data-ktp-execute-target']}
-          aria-describedby={mergeAriaAttributeValues(
-            slotProps.input['aria-describedby'],
-            keytipAttributes['aria-describedby'],
-          )}
-        />
+        <slots.input {...slotProps.input} />
         <slots.container {...slotProps.container}>
-          <slots.checkbox {...slotProps.checkbox} data-ktp-target={keytipAttributes['data-ktp-target']}>
+          <slots.checkbox {...slotProps.checkbox}>
             <slots.checkmark {...slotProps.checkmark} />
           </slots.checkbox>
 
@@ -47,17 +35,6 @@ export const CheckboxBase = compose<'div', ICheckboxProps, {}, ICheckboxProps, {
         </slots.container>
       </slots.root>
     );
-
-    if (keytipProps) {
-      return (
-        <KeytipData keytipProps={keytipProps} disabled={disabled}>
-          {// tslint:disable-next-line:no-any
-          (keytipAttributes: any): JSX.Element => renderContent(keytipAttributes)}
-        </KeytipData>
-      );
-    }
-
-    return renderContent();
   },
   {
     slots: defaultSlots,

diff --git a/packages/react-next/src/components/Checkbox/Checkbox.types.ts b/packages/react-next/src/components/Checkbox/Checkbox.types.ts
@@ -132,7 +132,9 @@ export interface ICheckboxProps extends React.ButtonHTMLAttributes<HTMLElement |
   onRenderLabel?: IRenderFunction<ICheckboxProps>;
 
   /**
-   * Optional keytip for this checkbox
+   * Optional keytip.
+   *
+   * @deprecated This no longer works. Use `useKeytipData` hook instead.
    */
   keytipProps?: IKeytipProps;