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

YuanboXue-Amber · 2022-05-05T09:49:02Z

Current Behavior

When a Dialog is opened, press click on content and release outside of Dialog will close the Dialog.

New Behavior

Press click on Dialog content and release outside of it will not close the Dialog.

Fixes:
According to click event definition:

If the button is pressed on one element and the pointer is moved outside the element before the button is released, the event is fired on the most specific ancestor element that contained both elements.

press mouse on dialog content and release outside will cause a click event to be fired on dialog overlay.
Events fired during this process: mousedown on content -> mousemove -> mouseup on dialog overlay -> click on dialog overlay.
The fix is to use a ref to register mousedown on content, and on click event handler, check this ref to decide if dialog should be closed.

e2e tests was added

codesandbox-ci · 2022-05-05T09:55:46Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit d7a27df:

Sandbox	Source
@fluentui/react 8 starter	Configuration
@fluentui/react-components 9 starter	Configuration

size-auditor · 2022-05-05T10:00:38Z

Asset size changes

Project	Bundle	Baseline Size	New Size	Difference
office-ui-fabric-react	fluentui-react-northstar-Dialog	124.691 kB	124.83 kB	139 bytes

Over Tolerance (1024 B) Over Baseline Below Baseline New Removed 1 kB = 1000 B

Baseline commit: 8f40f25712a38b3b3121e7eb71623ce3d5a187e1 (build)

fabricteam · 2022-05-05T10:03:33Z

📊 Bundle size report

_{🤖 This report was generated against 8f40f25712a38b3b3121e7eb71623ce3d5a187e1}

fabricteam · 2022-05-05T10:07:27Z

Perf Analysis (`@fluentui/react-northstar`)

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
GridMinimalPerf.default	350	314	1.11:1
HeaderMinimalPerf.default	377	340	1.11:1
DropdownManyItemsPerf.default	725	662	1.1:1
SkeletonMinimalPerf.default	357	326	1.1:1
LayoutMinimalPerf.default	382	349	1.09:1
CardMinimalPerf.default	586	546	1.07:1
DividerMinimalPerf.default	363	339	1.07:1
ListMinimalPerf.default	559	523	1.07:1
RadioGroupMinimalPerf.default	471	442	1.07:1
AccordionMinimalPerf.default	147	139	1.06:1
AttachmentSlotsPerf.default	1110	1056	1.05:1
FormMinimalPerf.default	426	407	1.05:1
InputMinimalPerf.default	1390	1325	1.05:1
PopupMinimalPerf.default	650	618	1.05:1
BoxMinimalPerf.default	343	330	1.04:1
FlexMinimalPerf.default	310	297	1.04:1
MenuButtonMinimalPerf.default	1737	1665	1.04:1
IconMinimalPerf.default	662	636	1.04:1
DropdownMinimalPerf.default	3209	3128	1.03:1
TextAreaMinimalPerf.default	488	474	1.03:1
AttachmentMinimalPerf.default	155	152	1.02:1
ListCommonPerf.default	658	647	1.02:1
ListNestedPerf.default	561	549	1.02:1
StatusMinimalPerf.default	710	697	1.02:1
TableMinimalPerf.default	398	391	1.02:1
ButtonSlotsPerf.default	590	585	1.01:1
DialogMinimalPerf.default	782	772	1.01:1
SliderMinimalPerf.default	1708	1686	1.01:1
SplitButtonMinimalPerf.default	4618	4582	1.01:1
TableManyItemsPerf.default	1971	1946	1.01:1
TextMinimalPerf.default	346	343	1.01:1
CustomToolbarPrototype.default	2768	2754	1.01:1
TooltipMinimalPerf.default	1074	1068	1.01:1
VideoMinimalPerf.default	647	639	1.01:1
AvatarMinimalPerf.default	195	195	1:1
CheckboxMinimalPerf.default	2804	2816	1:1
EmbedMinimalPerf.default	4346	4335	1:1
MenuMinimalPerf.default	862	858	1:1
TreeWith60ListItems.default	169	169	1:1
ChatWithPopoverPerf.default	370	375	0.99:1
DatepickerMinimalPerf.default	5959	6037	0.99:1
ListWith60ListItems.default	671	675	0.99:1
PortalMinimalPerf.default	167	168	0.99:1
ProviderMinimalPerf.default	400	406	0.99:1
ButtonOverridesMissPerf.default	1512	1548	0.98:1
CarouselMinimalPerf.default	466	476	0.98:1
RosterPerf.default	1137	1163	0.98:1
ProviderMergeThemesPerf.default	1289	1316	0.98:1
ImageMinimalPerf.default	368	381	0.97:1
ToolbarMinimalPerf.default	940	970	0.97:1
AnimationMinimalPerf.default	548	569	0.96:1
ItemLayoutMinimalPerf.default	1190	1237	0.96:1
LoaderMinimalPerf.default	680	707	0.96:1
RefMinimalPerf.default	238	249	0.96:1
SegmentMinimalPerf.default	343	358	0.96:1
ButtonMinimalPerf.default	165	174	0.95:1
ChatMinimalPerf.default	747	793	0.94:1
ReactionMinimalPerf.default	357	380	0.94:1
TreeMinimalPerf.default	794	849	0.94:1
AlertMinimalPerf.default	251	269	0.93:1
HeaderSlotsPerf.default	736	795	0.93:1
LabelMinimalPerf.default	374	405	0.92:1
ChatDuplicateMessagesPerf.default	281	314	0.89:1

