From 42315bf9bdc7810cd0fba0cc77ecb975d95ea86e Mon Sep 17 00:00:00 2001 From: jspurlin Date: Mon, 1 Nov 2021 17:06:17 -0700 Subject: [PATCH] Jspurlin/update keytip functionality (#20396) * Make callouts aware of the WindowSegments * Revert "Make callouts aware of the WindowSegments" This reverts commit 68fffc2aadfbcbf09fbcac09b51926af124f6f2e. * 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 | 18 +++++-- ...-9c097d21-6944-4b2a-b35f-09bf82daa408.json | 7 +++ ...-dfe0019e-0dd3-4f0b-b462-ffd253a88cfb.json | 7 +++ packages/react/etc/react.api.md | 1 + .../react/src/components/Keytip/Keytip.tsx | 12 ++++- .../src/components/Keytip/Keytip.types.ts | 6 +++ .../components/KeytipLayer/IKeytipTreeNode.ts | 6 +++ .../KeytipLayer/KeytipLayer.test.tsx | 10 ++++ .../src/components/KeytipLayer/KeytipTree.ts | 51 +++++++++++++++++-- .../components/OverflowSet/OverflowButton.tsx | 1 + packages/utilities/etc/utilities.api.md | 6 +++ packages/utilities/src/dom.ts | 1 + .../dom/getFirstVisibleElementFromSelector.ts | 16 ++++++ packages/utilities/src/focus.ts | 16 ++++++ 14 files changed, 149 insertions(+), 9 deletions(-) create mode 100644 change/@fluentui-react-9c097d21-6944-4b2a-b35f-09bf82daa408.json create mode 100644 change/@fluentui-utilities-dfe0019e-0dd3-4f0b-b462-ffd253a88cfb.json create mode 100644 packages/utilities/src/dom/getFirstVisibleElementFromSelector.ts diff --git a/apps/vr-tests/src/stories/Keytip.stories.tsx b/apps/vr-tests/src/stories/Keytip.stories.tsx index f731e649034ea..811add2033990 100644 --- a/apps/vr-tests/src/stories/Keytip.stories.tsx +++ b/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 => (
- + text {story()}
)) @@ -22,10 +22,18 @@ storiesOf('Keytip', module) {story()} , ) - .addStory('Root', () => ) + .addStory('Root', () => ( + + + + )) .addStory('Disabled', () => ( - + + + )) .addStory('Offset', () => ( - + + + )); diff --git a/change/@fluentui-react-9c097d21-6944-4b2a-b35f-09bf82daa408.json b/change/@fluentui-react-9c097d21-6944-4b2a-b35f-09bf82daa408.json new file mode 100644 index 0000000000000..89076e7e2181a --- /dev/null +++ b/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" +} diff --git a/change/@fluentui-utilities-dfe0019e-0dd3-4f0b-b462-ffd253a88cfb.json b/change/@fluentui-utilities-dfe0019e-0dd3-4f0b-b462-ffd253a88cfb.json new file mode 100644 index 0000000000000..0fa2deec99863 --- /dev/null +++ b/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" +} diff --git a/packages/react/etc/react.api.md b/packages/react/etc/react.api.md index 3b7be175d2e65..3416c82cc46e8 100644 --- a/packages/react/etc/react.api.md +++ b/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; diff --git a/packages/react/src/components/Keytip/Keytip.tsx b/packages/react/src/components/Keytip/Keytip.tsx index 1ab6348b66311..c4975d447d45a 100644 --- a/packages/react/src/components/Keytip/Keytip.tsx +++ b/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,7 +16,7 @@ export class Keytip extends React.Component { 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)); @@ -22,6 +24,14 @@ export class Keytip extends React.Component { 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 diff --git a/packages/react/src/components/Keytip/Keytip.types.ts b/packages/react/src/components/Keytip/Keytip.types.ts index c57c2d245e8ed..d8dcfd0a49ad7 100644 --- a/packages/react/src/components/Keytip/Keytip.types.ts +++ b/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; } /** diff --git a/packages/react/src/components/KeytipLayer/IKeytipTreeNode.ts b/packages/react/src/components/KeytipLayer/IKeytipTreeNode.ts index d1e510aead1c6..138089354d6de 100644 --- a/packages/react/src/components/KeytipLayer/IKeytipTreeNode.ts +++ b/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; } diff --git a/packages/react/src/components/KeytipLayer/KeytipLayer.test.tsx b/packages/react/src/components/KeytipLayer/KeytipLayer.test.tsx index 21bceece4cde9..79cea63c9a673 100644 --- a/packages/react/src/components/KeytipLayer/KeytipLayer.test.tsx +++ b/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(); + }); }); }); diff --git a/packages/react/src/components/KeytipLayer/KeytipTree.ts b/packages/react/src/components/KeytipLayer/KeytipTree.ts index 55571661f94b0..42d8f922df505 100644 --- a/packages/react/src/components/KeytipLayer/KeytipTree.ts +++ b/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) { diff --git a/packages/react/src/components/OverflowSet/OverflowButton.tsx b/packages/react/src/components/OverflowSet/OverflowButton.tsx index 7f8f1e62d5e0c..f6cecf94cc85b 100644 --- a/packages/react/src/components/OverflowSet/OverflowButton.tsx +++ b/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; diff --git a/packages/utilities/etc/utilities.api.md b/packages/utilities/etc/utilities.api.md index ad51ee4ca8491..69943dba909d6 100644 --- a/packages/utilities/etc/utilities.api.md +++ b/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 diff --git a/packages/utilities/src/dom.ts b/packages/utilities/src/dom.ts index 2f71e50367b86..7b75c8b966af6 100644 --- a/packages/utilities/src/dom.ts +++ b/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'; diff --git a/packages/utilities/src/dom/getFirstVisibleElementFromSelector.ts b/packages/utilities/src/dom/getFirstVisibleElementFromSelector.ts new file mode 100644 index 0000000000000..253b0f16116db --- /dev/null +++ b/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)); +} diff --git a/packages/utilities/src/focus.ts b/packages/utilities/src/focus.ts index 790bf0e4daeae..2df46b3fa29b3 100644 --- a/packages/utilities/src/focus.ts +++ b/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,