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"
+}
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 ;
- };
-
- it('should close when focus is on an external iframe', () => {
- mount(
- <>
-
-
-
-
-
-
-
- This is a popover
-
- ,
- );
-
- cy.get(popoverTriggerSelector).click().get('iframe').focus().get(popoverContentSelector).should('not.exist');
- });
-
- it('should not close when focus is on an internal iframe', () => {
- mount(
- <>
-
-
-
-
-
-
-
-
-
- ,
- );
-
- cy.get(popoverTriggerSelector)
- .click()
- .get('iframe')
- .focus()
- // wait is generally bad practice but since the iframe focus
- // detection works through polling, set a value here where the
- // the event would definitely dispatch
- .wait(2000)
- .get(popoverContentSelector)
- .should('exist');
- });
-
- it('should not close when focus is on an internal iframe in a nested popover', () => {
- mount(
- <>
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- ,
- );
-
- cy.get(popoverTriggerSelector)
- .first()
- .click()
- .get(popoverTriggerSelector)
- .eq(1)
- .click()
- .get('iframe')
- .focus()
- // wait is generally bad practice but since the iframe focus
- // detection works through polling, set a value here where the
- // the event would definitely dispatch
- .wait(2000)
- .get(popoverContentSelector)
- .should('have.length', 2);
- });
- });
});
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']);
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 ff01345078de2..2f9810c89e0ee 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.forEach(ref => ref.current?.addEventListener('mousedown', handleMouseDown, true));
+
+ return () => {
+ refs.forEach(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;