From c277bb373ca08d293563866a217c9beb540a272b Mon Sep 17 00:00:00 2001 From: Esteban Munoz Date: Thu, 17 Aug 2023 15:18:37 -0700 Subject: [PATCH 1/6] fix: Popover should close when tabbing out of the surface. --- .../components/InfoButton/useInfoButton.tsx | 48 ++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/packages/react-components/react-infobutton/src/components/InfoButton/useInfoButton.tsx b/packages/react-components/react-infobutton/src/components/InfoButton/useInfoButton.tsx index c543d7a0b7fbf..f958f9d086895 100644 --- a/packages/react-components/react-infobutton/src/components/InfoButton/useInfoButton.tsx +++ b/packages/react-components/react-infobutton/src/components/InfoButton/useInfoButton.tsx @@ -1,7 +1,14 @@ import * as React from 'react'; import { DefaultInfoButtonIcon12, DefaultInfoButtonIcon16, DefaultInfoButtonIcon20 } from './DefaultInfoButtonIcons'; -import { getNativeElementProps, mergeCallbacks, useControllableState, slot } from '@fluentui/react-utilities'; +import { + getNativeElementProps, + mergeCallbacks, + useControllableState, + slot, + useMergedRefs, +} from '@fluentui/react-utilities'; import { Popover, PopoverSurface } from '@fluentui/react-popover'; +import { useFocusFinders } from '@fluentui/react-tabster'; import type { InfoButtonProps, InfoButtonState } from './InfoButton.types'; import type { PopoverProps } from '@fluentui/react-popover'; @@ -27,6 +34,7 @@ const popoverSizeMap = { * @param ref - reference to root HTMLElement of InfoButton */ export const useInfoButton_unstable = (props: InfoButtonProps, ref: React.Ref): InfoButtonState => { + const { findAllFocusable } = useFocusFinders(); const { size = 'medium' } = props; const state: InfoButtonState = { @@ -74,5 +82,43 @@ export const useInfoButton_unstable = (props: InfoButtonProps, ref: React.Ref setPopoverOpen(data.open)); + const closeOnTabBack = (ev: KeyboardEvent) => { + if (ev.shiftKey && ev.key === 'Tab') { + setPopoverOpen(false); + } + }; + const closeOnTabForward = (ev: KeyboardEvent) => { + if (ev.key === 'Tab') { + setPopoverOpen(false); + } + }; + + // This ref creates the event listeners needed to dismiss the popover when tabbing out of the surface + const infoTabDismissRef = (element: HTMLDivElement) => { + if (element) { + const focusableElements = findAllFocusable(element); + + if (focusableElements.length > 0) { + // We need two events, one for the surface in case the popover is opened but tabbed back right away and one for + // the first item since the surface can only be focused programmatically because of its tabIndex = -1 + element.addEventListener('keydown', closeOnTabBack); + focusableElements[0].addEventListener( + 'keydown', + focusableElements.length > 1 ? closeOnTabBack : mergeCallbacks(closeOnTabBack, closeOnTabForward), + ); + + if (focusableElements.length > 1) { + focusableElements[focusableElements.length - 1].addEventListener('keydown', closeOnTabForward); + } + } else { + element.addEventListener('blur', () => { + setPopoverOpen(false); + }); + } + } + }; + + state.info.ref = useMergedRefs(state.info.ref, infoTabDismissRef); + return state; }; From 1dc54a35257f8800003fac8e7253d9a3ee3437cb Mon Sep 17 00:00:00 2001 From: Esteban Munoz Date: Thu, 17 Aug 2023 15:19:02 -0700 Subject: [PATCH 2/6] change file: --- ...ct-infobutton-e6d4cef7-a090-4513-ac13-c1754d642d63.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@fluentui-react-infobutton-e6d4cef7-a090-4513-ac13-c1754d642d63.json diff --git a/change/@fluentui-react-infobutton-e6d4cef7-a090-4513-ac13-c1754d642d63.json b/change/@fluentui-react-infobutton-e6d4cef7-a090-4513-ac13-c1754d642d63.json new file mode 100644 index 0000000000000..4a6f683a2620d --- /dev/null +++ b/change/@fluentui-react-infobutton-e6d4cef7-a090-4513-ac13-c1754d642d63.json @@ -0,0 +1,7 @@ +{ + "type": "prerelease", + "comment": "fix: Popover should close when tabbing out of the surface.", + "packageName": "@fluentui/react-infobutton", + "email": "esteban.230@hotmail.com", + "dependentChangeType": "patch" +} From ff9668b5665ed4bdf92d02b3db4c2bce8dd09ca9 Mon Sep 17 00:00:00 2001 From: Esteban Munoz Date: Fri, 18 Aug 2023 13:26:58 -0700 Subject: [PATCH 3/6] add e2e tests --- .../react-infobutton/cypress.config.ts | 3 + .../react-infobutton/package.json | 2 + .../components/InfoButton/useInfoButton.tsx | 8 +- .../src/components/InfoLabel/InfoLabel.cy.tsx | 80 +++++++++++++++++++ .../react-infobutton/tsconfig.cy.json | 9 +++ .../react-infobutton/tsconfig.json | 3 + 6 files changed, 101 insertions(+), 4 deletions(-) create mode 100644 packages/react-components/react-infobutton/cypress.config.ts create mode 100644 packages/react-components/react-infobutton/src/components/InfoLabel/InfoLabel.cy.tsx create mode 100644 packages/react-components/react-infobutton/tsconfig.cy.json diff --git a/packages/react-components/react-infobutton/cypress.config.ts b/packages/react-components/react-infobutton/cypress.config.ts new file mode 100644 index 0000000000000..ca52cf041bbf2 --- /dev/null +++ b/packages/react-components/react-infobutton/cypress.config.ts @@ -0,0 +1,3 @@ +import { baseConfig } from '@fluentui/scripts-cypress'; + +export default baseConfig; diff --git a/packages/react-components/react-infobutton/package.json b/packages/react-components/react-infobutton/package.json index 3d73bc8d6e879..409243d23b8c2 100644 --- a/packages/react-components/react-infobutton/package.json +++ b/packages/react-components/react-infobutton/package.json @@ -23,6 +23,8 @@ "type-check": "tsc -b tsconfig.json", "storybook": "start-storybook", "start": "yarn storybook", + "e2e": "cypress run --component", + "e2e:local": "cypress open --component", "test-ssr": "test-ssr \"./stories/**/*.stories.tsx\"" }, "devDependencies": { diff --git a/packages/react-components/react-infobutton/src/components/InfoButton/useInfoButton.tsx b/packages/react-components/react-infobutton/src/components/InfoButton/useInfoButton.tsx index f958f9d086895..52d9946fe5be3 100644 --- a/packages/react-components/react-infobutton/src/components/InfoButton/useInfoButton.tsx +++ b/packages/react-components/react-infobutton/src/components/InfoButton/useInfoButton.tsx @@ -83,12 +83,14 @@ export const useInfoButton_unstable = (props: InfoButtonProps, ref: React.Ref setPopoverOpen(data.open)); const closeOnTabBack = (ev: KeyboardEvent) => { - if (ev.shiftKey && ev.key === 'Tab') { + ev.stopPropagation(); + if (ev.currentTarget === ev.target && ev.shiftKey && ev.key === 'Tab') { setPopoverOpen(false); } }; const closeOnTabForward = (ev: KeyboardEvent) => { - if (ev.key === 'Tab') { + ev.stopPropagation(); + if (ev.currentTarget === ev.target && !ev.shiftKey && ev.key === 'Tab') { setPopoverOpen(false); } }; @@ -97,7 +99,6 @@ export const useInfoButton_unstable = (props: InfoButtonProps, ref: React.Ref { if (element) { const focusableElements = findAllFocusable(element); - if (focusableElements.length > 0) { // We need two events, one for the surface in case the popover is opened but tabbed back right away and one for // the first item since the surface can only be focused programmatically because of its tabIndex = -1 @@ -106,7 +107,6 @@ export const useInfoButton_unstable = (props: InfoButtonProps, ref: React.Ref 1 ? closeOnTabBack : mergeCallbacks(closeOnTabBack, closeOnTabForward), ); - if (focusableElements.length > 1) { focusableElements[focusableElements.length - 1].addEventListener('keydown', closeOnTabForward); } diff --git a/packages/react-components/react-infobutton/src/components/InfoLabel/InfoLabel.cy.tsx b/packages/react-components/react-infobutton/src/components/InfoLabel/InfoLabel.cy.tsx new file mode 100644 index 0000000000000..33c32be48be4d --- /dev/null +++ b/packages/react-components/react-infobutton/src/components/InfoLabel/InfoLabel.cy.tsx @@ -0,0 +1,80 @@ +import * as React from 'react'; +import { mount as mountBase } from '@cypress/react'; +import { FluentProvider } from '@fluentui/react-provider'; +import { teamsLightTheme } from '@fluentui/react-theme'; +import { InfoLabel } from '@fluentui/react-infobutton'; + +const mount = (element: JSX.Element) => { + mountBase({element}); +}; + +const surfaceSelector = '[role="note"]'; + +describe('InfoLabel - close on tab-out', () => { + const openInfoButton = () => { + cy.get('button').focus().realPress('{enter}'); + }; + + it('no focusable elements', () => { + mount(); + + openInfoButton(); + cy.realPress(['Shift', 'Tab']).get(surfaceSelector).should('not.exist'); + openInfoButton(); + cy.realPress('Tab').get(surfaceSelector).should('not.exist'); + }); + + it('single focusable element', () => { + mount( + + Example non-focusable info + + + } + />, + ); + + openInfoButton(); + cy.realPress(['Shift', 'Tab']).get(surfaceSelector).should('not.exist'); + openInfoButton(); + // moving into the focusable item + cy.realPress('Tab').get(surfaceSelector).should('exist'); + // tabbing out with shift + tab from the first focusable item should close the surface since + // the surface is only focusable programmatically + cy.realPress(['Shift', 'Tab']).get(surfaceSelector).should('not.exist'); + openInfoButton(); + cy.realPress('Tab').realPress('Tab').get(surfaceSelector).should('not.exist'); + }); + + it('one or more focusable elements', () => { + mount( + + Example non-focusable info + + + + + } + />, + ); + + openInfoButton(); + cy.realPress(['Shift', 'Tab']).get(surfaceSelector).should('not.exist'); + openInfoButton(); + // moving into the focusable item + cy.realPress('Tab').get(surfaceSelector).should('exist'); + // tabbing out with shift + tab from the first focusable item should close the surface since + // the surface is only focusable programmatically + cy.realPress(['Shift', 'Tab']).get(surfaceSelector).should('not.exist'); + openInfoButton(); + // checking that event does not propagate to children + cy.realPress('Tab').realPress('Tab').realPress(['Shift', 'Tab']).get(surfaceSelector).should('exist'); + cy.realPress('Tab').realPress('Tab').realPress('Tab').get(surfaceSelector).should('not.exist'); + }); +}); diff --git a/packages/react-components/react-infobutton/tsconfig.cy.json b/packages/react-components/react-infobutton/tsconfig.cy.json new file mode 100644 index 0000000000000..93a140885851d --- /dev/null +++ b/packages/react-components/react-infobutton/tsconfig.cy.json @@ -0,0 +1,9 @@ +{ + "extends": "./tsconfig.json", + "compilerOptions": { + "isolatedModules": false, + "types": ["node", "cypress", "cypress-storybook/cypress", "cypress-real-events"], + "lib": ["ES2019", "dom"] + }, + "include": ["**/*.cy.ts", "**/*.cy.tsx"] +} diff --git a/packages/react-components/react-infobutton/tsconfig.json b/packages/react-components/react-infobutton/tsconfig.json index 1941a041d46c1..1317f81620ca5 100644 --- a/packages/react-components/react-infobutton/tsconfig.json +++ b/packages/react-components/react-infobutton/tsconfig.json @@ -20,6 +20,9 @@ }, { "path": "./.storybook/tsconfig.json" + }, + { + "path": "./tsconfig.cy.json" } ] } From 891ed5048608b0ac7c08908b6a4006024b5ee289 Mon Sep 17 00:00:00 2001 From: Esteban Munoz Date: Wed, 6 Sep 2023 13:38:16 -0700 Subject: [PATCH 4/6] use measured ref instead and simplify logic --- .../react-infobutton/package.json | 1 + .../components/InfoButton/useInfoButton.tsx | 53 ++++++------------- .../src/components/InfoLabel/InfoLabel.cy.tsx | 2 + 3 files changed, 20 insertions(+), 36 deletions(-) diff --git a/packages/react-components/react-infobutton/package.json b/packages/react-components/react-infobutton/package.json index bde20c47918e2..d5ad61c01f1ee 100644 --- a/packages/react-components/react-infobutton/package.json +++ b/packages/react-components/react-infobutton/package.json @@ -38,6 +38,7 @@ "@fluentui/react-icons": "^2.0.207", "@fluentui/react-label": "^9.1.29", "@fluentui/react-popover": "^9.7.18", + "@fluentui/react-portal": "^9.3.9", "@fluentui/react-tabster": "^9.12.4", "@fluentui/react-theme": "^9.1.11", "@fluentui/react-utilities": "^9.12.0", diff --git a/packages/react-components/react-infobutton/src/components/InfoButton/useInfoButton.tsx b/packages/react-components/react-infobutton/src/components/InfoButton/useInfoButton.tsx index 28fb78d5b9733..26839e39f0cad 100644 --- a/packages/react-components/react-infobutton/src/components/InfoButton/useInfoButton.tsx +++ b/packages/react-components/react-infobutton/src/components/InfoButton/useInfoButton.tsx @@ -6,9 +6,10 @@ import { useControllableState, slot, useMergedRefs, + isHTMLElement, } from '@fluentui/react-utilities'; +import { elementContains } from '@fluentui/react-portal'; import { Popover, PopoverSurface } from '@fluentui/react-popover'; -import { useFocusFinders } from '@fluentui/react-tabster'; import type { InfoButtonProps, InfoButtonState } from './InfoButton.types'; import type { PopoverProps } from '@fluentui/react-popover'; @@ -34,7 +35,6 @@ const popoverSizeMap = { * @param ref - reference to root HTMLElement of InfoButton */ export const useInfoButton_unstable = (props: InfoButtonProps, ref: React.Ref): InfoButtonState => { - const { findAllFocusable } = useFocusFinders(); const { size = 'medium', inline = true } = props; const state: InfoButtonState = { @@ -84,43 +84,24 @@ export const useInfoButton_unstable = (props: InfoButtonProps, ref: React.Ref setPopoverOpen(data.open)); - const closeOnTabBack = (ev: KeyboardEvent) => { - ev.stopPropagation(); - if (ev.currentTarget === ev.target && ev.shiftKey && ev.key === 'Tab') { - setPopoverOpen(false); - } - }; - const closeOnTabForward = (ev: KeyboardEvent) => { - ev.stopPropagation(); - if (ev.currentTarget === ev.target && !ev.shiftKey && ev.key === 'Tab') { - setPopoverOpen(false); - } - }; + const focusOutRef = React.useCallback( + (el: HTMLDivElement) => { + if (!el) { + return; + } - // This ref creates the event listeners needed to dismiss the popover when tabbing out of the surface - const infoTabDismissRef = (element: HTMLDivElement) => { - if (element) { - const focusableElements = findAllFocusable(element); - if (focusableElements.length > 0) { - // We need two events, one for the surface in case the popover is opened but tabbed back right away and one for - // the first item since the surface can only be focused programmatically because of its tabIndex = -1 - element.addEventListener('keydown', closeOnTabBack); - focusableElements[0].addEventListener( - 'keydown', - focusableElements.length > 1 ? closeOnTabBack : mergeCallbacks(closeOnTabBack, closeOnTabForward), - ); - if (focusableElements.length > 1) { - focusableElements[focusableElements.length - 1].addEventListener('keydown', closeOnTabForward); - } - } else { - element.addEventListener('blur', () => { + el.addEventListener('focusout', e => { + const nextFocused = e.relatedTarget; + + if (isHTMLElement(nextFocused) && !elementContains(el, nextFocused)) { setPopoverOpen(false); - }); - } - } - }; + } + }); + }, + [setPopoverOpen], + ); - state.info.ref = useMergedRefs(state.info.ref, infoTabDismissRef); + state.info.ref = useMergedRefs(state.info.ref, focusOutRef); return state; }; diff --git a/packages/react-components/react-infobutton/src/components/InfoLabel/InfoLabel.cy.tsx b/packages/react-components/react-infobutton/src/components/InfoLabel/InfoLabel.cy.tsx index 33c32be48be4d..f251f54f816da 100644 --- a/packages/react-components/react-infobutton/src/components/InfoLabel/InfoLabel.cy.tsx +++ b/packages/react-components/react-infobutton/src/components/InfoLabel/InfoLabel.cy.tsx @@ -1,3 +1,5 @@ +/// + import * as React from 'react'; import { mount as mountBase } from '@cypress/react'; import { FluentProvider } from '@fluentui/react-provider'; From 9206b531b8d2bf4b300eb7d135e048f24e9ad9ad Mon Sep 17 00:00:00 2001 From: Esteban Munoz Date: Wed, 6 Sep 2023 14:05:04 -0700 Subject: [PATCH 5/6] fixing deps --- packages/react-components/react-infobutton/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-components/react-infobutton/package.json b/packages/react-components/react-infobutton/package.json index b5f69c06c6701..47c0058cc63ac 100644 --- a/packages/react-components/react-infobutton/package.json +++ b/packages/react-components/react-infobutton/package.json @@ -38,7 +38,7 @@ "@fluentui/react-icons": "^2.0.207", "@fluentui/react-label": "^9.1.33", "@fluentui/react-popover": "^9.8.3", - "@fluentui/react-portal": "^9.3.9", + "@fluentui/react-portal": "^9.3.13", "@fluentui/react-tabster": "^9.12.8", "@fluentui/react-theme": "^9.1.12", "@fluentui/react-utilities": "^9.13.3", From e2a7a64149216f9328ffd3503ea5067a9cfad73a Mon Sep 17 00:00:00 2001 From: Esteban Munoz Date: Wed, 6 Sep 2023 14:06:03 -0700 Subject: [PATCH 6/6] reverting charting --- .../src/components/AreaChart/AreaChart.base.tsx | 6 +++--- .../HorizontalBarChart.base.tsx | 2 +- .../components/LineChart/LineChartRTL.test.tsx | 2 +- .../MultiStackedBarChart.base.tsx | 16 ++++++++-------- .../StackedBarChart/StackedBarChart.base.tsx | 8 ++++---- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/packages/react-charting/src/components/AreaChart/AreaChart.base.tsx b/packages/react-charting/src/components/AreaChart/AreaChart.base.tsx index 22adbd700a7e5..40209941efafa 100644 --- a/packages/react-charting/src/components/AreaChart/AreaChart.base.tsx +++ b/packages/react-charting/src/components/AreaChart/AreaChart.base.tsx @@ -293,7 +293,7 @@ export class AreaChartBase extends React.Component { this._onLegendClick(singleChartData.legend); }, diff --git a/packages/react-charting/src/components/HorizontalBarChart/HorizontalBarChart.base.tsx b/packages/react-charting/src/components/HorizontalBarChart/HorizontalBarChart.base.tsx index 82b83cac3919d..72f3f627659fe 100644 --- a/packages/react-charting/src/components/HorizontalBarChart/HorizontalBarChart.base.tsx +++ b/packages/react-charting/src/components/HorizontalBarChart/HorizontalBarChart.base.tsx @@ -199,7 +199,7 @@ export class HorizontalBarChartBase extends React.Component { testWithWait( 'Should render events with defined data', LineChart, - { data: simplePoints, eventAnnotationProps, tickValues, tickFormat: '%m/%d' }, + { data: simplePoints, eventAnnotationProps: eventAnnotationProps, tickValues: tickValues, tickFormat: '%m/%d' }, container => { // Arrange const event = screen.queryByText('3 events'); diff --git a/packages/react-charting/src/components/StackedBarChart/MultiStackedBarChart.base.tsx b/packages/react-charting/src/components/StackedBarChart/MultiStackedBarChart.base.tsx index 27857d3df1d19..944b857a1c50c 100644 --- a/packages/react-charting/src/components/StackedBarChart/MultiStackedBarChart.base.tsx +++ b/packages/react-charting/src/components/StackedBarChart/MultiStackedBarChart.base.tsx @@ -267,8 +267,8 @@ export class MultiStackedBarChartBase extends React.Component { this._onClick(point.legend!); }, @@ -495,7 +495,7 @@ export class MultiStackedBarChartBase extends React.Component { this._onClick(point.legend!); }, @@ -556,7 +556,7 @@ export class MultiStackedBarChartBase extends React.Component 0 ? () => { @@ -316,7 +316,7 @@ export class StackedBarChartBase extends React.Component