Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/fluentui/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- fix `Tooltip` constrast style for pointing and subtle=false @yuanboxue-amber ([#22696](https://github.com/microsoft/fluentui/pull/22696))
- Vertical menu background color should not change on focus in dark theme @yuanboxue-amber ([#22707](https://github.com/microsoft/fluentui/pull/22707))
- Align `default` scheme colors between v0 and v9 @jurokapsiar ([#22699](https://github.com/microsoft/fluentui/pull/22699))
- Fix `Dialog` to keep it open when press click on content and release outside @yuanboxue-amber ([#22849](https://github.com/microsoft/fluentui/pull/22849))
- Align Avatar `font-weight` between v0 and v9 @annabratseiko ([#22822](https://github.com/microsoft/fluentui/pull/22822))
- Fix `Pill` to be selectable by keyboard @chpalac ([#22839](https://github.com/microsoft/fluentui/pull/22839))

Expand Down
18 changes: 18 additions & 0 deletions packages/fluentui/e2e/tests/dialog-example.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import * as React from 'react';
import { Button, Dialog } from '@fluentui/react-northstar';

export const selectors = {
trigger: 'trigger',
cancelButton: 'cancelButton',
};

const DialogBlockBodyScrollExample = () => (
<Dialog
cancelButton={{ content: 'Close', id: selectors.cancelButton }}
content={'dialog content'}
header="Action confirmation"
trigger={<Button id={selectors.trigger} content="Open a dialog" />}
/>
);

export default DialogBlockBodyScrollExample;
50 changes: 50 additions & 0 deletions packages/fluentui/e2e/tests/dialog.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import { selectors } from './dialog-example';

describe('Dialog', () => {
const trigger = `#${selectors.trigger}`;
const cancelButton = `#${selectors.cancelButton}`;

beforeEach(() => {
cy.gotoTestCase(__filename, trigger);
cy.get('body').click('bottomRight');
});

it('should open on click trigger', () => {
cy.clickOn(trigger);
cy.visible(cancelButton);
});

it('should close on click cancel button', () => {
cy.clickOn(trigger);
cy.visible(cancelButton);

cy.clickOn(cancelButton);
cy.notExist(cancelButton);
});

it('should close on click overlay', () => {
cy.clickOn(trigger);
cy.visible(cancelButton);

cy.get('.ui-dialog__overlay').click('topLeft');
cy.notExist(cancelButton);
});

it('should keep open when mouse down on button, and drag mouse outside of Dialog', () => {
cy.clickOn(trigger);
cy.visible(cancelButton);

// press click within Dialog content, drag mouse outside of Dialog content
cy.get(cancelButton).trigger('mousedown', { eventConstructor: 'MouseEvent', button: 0 }).trigger('mousemove', {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add some comments here wth you are trying to do

eventConstructor: 'MouseEvent',
clientX: 1,
clientY: 1,
pageX: 1,
pageY: 1,
screenX: 1,
screenY: 1,
}); // move mouse to top-left corner
cy.get('.ui-dialog__overlay').click('topLeft');
cy.visible(cancelButton);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -258,10 +258,24 @@ export const Dialog = (React.forwardRef<HTMLDivElement, DialogProps>((props, ref
},
});

// when press left click on Dialog content and hold, and mouse up on Dialog overlay, Dialog should keep open
const isMouseDownInsideContent = React.useRef(false);
const registerMouseDownOnDialogContent = (e: React.MouseEvent) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be testable in cypress by triggering a mousedown followed by a click somewhere else ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heyy, if I do

cy.trigger('mousedown').
trigger('mousemove', {clientX: 0, clientY: 0}). // move mouse to top left corner of page
trigger('mouseup')

It mimics the behavior of dragging mouse from content to outside of content. But unlike browser, cypress will not trigger an automatic click event after that.

And if I chain it with a .trigger('click'), the test would be testing "clicking outside of dialog closes it". And it lost the purpose of testing the automatic click triggered after dragging.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if I chain it with a .trigger('click'), the test would be testing "clicking outside of dialog closes it". And it lost the purpose of testing the automatic click triggered after dragging.

but if you chain it with trigger('click') the dialog should still be open right ? since there is no other way for isMouseDownInsideContenxt to be reset

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha I see what you meant. Let me try adding some tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahaa it worked ❤️ on cypress with my change chained 'click' does not close dialog

if (e.button === 0) {
isMouseDownInsideContent.current = true;
}
if (unhandledProps.onMouseDown) {
_.invoke(unhandledProps, 'onMouseDown', e);
}
};

const handleOverlayClick = (e: MouseEvent) => {
// Dialog has different conditions to close than Popup, so we don't need to iterate across all
// refs
const isInsideContentClick = doesNodeContainClick(contentRef.current, e, context.target);
const isInsideContentClick =
isMouseDownInsideContent.current || doesNodeContainClick(contentRef.current, e, context.target);
isMouseDownInsideContent.current = false;

const isInsideOverlayClick = doesNodeContainClick(overlayRef.current, e, context.target);

const shouldClose = !isInsideContentClick && isInsideOverlayClick;
Expand Down Expand Up @@ -318,6 +332,7 @@ export const Dialog = (React.forwardRef<HTMLDivElement, DialogProps>((props, ref
className: classes.root,
ref,
...unhandledProps,
onMouseDown: registerMouseDownOnDialogContent,
})}
>
{Header.create(header, {
Expand Down