From e3b4f7422ce753e73e1a7b1ca65dc26d18fc5314 Mon Sep 17 00:00:00 2001 From: YuanboXue-Amber Date: Mon, 4 Sep 2023 18:15:43 +0200 Subject: [PATCH 1/5] fix --- .../react-utilities/src/hooks/useOnClickOutside.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/react-components/react-utilities/src/hooks/useOnClickOutside.ts b/packages/react-components/react-utilities/src/hooks/useOnClickOutside.ts index ff01345078de2..1d8ddf105952c 100644 --- a/packages/react-components/react-utilities/src/hooks/useOnClickOutside.ts +++ b/packages/react-components/react-utilities/src/hooks/useOnClickOutside.ts @@ -72,6 +72,14 @@ export const useOnClickOutside = (options: UseOnClickOrScrollOutsideOptions) => isMouseDownInsideRef.current = refs.some(ref => contains(ref.current || null, ev.target as HTMLElement)); }); + React.useEffect(() => { + refs.every(ref => ref.current?.addEventListener('mousedown', handleMouseDown, true)); + + return () => { + refs.every(ref => ref.current?.removeEventListener('mousedown', handleMouseDown, true)); + }; + }, [handleMouseDown, refs]); + React.useEffect(() => { if (disabled) { return; @@ -95,7 +103,6 @@ export const useOnClickOutside = (options: UseOnClickOrScrollOutsideOptions) => // use capture phase because React can update DOM before the event bubbles to the document element?.addEventListener('touchstart', conditionalHandler, true); element?.addEventListener('mouseup', conditionalHandler, true); - element?.addEventListener('mousedown', handleMouseDown, true); // Garbage collect this event after it's no longer useful to avoid memory leaks timeoutId.current = window.setTimeout(() => { @@ -105,7 +112,6 @@ export const useOnClickOutside = (options: UseOnClickOrScrollOutsideOptions) => return () => { element?.removeEventListener('touchstart', conditionalHandler, true); element?.removeEventListener('mouseup', conditionalHandler, true); - element?.removeEventListener('mousedown', handleMouseDown, true); clearTimeout(timeoutId.current); currentEvent = undefined; From bf08f2b11b69e0dcdcee5d886caf995e453258b6 Mon Sep 17 00:00:00 2001 From: YuanboXue-Amber Date: Mon, 4 Sep 2023 18:54:14 +0200 Subject: [PATCH 2/5] fix --- .../src/hooks/useOnClickOutside.cy.tsx | 51 ++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/packages/react-components/react-utilities/src/hooks/useOnClickOutside.cy.tsx b/packages/react-components/react-utilities/src/hooks/useOnClickOutside.cy.tsx index a8bb7abc798e6..5ddba19405309 100644 --- a/packages/react-components/react-utilities/src/hooks/useOnClickOutside.cy.tsx +++ b/packages/react-components/react-utilities/src/hooks/useOnClickOutside.cy.tsx @@ -5,7 +5,10 @@ import Frame from 'react-frame-component'; import { useOnClickOutside } from './useOnClickOutside'; -const OutsideClickExample: React.FC<{ useShadowDOM: boolean; onOutsideClick: () => void }> = props => { +const OutsideClickExample: React.FC<{ + useShadowDOM: boolean; + onOutsideClick: () => void; +}> = props => { const innerRef = React.useRef(null); useOnClickOutside({ @@ -146,6 +149,52 @@ describe('useOnClickOutside', () => { }); }); + it('should not call callback when right-clicking on trigger and mouseup outside of trigger', () => { + // mimic popover that opens on context. + // `useOnClickOutside` is only enabled when popover is open. + // When right click mouse down happens in the trigger, a contextmenu event is immediately fired and `useOnClickOutside` is enabled. + // If the mouseup happens outside, it should not call the callback. + const onOutsideClick = cy.spy(); + + const TestComponent = () => { + const [open, setOpen] = React.useState(false); + const toggleOpen: React.MouseEventHandler = e => { + e.preventDefault(); + setOpen(v => !v); + }; + const triggerRef = React.useRef(null); + const popoverRef = React.useRef(null); + useOnClickOutside({ + element: document, + callback: onOutsideClick, + refs: [triggerRef, popoverRef], + disabled: !open, + }); + + return ( + <> + + {open &&
popover
} + + + ); + }; + mount(); + + cy.get('#context-trigger') + .trigger('mousedown', { which: 3 }) + .trigger('contextmenu') + .trigger('mousemove') + .get('#outside-button') + .trigger('mousemove') + .trigger('mouseup') + .then(() => { + expect(onOutsideClick).to.not.be.called; + }); + }); + describe('iframes', () => { beforeEach(() => { cy.clock(new Date(), ['setInterval']); From 37494a67efcf61b6e993d2972082d06eaee395d1 Mon Sep 17 00:00:00 2001 From: YuanboXue-Amber Date: Mon, 4 Sep 2023 19:05:41 +0200 Subject: [PATCH 3/5] chg --- ...act-utilities-0b20e4dd-38cf-4fc3-a80a-b50b0879a22b.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@fluentui-react-utilities-0b20e4dd-38cf-4fc3-a80a-b50b0879a22b.json diff --git a/change/@fluentui-react-utilities-0b20e4dd-38cf-4fc3-a80a-b50b0879a22b.json b/change/@fluentui-react-utilities-0b20e4dd-38cf-4fc3-a80a-b50b0879a22b.json new file mode 100644 index 0000000000000..b8700121cd669 --- /dev/null +++ b/change/@fluentui-react-utilities-0b20e4dd-38cf-4fc3-a80a-b50b0879a22b.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "fix: `useOnClickOutside` checks if mouseDown happens inside regardless of `disable` option", + "packageName": "@fluentui/react-utilities", + "email": "yuanboxue@microsoft.com", + "dependentChangeType": "patch" +} From 0e479520b3e8f9f5ec799e18fef60de9dd682af6 Mon Sep 17 00:00:00 2001 From: YuanboXue-Amber Date: Mon, 4 Sep 2023 19:41:02 +0200 Subject: [PATCH 4/5] test --- .../src/hooks/useOnClickOutside.test.ts | 23 ++++++++++++++++++- .../src/hooks/useOnClickOutside.ts | 4 ++-- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/packages/react-components/react-utilities/src/hooks/useOnClickOutside.test.ts b/packages/react-components/react-utilities/src/hooks/useOnClickOutside.test.ts index 575ec75bb5c34..56d88b7c992c8 100644 --- a/packages/react-components/react-utilities/src/hooks/useOnClickOutside.test.ts +++ b/packages/react-components/react-utilities/src/hooks/useOnClickOutside.test.ts @@ -6,7 +6,8 @@ describe('useOnClickOutside', () => { jest.useRealTimers(); }); - const supportedEvents = ['mouseup', 'touchstart', 'fuiframefocus', 'mousedown']; + const supportedEvents = ['mouseup', 'touchstart', 'fuiframefocus']; + const supportedEventsWhenDisabled = ['mousedown']; it.each(supportedEvents)('should add %s listener', event => { // Arrange @@ -33,6 +34,26 @@ describe('useOnClickOutside', () => { expect(element.removeEventListener).toHaveBeenCalledWith(event, expect.anything(), true); }); + it.each(supportedEventsWhenDisabled)('should add and cleanup %s listener when disabled', event => { + // Arrange + const element = { addEventListener: jest.fn(), removeEventListener: jest.fn() } as unknown as Document; + const refs = [ + { current: { addEventListener: jest.fn(), removeEventListener: jest.fn() } as unknown as HTMLElement }, + { current: { addEventListener: jest.fn(), removeEventListener: jest.fn() } as unknown as HTMLElement }, + ]; + + // Act + const { unmount } = renderHook(() => useOnClickOutside({ disabled: true, element, callback: jest.fn(), refs })); + unmount(); + + // Assert + expect(element.addEventListener).toHaveBeenCalledTimes(0); + refs.forEach(ref => { + expect(ref.current.addEventListener).toHaveBeenCalledWith(event, expect.anything(), true); + expect(ref.current.removeEventListener).toHaveBeenCalledWith(event, expect.anything(), true); + }); + }); + it('should not add or remove event listeners when disabled', () => { // Arrange const element = { addEventListener: jest.fn(), removeEventListener: jest.fn() } as unknown as Document; diff --git a/packages/react-components/react-utilities/src/hooks/useOnClickOutside.ts b/packages/react-components/react-utilities/src/hooks/useOnClickOutside.ts index 1d8ddf105952c..2f9810c89e0ee 100644 --- a/packages/react-components/react-utilities/src/hooks/useOnClickOutside.ts +++ b/packages/react-components/react-utilities/src/hooks/useOnClickOutside.ts @@ -73,10 +73,10 @@ export const useOnClickOutside = (options: UseOnClickOrScrollOutsideOptions) => }); React.useEffect(() => { - refs.every(ref => ref.current?.addEventListener('mousedown', handleMouseDown, true)); + refs.forEach(ref => ref.current?.addEventListener('mousedown', handleMouseDown, true)); return () => { - refs.every(ref => ref.current?.removeEventListener('mousedown', handleMouseDown, true)); + refs.forEach(ref => ref.current?.removeEventListener('mousedown', handleMouseDown, true)); }; }, [handleMouseDown, refs]); From ce5953ab82a8c6c05aeab0c17e610101b8e8bba9 Mon Sep 17 00:00:00 2001 From: YuanboXue-Amber Date: Tue, 5 Sep 2023 12:13:26 +0200 Subject: [PATCH 5/5] test ci --- .../src/components/Popover/Popover.cy.tsx | 530 +----------------- 1 file changed, 14 insertions(+), 516 deletions(-) diff --git a/packages/react-components/react-popover/src/components/Popover/Popover.cy.tsx b/packages/react-components/react-popover/src/components/Popover/Popover.cy.tsx index 4069d2bc9e2a7..4094ec48a9512 100644 --- a/packages/react-components/react-popover/src/components/Popover/Popover.cy.tsx +++ b/packages/react-components/react-popover/src/components/Popover/Popover.cy.tsx @@ -5,540 +5,38 @@ import { FluentProvider } from '@fluentui/react-provider'; import { teamsLightTheme } from '@fluentui/react-theme'; import { Popover, PopoverTrigger, PopoverSurface } from '@fluentui/react-popover'; -import type { PopoverProps } from '@fluentui/react-popover'; const mount = (element: JSX.Element) => { mountBase({element}); }; const popoverTriggerSelector = '[aria-expanded]'; const popoverContentSelector = '[role="group"]'; -const popoverInteractiveContentSelector = '[role="dialog"]'; describe('Popover', () => { - ['uncontrolled', 'controlled'].forEach(scenario => { - const UncontrolledExample = () => ( - - - - - This is a popover - - ); + const UncontrolledExample = () => ( + + + + + This is a popover + + ); + + describe('uncontrolled', () => { + const Example = UncontrolledExample; - const ControlledExample = () => { - const [open, setOpen] = React.useState(false); - - return ( - setOpen(data.open)}> - - - - This is a popover - - ); - }; - - describe(scenario, () => { - const Example = scenario === 'controlled' ? ControlledExample : UncontrolledExample; - - beforeEach(() => { - mount(); - cy.get('body').click('bottomRight'); - }); - - it('should open when clicked', () => { - cy.get(popoverTriggerSelector).click().get(popoverContentSelector).should('be.visible'); - }); - - (['{enter}', 'Space'] as const).forEach((key: '{enter}' | 'Space') => { - it(`should open with ${key}`, () => { - cy.get(popoverTriggerSelector).focus().realPress(key); - - cy.get(popoverContentSelector).should('be.visible'); - }); - }); - - it('should dismiss on click outside', () => { - cy.get(popoverTriggerSelector) - .click() - .get('body') - .click('bottomRight') - .get(popoverContentSelector) - .should('not.exist'); - }); - - it('should dismiss on Escape keydown', () => { - cy.get(popoverTriggerSelector).click().realPress('Escape'); - cy.get(popoverContentSelector).should('not.exist'); - }); - - it('should keep open state on scroll outside', () => { - cy.get(popoverTriggerSelector).click().get(popoverContentSelector).should('be.visible'); - cy.get('body').trigger('wheel').get(popoverContentSelector).should('be.visible'); - }); - }); - }); - - describe('Open on hover', () => { beforeEach(() => { - mount( - - - - - This is a popover - , - ); + mount(); cy.get('body').click('bottomRight'); }); - it('should open on hover, and keep open on mouse move to content', () => { - cy.get(popoverTriggerSelector).trigger('mouseover').get(popoverContentSelector).should('be.visible'); - cy.get(popoverContentSelector).trigger('mouseover').get(popoverContentSelector).should('be.visible'); - }); - }); - - describe('With custom trigger', () => { - const CustomTrigger = React.forwardRef((props, ref) => { - return ( - - ); - }); - it('should dismiss on click outside', () => { - mount( - - - - - This is a popover - , - ); - cy.get(popoverTriggerSelector).get('body').click('bottomRight').get(popoverContentSelector).should('not.exist'); - }); - }); - - describe('Context popover', () => { - beforeEach(() => { - mount( - - - - - This is a popover - , - ); - cy.get('body').click('bottomRight'); - }); - - it('should open when right clicked', () => { - cy.get(popoverTriggerSelector).rightclick().get(popoverContentSelector).should('be.visible'); - }); - - it('should dismiss on scroll outside', () => { cy.get(popoverTriggerSelector) - .rightclick() + .click() .get('body') - .trigger('wheel') + .click('bottomRight') .get(popoverContentSelector) .should('not.exist'); }); }); - - describe('popover with closeOnScroll', () => { - beforeEach(() => { - mount( - - - - - This is a popover - , - ); - cy.get('body').click('bottomRight'); - }); - - it('should dismiss on scroll outside', () => { - cy.get(popoverTriggerSelector).click().get(popoverContentSelector).should('be.visible'); - cy.get('body').trigger('wheel').get(popoverContentSelector).should('not.exist'); - }); - }); - - describe('Nested', () => { - const PopoverL1 = () => { - const id = 'first'; - return ( - - - - - - - - - - - - ); - }; - - const PopoverL2 = () => { - const id = 'second'; - return ( - - - - - - - - - - ); - }; - - const Example = () => { - return ( - - - - - - - - - - - ); - }; - - beforeEach(() => { - mount(); - // Open the whole stack of popovers - cy.contains('Root').click().get('body').contains('First').click().get('body').contains('Second').first().click(); - }); - - it('should trap focus with tab', () => { - cy.focused().then(beforeFocused => { - cy.focused().realPress('Tab'); - cy.realPress(['Shift', 'Tab']); - cy.focused().then(afterFocused => { - expect(beforeFocused[0]).eq(afterFocused[0]); - }); - }); - }); - - it('should trap focus with shift+tab', () => { - cy.focused().then(beforeFocused => { - cy.focused().realPress('Tab'); - cy.realPress(['Shift', 'Tab']); - cy.focused().then(afterFocused => { - expect(beforeFocused[0]).eq(afterFocused[0]); - }); - }); - }); - - it('should dismiss all nested popovers on outside click', () => { - cy.get('body').click('bottomRight').get(popoverInteractiveContentSelector).should('not.exist'); - }); - - it('should not dismiss when clicking on nested content', () => { - cy.contains('Second nested button').click().get(popoverInteractiveContentSelector).should('have.length', 3); - }); - - it('should dismiss child popovers when clicking on parents', () => { - cy.contains('First nested button') - .click() - .get(popoverInteractiveContentSelector) - .should('have.length', 2) - .contains('Root button') - .click() - .get(popoverInteractiveContentSelector) - .should('have.length', 1); - }); - - it('should when opening a sibling popover, should dismiss other sibling popover', () => { - const secondNestedTriggerSelector = 'button:contains(Second nested trigger)'; - - cy.get(secondNestedTriggerSelector) - .eq(1) - .click() - .get(popoverInteractiveContentSelector) - .should('have.length', 3) - .get(secondNestedTriggerSelector) - .eq(0) - .click() - .get(popoverInteractiveContentSelector) - .should('have.length', 3); - }); - - it('should dismiss each popover in the stack with Escape keydown', () => { - cy.focused().realPress('Escape'); - cy.get(popoverInteractiveContentSelector).should('have.length', 2); - cy.focused().realPress('Escape'); - cy.get(popoverInteractiveContentSelector).should('have.length', 1); - cy.focused().realPress('Escape'); - cy.get(popoverInteractiveContentSelector).should('not.exist'); - }); - }); - - describe('updating content', () => { - const Example = () => { - const [visible, setVisible] = React.useState(false); - - const changeContent = () => setVisible(true); - const onOpenChange: PopoverProps['onOpenChange'] = (e, data) => { - if (data.open === false) { - setVisible(false); - } - }; - - return ( - - - - - - - {visible ? ( -
The second panel
- ) : ( -
- -
- )} - - - ); - }; - - it('should not close popover', () => { - mount(); - cy.get(popoverTriggerSelector) - .click() - .get(popoverContentSelector) - .within(() => { - cy.get('button').click(); - }) - .get(popoverContentSelector) - .should('exist'); - }); - }); - - describe('with inline prop', () => { - it('should render PopoverSurface in DOM order', () => { - mount( - <> -
- - - - - - This is a Popover - -
-
Outside content
- , - ); - - cy.get(popoverTriggerSelector) - .click() - .get(popoverContentSelector) - .prev() - .then(popoverSurfacePrev => { - cy.get(popoverTriggerSelector).then(popoverTrigger => { - expect(popoverTrigger[0]).eq(popoverSurfacePrev[0]); - }); - }); - }); - - describe('legacy focus trap behaviour', () => { - it('Tab should not go to the window', () => { - mount( - - - - - - - - - - , - ); - - cy.get(popoverTriggerSelector).focus().realPress('Enter'); - - cy.contains('One').should('have.focus').realPress('Tab'); - cy.contains('Two').should('have.focus').realPress('Tab'); - cy.contains('One').should('have.focus').realPress(['Shift', 'Tab']); - cy.contains('Two').should('have.focus'); - }); - - it('should work as inertTrapFocus when set to false', () => { - mount( - - - - - - - - - - , - ); - - cy.get(popoverTriggerSelector).focus().realPress('Enter'); - - cy.contains('One').should('have.focus').realPress('Tab'); - cy.contains('Two').should('have.focus').realPress('Tab'); - cy.focused().should('not.exist'); - }); - }); - - describe('inert focus trap behaviour', () => { - it('Tab should go to the window', () => { - mount( - - - - - - - - - - , - ); - - cy.get(popoverTriggerSelector).focus().realPress('Enter'); - - cy.contains('One').should('have.focus').realPress('Tab'); - cy.contains('Two').should('have.focus').realPress('Tab'); - cy.focused().should('not.exist'); - }); - }); - - describe('trap focus behaviour', () => { - it('should focus on PopoverSurface when its tabIndex is a number', () => { - mount( - - - - - - - - - - , - ); - - cy.get(popoverTriggerSelector).focus().realPress('Enter'); - - cy.get('#popover-surface').should('have.focus').realPress('Tab'); - cy.contains('One').should('have.focus').realPress('Tab'); - cy.contains('Two').should('have.focus').realPress('Tab'); - cy.contains('One').should('have.focus').realPress(['Shift', 'Tab']); - cy.contains('Two').should('have.focus'); - }); - }); - }); - - describe('with Iframe', () => { - const iframeContent = `
- -
`; - - const ExampleFrame = () => { - return