Jspurlin/update keytip functionality (#20396)

* Make callouts aware of the WindowSegments

* Revert "Make callouts aware of the WindowSegments"

This reverts commit 68fffc2.

* Keytips: Make keytips aware of the visibility of their targets

* Change files

* Fix the lint errors

* Update keytip story

* Actually update the story to use a DefaultButton instead of an html button

* actually I just noticed there was already a ktp target span, undoing my story update, adding some text to the keytipElement, and making the size of the area larger

* Update story because js was executing before first paint causing keytips to not show up

apps/vr-tests/src/stories/Keytip.stories.tsx

@@ -2,12 +2,12 @@ import * as React from 'react';
 import Screener from 'screener-storybook/src/screener';
 import { storiesOf } from '@storybook/react';
 import { FabricDecorator } from '../utilities/index';
-import { Keytip } from '@fluentui/react';
+import { DelayedRender, Keytip } from '@fluentui/react';
 
 storiesOf('Keytip', module)
  .addDecorator(story => (
  <div style={{ width: '50px', height: '50px' }}>
- <span data-ktp-target={'ktp-a'} />
+ <span data-ktp-target={'ktp-a'}>text</span>
  {story()}
  </div>
  ))
@@ -22,10 +22,18 @@ storiesOf('Keytip', module)
  {story()}
  </Screener>,
  )
- .addStory('Root', () => <Keytip content={'A'} keySequences={['a']} visible={true} />)
+ .addStory('Root', () => (
+ <DelayedRender>
+ <Keytip content={'A'} keySequences={['a']} visible={true} />
+ </DelayedRender>
+ ))
  .addStory('Disabled', () => (
- <Keytip content={'A'} keySequences={['a']} visible={true} disabled={true} />
+ <DelayedRender>
+ <Keytip content={'A'} keySequences={['a']} visible={true} disabled={true} />
+ </DelayedRender>
  ))
  .addStory('Offset', () => (
- <Keytip content={'A'} keySequences={['a']} visible={true} offset={{ x: 15, y: 15 }} />
+ <DelayedRender>
+ <Keytip content={'A'} keySequences={['a']} visible={true} offset={{ x: 15, y: 15 }} />
+ </DelayedRender>
  ));

change/@fluentui-react-9c097d21-6944-4b2a-b35f-09bf82daa408.json

@@ -0,0 +1,7 @@
+{
+ "type": "minor",
+ "comment": "Keytips: Make keytips aware of the visibility of their targets",
+ "packageName": "@fluentui/react",
+ "email": "jspurlin@microsoft.com",
+ "dependentChangeType": "patch"
+}

change/@fluentui-utilities-dfe0019e-0dd3-4f0b-b462-ffd253a88cfb.json

@@ -0,0 +1,7 @@
+{
+ "type": "patch",
+ "comment": "Add isElementVisibleAndNotHidden utility function",
+ "packageName": "@fluentui/utilities",
+ "email": "jspurlin@microsoft.com",
+ "dependentChangeType": "patch"
+}

packages/react/etc/react.api.md

@@ -5562,6 +5562,7 @@ export interface IKeytipProps {
  disabled?: boolean;
  hasDynamicChildren?: boolean;
  hasMenu?: boolean;
+ hasOverflowSubMenu?: boolean;
  keySequences: string[];
  offset?: Point;
  onExecute?: (executeTarget: HTMLElement | null, target: HTMLElement | null) => void;

packages/react/src/components/Keytip/Keytip.tsx

@@ -1,10 +1,12 @@
 import * as React from 'react';
+import { getFirstVisibleElementFromSelector } from '../../Utilities';
 import { mergeOverflows, ktpTargetFromSequences } from '../../utilities/keytips/KeytipUtils';
 import { Callout } from '../../Callout';
 import { DirectionalHint } from '../../ContextualMenu';
 import { KeytipContent } from './KeytipContent';
 import { getCalloutStyles, getCalloutOffsetStyles } from './Keytip.styles';
 import type { IKeytipProps } from './Keytip.types';
+import type { Target } from '@fluentui/react-hooks';
 
 /**
  * A callout corresponding to another Fabric component to describe a key sequence that will activate that component
@@ -14,14 +16,22 @@ export class Keytip extends React.Component<IKeytipProps, {}> {
  const { keySequences, offset, overflowSetSequence } = this.props;
  let { calloutProps } = this.props;
 
- let keytipTarget: string;
+ let keytipTarget: Target;
  // Take into consideration the overflow sequence
  if (overflowSetSequence) {
  keytipTarget = ktpTargetFromSequences(mergeOverflows(keySequences, overflowSetSequence));
  } else {
  keytipTarget = ktpTargetFromSequences(keySequences);
  }
 
+ const element = getFirstVisibleElementFromSelector(keytipTarget);
+
+ if (!element) {
+ return <></>;
+ }
+
+ keytipTarget = element;
+
  if (offset) {
  // Set callout to top-left corner, will be further positioned in
  // getCalloutOffsetStyles

packages/react/src/components/Keytip/Keytip.types.ts

@@ -78,6 +78,12 @@ export interface IKeytipProps {
  * Keytip mode will stay on when a menu is opened, even if the items in that menu have no keytips
  */
  hasMenu?: boolean;
+
+ /**
+ * Whether or not this keytip belongs to a component that is in an overflow menu
+ * and also has a menu
+ */
+ hasOverflowSubMenu?: boolean;
 }
 
 /**

packages/react/src/components/KeytipLayer/IKeytipTreeNode.ts

@@ -55,4 +55,10 @@ export interface IKeytipTreeNode {
  * T/F if this keytip is a persisted keytip
  */
  persisted?: boolean;
