Skip to content

fix(Dialog): keep Dialog open when press click on content and release on overlay #22849

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
YuanboXue-Amber merged 9 commits into from
May 10, 2022
Merged

fix(Dialog): keep Dialog open when press click on content and release on overlay #22849

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
86a73ac
check mouse down
YuanboXue-Amber May 5, 2022
c8128d4
e2e fix
YuanboXue-Amber May 5, 2022
2983599
chglog
YuanboXue-Amber May 5, 2022
d42d7de
Revert "e2e fix"
YuanboXue-Amber May 5, 2022
1f5cfa4
Merge branch 'master' into dialog-drag
YuanboXue-Amber May 6, 2022
f6cfba3
e2e
YuanboXue-Amber May 9, 2022
66f5ab9
Merge branch 'dialog-drag' of https://github.com/YuanboXue-Amber/flue…
YuanboXue-Amber May 9, 2022
3a10561
Merge remote-tracking branch 'upstream/master' into dialog-drag
YuanboXue-Amber May 9, 2022
d7a27df
comments on test
YuanboXue-Amber May 10, 2022
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
View file
Open in desktop
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))

### Documentation
- Use https for Embed and Video examples @Hirse ([#22738](https://github.com/microsoft/fluentui/pull/22738))
Expand Down
17 changes: 16 additions & 1 deletion packages/fluentui/react-northstar/src/components/Dialog/Dialog.tsx
View file
Open in desktop
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) => {

@ling1726 ling1726 May 5, 2022

Copy link
Copy Markdown
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 ?

@YuanboXue-Amber YuanboXue-Amber May 6, 2022

Copy link
Copy Markdown
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.

@ling1726 ling1726 May 9, 2022

Copy link
Copy Markdown
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

@YuanboXue-Amber YuanboXue-Amber May 9, 2022

Copy link
Copy Markdown
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

@YuanboXue-Amber YuanboXue-Amber May 9, 2022

Copy link
Copy Markdown
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