This reverts commit c8128d4.

ling1726 · 2022-05-05T14:34:05Z

packages/fluentui/react-northstar/src/components/Dialog/Dialog.tsx


+  // 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) => {


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

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.

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

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

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

…ntui into dialog-drag

ling1726 · 2022-05-09T15:57:04Z

packages/fluentui/e2e/tests/dialog.spec.ts

+    cy.clickOn(trigger);
+    cy.visible(cancelButton);
+
+    cy.get(cancelButton).trigger('mousedown', { eventConstructor: 'MouseEvent', button: 0 }).trigger('mousemove', {


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

… on overlay (microsoft#22849) * check mouse down * e2e fix * chglog * Revert "e2e fix" This reverts commit c8128d4. * e2e * comments on test

check mouse down

86a73ac

msft-fluent-ui-bot added the Fluent UI react-northstar (v0) Work related to Fluent UI V0 label May 5, 2022

msft-fluent-ui-bot assigned GeoffCoxMSFT May 5, 2022

e2e fix

c8128d4

YuanboXue-Amber changed the title ~~fix(Dialog): keep Dialog open when press click on content and release on overlay + enable Dialog cypress~~ fix(Dialog): keep Dialog open when press click on content and release on overlay May 5, 2022

YuanboXue-Amber added 2 commits May 5, 2022 14:06

chglog

2983599

Revert "e2e fix"

d42d7de

This reverts commit c8128d4.

YuanboXue-Amber marked this pull request as ready for review May 5, 2022 12:08

YuanboXue-Amber requested a review from a team as a code owner May 5, 2022 12:08

ling1726 reviewed May 5, 2022

View reviewed changes

YuanboXue-Amber added 4 commits May 6, 2022 11:22

Merge branch 'master' into dialog-drag

1f5cfa4

e2e

f6cfba3

Merge branch 'dialog-drag' of https://github.com/YuanboXue-Amber/flue…

66f5ab9

…ntui into dialog-drag

Merge remote-tracking branch 'upstream/master' into dialog-drag

3a10561

ling1726 reviewed May 9, 2022

View reviewed changes

ling1726 approved these changes May 9, 2022

View reviewed changes

comments on test

d7a27df

YuanboXue-Amber enabled auto-merge (squash) May 10, 2022 08:14

YuanboXue-Amber merged commit a5aaf19 into microsoft:master May 10, 2022

YuanboXue-Amber deleted the dialog-drag branch May 10, 2022 08:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

YuanboXue-Amber commented May 5, 2022 •

edited

Loading

Uh oh!

codesandbox-ci bot commented May 5, 2022 •

edited

Loading

Uh oh!

size-auditor bot commented May 5, 2022 •

edited

Loading

Uh oh!

fabricteam commented May 5, 2022 •

edited

Loading

Uh oh!

fabricteam commented May 5, 2022 •

edited

Loading

Uh oh!

ling1726 May 5, 2022

Uh oh!

YuanboXue-Amber May 6, 2022

Uh oh!

ling1726 May 9, 2022

Uh oh!

YuanboXue-Amber May 9, 2022

Uh oh!

YuanboXue-Amber May 9, 2022

Uh oh!

ling1726 May 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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

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

Uh oh!

Conversation

YuanboXue-Amber commented May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current Behavior

New Behavior

Uh oh!

codesandbox-ci bot commented May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

size-auditor bot commented May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Asset size changes

Uh oh!

fabricteam commented May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Bundle size report

Uh oh!

fabricteam commented May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Perf Analysis (@fluentui/react-northstar)

Uh oh!

ling1726 May 5, 2022

Choose a reason for hiding this comment

Uh oh!

YuanboXue-Amber May 6, 2022

Choose a reason for hiding this comment

Uh oh!

ling1726 May 9, 2022

Choose a reason for hiding this comment

Uh oh!

YuanboXue-Amber May 9, 2022

Choose a reason for hiding this comment

Uh oh!

YuanboXue-Amber May 9, 2022

Choose a reason for hiding this comment

Uh oh!

ling1726 May 9, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

YuanboXue-Amber commented May 5, 2022 •

edited

Loading

codesandbox-ci bot commented May 5, 2022 •

edited

Loading

size-auditor bot commented May 5, 2022 •

edited

Loading

fabricteam commented May 5, 2022 •

edited

Loading

fabricteam commented May 5, 2022 •

edited

Loading

Perf Analysis (`@fluentui/react-northstar`)