+
+ /**
+ * Whether or not this keytip belongs to a component that is in an overflow menu
+ * and also has a menu
+ */
+ hasOverflowSubMenu?: boolean;
 }

packages/react/src/components/KeytipLayer/KeytipLayer.test.tsx

@@ -24,6 +24,7 @@ describe('KeytipLayer', () => {
 
  const keytipIdC = KTP_FULL_PREFIX + 'c';
  const uniqueIdC = '2';
+ const uniqueIdC2 = '22';
  const keytipC: IKeytipProps = {
  content: 'C',
  keySequences: ['c'],
@@ -316,6 +317,15 @@ describe('KeytipLayer', () => {
  // E2 should be triggered
  expect(nodeE2.onExecute).toBeCalled();
  });
+ it('Process a node with no matching visible element and is a submenu in an overflow', () => {
+ // Make C2 a submenu in an overflow
+ const onExecuteC2: jest.Mock = jest.fn();
+ ktpTree.addNode({ ...keytipC, hasOverflowSubMenu: true, onExecute: onExecuteC2 }, uniqueIdC2);
+ ktpTree.currentKeytip = ktpTree.root;
+ layerValue.processInput('c');
+ // C2 should be triggered
+ expect(onExecuteC2).toBeCalled();
+ });
  });
  });
 

packages/react/src/components/KeytipLayer/KeytipTree.ts

@@ -1,5 +1,5 @@
-import { find, values } from '../../Utilities';
-import { mergeOverflows, sequencesToID } from '../../utilities/keytips/KeytipUtils';
+import { find, isElementVisibleAndNotHidden, values } from '../../Utilities';
+import { ktpTargetFromSequences, mergeOverflows, sequencesToID } from '../../utilities/keytips/KeytipUtils';
 import { KTP_LAYER_ID } from '../../utilities/keytips/KeytipConstants';
 import type { IKeytipProps } from '../../Keytip';
 import type { IKeytipTreeNode } from './IKeytipTreeNode';
@@ -126,9 +126,52 @@ export class KeytipTree {
  */
  public getExactMatchedNode(keySequence: string, currentKeytip: IKeytipTreeNode): IKeytipTreeNode | undefined {
  const possibleNodes = this.getNodes(currentKeytip.children);
- return find(possibleNodes, (node: IKeytipTreeNode) => {
+ const matchingNodes = possibleNodes.filter((node: IKeytipTreeNode) => {
  return this._getNodeSequence(node) === keySequence && !node.disabled;
  });
+
+ // If we found no nodes, we are done
+ if (matchingNodes.length === 0) {
+ return undefined;
+ }
+
+ // Since the matching nodes all have the same key sequence,
+ // Grab the first one build the correct selector
+ const node = matchingNodes[0];
+
+ // If we have exactly one node, return it
+ if (matchingNodes.length === 1) {
+ return node;
+ }
+
+ // Get the potential target elements based on a selector from the sequences
+ const keySequences = node.keySequences;
+ const overflowSetSequence = node.overflowSetSequence;
+ const fullKeySequences = overflowSetSequence ? mergeOverflows(keySequences, overflowSetSequence) : keySequences;
+ const keytipTargetSelector = ktpTargetFromSequences(fullKeySequences);
+ const potentialTargetElements = document.querySelectorAll(keytipTargetSelector);
+
+ // If we have less nodes than the potential target elements,
+ // we won't be able to map element to node, return the first node.
+ // Note, the number of nodes could be more than the number of potential
+ // target elements, if an OverflowSet is involved
+ if (matchingNodes.length < potentialTargetElements.length) {
+ return node;
+ }
+
+ // Attempt to find the node that corresponds to the first visible/non-hidden element
+ const matchingIndex = Array.from(potentialTargetElements).findIndex((element: HTMLElement) =>
+ isElementVisibleAndNotHidden(element),
+ );
+ if (matchingIndex !== -1) {
+ return matchingNodes[matchingIndex];
+ }
+
+ // We did not find any visible elements associated with any of the nodes.
+ // We may be dealing with a keytip that is a submenu in an OverflowSet.
+ // Worst case, fall back to the first node returned
+ const overflowNode = matchingNodes.find(matchingNode => matchingNode.hasOverflowSubMenu);
+ return overflowNode || node;
  }
 
  /**
@@ -259,6 +302,7 @@ export class KeytipTree {
  onExecute,
  onReturn,
  disabled,
+ hasOverflowSubMenu,
  } = keytipProps;
  const node = {
  id,
@@ -272,6 +316,7 @@ export class KeytipTree {
  hasMenu,
  disabled,
  persisted,
+ hasOverflowSubMenu,
  };
  node.children = Object.keys(this.nodeMap).reduce((array: string[], nodeMapKey: string): string[] => {
  if (this.nodeMap[nodeMapKey].parent === id) {

packages/react/src/components/OverflowSet/OverflowButton.tsx

@@ -101,6 +101,7 @@ export const OverflowButton = (props: IOverflowSetProps) => {
  keytipSequences,
  overflowItem?.keytipProps?.keySequences,
  );
+ persistedKeytip.hasOverflowSubMenu = true;
  } else {
  // If the keytip doesn't have a submenu, just execute the original function
  persistedKeytip.onExecute = keytip.onExecute;

packages/utilities/etc/utilities.api.md

@@ -321,6 +321,9 @@ export function getFirstFocusable(rootElement: HTMLElement, currentElement: HTML
 // @public
 export function getFirstTabbable(rootElement: HTMLElement, currentElement: HTMLElement, includeElementsInFocusZones?: boolean, checkNode?: boolean): HTMLElement | null;
 
+// @public
+export function getFirstVisibleElementFromSelector(selector: string): Element | undefined;
+
 // @public
 export function getFocusableByIndexPath(parent: HTMLElement, path: number[]): HTMLElement | undefined;
 
@@ -765,6 +768,9 @@ export function isElementTabbable(element: HTMLElement, checkTabIndex?: boolean)
 // @public
 export function isElementVisible(element: HTMLElement | undefined | null): boolean;
 
+// @public
+export function isElementVisibleAndNotHidden(element: HTMLElement | undefined | null): boolean;
+
 // Warning: (ae-internal-missing-underscore) The name "ISerializableObject" should be prefixed with an underscore because the declaration is marked as @internal
 //
 // @internal

packages/utilities/src/dom.ts

@@ -4,6 +4,7 @@ export * from './dom/elementContainsAttribute';
 export * from './dom/findElementRecursive';
 export * from './dom/getChildren';
 export * from './dom/getDocument';
+export * from './dom/getFirstVisibleElementFromSelector';
 export * from './dom/getParent';
 export * from './dom/getRect';
 export * from './dom/getVirtualParent';

packages/utilities/src/dom/getFirstVisibleElementFromSelector.ts

@@ -0,0 +1,16 @@
+import { isElementVisibleAndNotHidden } from '../focus';
+import { getDocument } from './getDocument';
+
+/**
+ * Gets the first visible element that matches the given selector
+ * @param selector - The selector to use to find potential visible elements
+ * @returns The first visible element that matches the selector, otherwise undefined
+ *
+ * @public
+ */
+export function getFirstVisibleElementFromSelector(selector: string): Element | undefined {
+ const elements = getDocument()!.querySelectorAll(selector);
+
+ // Iterate across the elements that match the selector and return the first visible/non-hidden element
+ return Array.from(elements).find((element: HTMLElement) => isElementVisibleAndNotHidden(element));
+}

packages/utilities/src/focus.ts

@@ -358,6 +358,22 @@ export function isElementVisible(element: HTMLElement | undefined | null): boole
  ); // used as a workaround for testing.
 }
 
+/**
+ * Determines if an element is visible and not hidden
+ * @param element - Element to check
+ * @returns Returns true if the given element is visible and not hidden
+ *
+ * @public
+ */
+export function isElementVisibleAndNotHidden(element: HTMLElement | undefined | null): boolean {
+ return (
+ !!element &&
+ isElementVisible(element) &&
+ !element.hidden &&
+ window.getComputedStyle(element).visibility !== 'hidden'
+ );
+}
+
 /**
  * Determines if an element can receive focus programmatically or via a mouse click.
  * If checkTabIndex is true, additionally checks to ensure the element can be focused with the tab key,