feat(Popover): Support nesting #18148

ling1726 · 2021-05-11T16:20:21Z

Pull request checklist

Addresses an existing issue: Fixes #0000
Include a change request file using $ yarn change

Description of changes

Introduces virtual-parents to react-portal. Portal will always be rendered with a hidden span so that elementContains will work for all Portals in the same React tree

The virtual parent feature will allow Popovers, Menus and Tooltips to be nested so that:

click outside works for the entire out of order stack
clicking in parent out of order component will close nested out of order component

Focus areas to test

(optional)

Introduces `@fluentui/react-virtual-parents` as a utility library to set virtual parents. Integrates it as an optional feature in `usePopper`. The virtual parent feature will allow `Popovers` to be nested so that: * click outside works for the entire `Popover` stack * clicking in parent `Popovers` will close nested `Popovers`

codesandbox-ci · 2021-05-11T16:36:03Z

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 aae1997:

Sandbox	Source
Fluent UI Button	Configuration
codesandbox-react-template	Configuration
codesandbox-react-northstar-template	Configuration

ling1726 · 2021-05-11T19:29:39Z

Marking as draft again, to extend the solution to Portal

packages/react-utilities/src/hooks/useOnClickOutside.ts

layershifter

Can you please also port some unit tests for virtualParent?

packages/react-portal/src/virtualParent/setVirtualParent.ts

packages/react-portal/src/virtualParent/isVirtualElement.ts

packages/react-portal/src/virtualParent/getVirtualParent.ts

packages/react-portal/src/virtualParent/getParent.ts

packages/react-portal/src/virtualParent/elementContains.ts

packages/react-portal/src/components/Portal/usePortal.ts

packages/react-utilities/src/hooks/useOnClickOutside.ts

size-auditor · 2021-05-12T08:47:16Z

Asset size changes

Project	Bundle	Baseline Size	New Size	Difference
office-ui-fabric-react	fluentui-react-components-Menu	99.497 kB	99.876 kB	379 bytes
office-ui-fabric-react	fluentui-react-components-Portal	2.211 kB	2.553 kB	342 bytes
office-ui-fabric-react	fluentui-react-components-Tooltip	39.083 kB	39.425 kB	342 bytes

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

Baseline commit: 644df65321367dd615548b2877293d5268595c2b (build)

fabricteam · 2021-05-12T08:58:50Z

Perf Analysis (`@fluentui/react`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	946	942	5000
BaseButton	mount	1025	1050	5000
Breadcrumb	mount	2816	2859	1000
ButtonNext	mount	561	563	5000
Checkbox	mount	1793	1773	5000
CheckboxBase	mount	1482	1511	5000
ChoiceGroup	mount	5352	5428	5000
ComboBox	mount	1168	1107	1000
CommandBar	mount	11099	11048	1000
ContextualMenu	mount	6839	6785	1000
DefaultButton	mount	1291	1300	5000
DetailsRow	mount	4081	4156	5000
DetailsRowFast	mount	4210	4160	5000
DetailsRowNoStyles	mount	3895	3912	5000
Dialog	mount	2310	2329	1000
DocumentCardTitle	mount	163	158	1000
Dropdown	mount	3622	3576	5000
FocusTrapZone	mount	1928	2020	5000
FocusZone	mount	1938	1989	5000
IconButton	mount	1948	2005	5000
Label	mount	387	380	5000
Layer	mount	1991	2018	5000
Link	mount	517	515	5000
MakeStyles	mount	1927	2045	50000
MenuButton	mount	1648	1673	5000
MessageBar	mount	2199	2220	5000
Nav	mount	3695	3597	1000
OverflowSet	mount	1160	1126	5000
Panel	mount	2286	2236	1000
Persona	mount	897	900	1000
Pivot	mount	1578	1585	1000
PrimaryButton	mount	1406	1435	5000
Rating	mount	8583	8745	5000
SearchBox	mount	1513	1512	5000
Shimmer	mount	2844	2989	5000
Slider	mount	2198	2184	5000
SpinButton	mount	5558	5500	5000
Spinner	mount	478	450	5000
SplitButton	mount	3502	3482	5000
Stack	mount	552	565	5000
StackWithIntrinsicChildren	mount	1699	1694	5000
StackWithTextChildren	mount	5192	5128	5000
SwatchColorPicker	mount	11239	11232	5000
Tabs	mount	1573	1533	1000
TagPicker	mount	2817	2775	5000
TeachingBubble	mount	12908	12886	5000
Text	mount	484	500	5000
TextField	mount	1576	1559	5000
ThemeProvider	mount	1328	1300	5000
ThemeProvider	virtual-rerender	658	667	5000
ThemeProviderNext	mount	7075	7023	5000
Toggle	mount	906	886	5000
buttonNative	mount	116	133	5000

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

⚠️ 1 potential perf regressions detected

Potential regressions comparing to master

Scenario	Current PR Ticks	Baseline Ticks	Ratio	Regression Analysis
AvatarMinimalPerf.default	218	222	0.98:1	analysis

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
ButtonMinimalPerf.default	215	190	1.13:1
AttachmentMinimalPerf.default	198	177	1.12:1
SkeletonMinimalPerf.default	441	401	1.1:1
CarouselMinimalPerf.default	546	502	1.09:1
HeaderMinimalPerf.default	444	413	1.08:1
LabelMinimalPerf.default	462	430	1.07:1
ReactionMinimalPerf.default	454	424	1.07:1
TableMinimalPerf.default	481	451	1.07:1
TextMinimalPerf.default	421	394	1.07:1
MenuMinimalPerf.default	997	940	1.06:1
SegmentMinimalPerf.default	410	385	1.06:1
VideoMinimalPerf.default	734	692	1.06:1
AccordionMinimalPerf.default	183	175	1.05:1
TooltipMinimalPerf.default	1116	1067	1.05:1
CardMinimalPerf.default	643	617	1.04:1
DropdownManyItemsPerf.default	791	764	1.04:1
ItemLayoutMinimalPerf.default	1495	1438	1.04:1
LayoutMinimalPerf.default	426	411	1.04:1
ListCommonPerf.default	745	715	1.04:1
PortalMinimalPerf.default	174	167	1.04:1
RadioGroupMinimalPerf.default	512	491	1.04:1
IconMinimalPerf.default	721	691	1.04:1
ToolbarMinimalPerf.default	1112	1072	1.04:1
DatepickerMinimalPerf.default	6224	6043	1.03:1
DividerMinimalPerf.default	427	414	1.03:1
FlexMinimalPerf.default	328	318	1.03:1
GridMinimalPerf.default	396	384	1.03:1
ProviderMinimalPerf.default	1114	1084	1.03:1
StatusMinimalPerf.default	804	783	1.03:1
TreeMinimalPerf.default	908	878	1.03:1
AnimationMinimalPerf.default	477	467	1.02:1
BoxMinimalPerf.default	397	388	1.02:1
ChatDuplicateMessagesPerf.default	328	323	1.02:1
CheckboxMinimalPerf.default	3059	2997	1.02:1
MenuButtonMinimalPerf.default	1803	1768	1.02:1
CustomToolbarPrototype.default	4123	4059	1.02:1
AttachmentSlotsPerf.default	1268	1257	1.01:1
DialogMinimalPerf.default	827	821	1.01:1
DropdownMinimalPerf.default	3386	3343	1.01:1
InputMinimalPerf.default	1376	1368	1.01:1
ListMinimalPerf.default	571	564	1.01:1
SliderMinimalPerf.default	1698	1683	1.01:1
SplitButtonMinimalPerf.default	4206	4168	1.01:1
TableManyItemsPerf.default	2207	2180	1.01:1
TextAreaMinimalPerf.default	599	593	1.01:1
TreeWith60ListItems.default	188	187	1.01:1
ButtonOverridesMissPerf.default	1851	1859	1:1
ButtonSlotsPerf.default	620	621	1:1
ChatMinimalPerf.default	708	709	1:1
EmbedMinimalPerf.default	4537	4537	1:1
ImageMinimalPerf.default	435	437	1:1
LoaderMinimalPerf.default	758	760	1:1
AlertMinimalPerf.default	319	322	0.99:1
HeaderSlotsPerf.default	881	892	0.99:1
FormMinimalPerf.default	466	477	0.98:1
ListWith60ListItems.default	720	738	0.98:1
ProviderMergeThemesPerf.default	1743	1770	0.98:1
PopupMinimalPerf.default	620	639	0.97:1
RefMinimalPerf.default	256	264	0.97:1
ChatWithPopoverPerf.default	394	416	0.95:1
ListNestedPerf.default	618	655	0.94:1
RosterPerf.default	1250	1351	0.93:1

Introduces `@fluentui/react-virtual-parents` as a utility library to set virtual parents. Integrates it as an optional feature in `usePopper`. The virtual parent feature will allow `Popovers` to be nested so that: * click outside works for the entire `Popover` stack * clicking in parent `Popovers` will close nested `Popovers`

ling1726 · 2021-05-12T14:09:12Z

@behowell can you take a look at the tooltip snapshot updates ? It's caused by Portal rendering a span for virtual parents

packages/react-portal/src/virtualParent/index.ts

packages/react-portal/src/virtualParent/getParent.ts

miroslavstastny · 2021-05-17T14:43:41Z

packages/react-portal/src/virtualParent/getVirtualParent.ts

+ */
+export function getVirtualParent(child: HTMLElement): HTMLElement | undefined {
+  let parent: HTMLElement | undefined;
+  if (child && isVirtualElement(child)) {


Do you need to check child here?
It is HTMLElement and it's isVirtualElement's responsibility to check it's truthy

You're right, it's unnecessary

packages/react-portal/README.md

packages/react-portal/Spec.md

packages/react-portal/src/components/Portal/renderPortal.tsx

behowell · 2021-05-17T17:46:17Z

packages/react-portal/src/components/Portal/usePortal.ts

+  React.useEffect(() => {
+    if (state.virtualParentRootRef.current && state.mountNode) {
+      setVirtualParent(state.mountNode, state.virtualParentRootRef.current);
+    }
+  }, [state.virtualParentRootRef, state.mountNode]);


This won't re-run if virtualParentRootRef.current changes. You might be able to make it work by making virtualParentRootRef a function that gets called whenever the ref is set... but then you'd also want to update the virtual parent whenever mountNode changes.

That said, presumably setVirtualParent is cheap right? Maybe it's simpler to make this effect run after every render (remove the deps).

I'm not entirely sure if virtualParentRootRef.current would ever change, since it is always set internally into that span that users will never have access to.

That inacessible slot always renders even during server render in SSR or in the client when the Portal can't be rendered.

packages/react-portal/src/virtualParent/getParent.ts

packages/react-portal/src/virtualParent/types.ts

packages/react-popover/src/components/Popover/usePopover.ts

msft-fluent-ui-bot · 2021-05-19T07:37:28Z

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

msft-fluent-ui-bot · 2021-05-19T07:37:52Z

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

msft-fluent-ui-bot · 2021-05-19T07:38:52Z

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

ling1726 added 2 commits May 11, 2021 14:43

nested popover story

1b898bc

ling1726 requested review from layershifter and a team May 11, 2021 16:20

Change files

5bb2423

ling1726 marked this pull request as draft May 11, 2021 17:27

ling1726 added 3 commits May 12, 2021 09:05

move virtual parent code to portal

59551ce

remove package, refactor vparent

1db41ad

Change files

850353a

ling1726 marked this pull request as ready for review May 12, 2021 07:57

layershifter reviewed May 12, 2021

View reviewed changes

packages/react-utilities/src/hooks/useOnClickOutside.ts Outdated Show resolved Hide resolved

Add docs and spec for portal

8c1cfb2

ling1726 added the Type: Spec Component spec PR label May 12, 2021

layershifter approved these changes May 12, 2021

View reviewed changes

fix api extractor

766af0e

ling1726 added the Component: Popover label May 12, 2021

ling1726 added 2 commits May 12, 2021 11:34

PR suggestions

eb9683e

Add E2E tests for nested popovers

6372751

ling1726 requested a review from a team as a code owner May 12, 2021 10:19

turn off as conformance test

5452246

ling1726 enabled auto-merge (squash) May 12, 2021 11:31

ling1726 added 3 commits May 12, 2021 14:59

nested popover story

35529c3

Change files

3476574

ling1726 requested review from lorejoh12, miroslavstastny and Raghurk as code owners May 12, 2021 14:06

ling1726 requested a review from a team May 12, 2021 14:06

ling1726 disabled auto-merge May 12, 2021 14:06

Merge remote-tracking branch 'origin/master' into feat/nested-popup

80f363d

ling1726 removed request for a team, dzearing, ecraig12345, khmakoto, Raghurk, assuncaocharles, lorejoh12, miroslavstastny, hyoshis, Jacqueline-ms, evlevy and JustSlone May 12, 2021 14:07

khmakoto reviewed May 17, 2021

View reviewed changes

packages/react-portal/src/virtualParent/index.ts Outdated Show resolved Hide resolved

miroslavstastny approved these changes May 17, 2021

View reviewed changes

behowell approved these changes May 17, 2021

View reviewed changes

ling1726 added 2 commits May 18, 2021 08:19

Merge remote-tracking branch 'origin' into feat/nested-popup

319c017

PR suggestions

aae1997

ling1726 enabled auto-merge (squash) May 18, 2021 06:48

ling1726 merged commit 90cd8ca into microsoft:master May 18, 2021

layershifter mentioned this pull request Nov 22, 2024

v3: roadmap to release Semantic-Org/Semantic-UI-React#4409

Open

27 tasks

feat(Popover): Support nesting #18148

feat(Popover): Support nesting #18148

Uh oh!

Conversation

ling1726 commented May 11, 2021 • edited by khmakoto Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

Description of changes

Focus areas to test

Uh oh!

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

Uh oh!

Uh oh!

ling1726 commented May 11, 2021

Uh oh!

Uh oh!

layershifter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Asset size changes

Uh oh!

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

Uh oh!

Perf Analysis (@fluentui/react)

Perf Analysis (@fluentui/react-northstar)

Potential regressions comparing to master

Uh oh!

ling1726 commented May 12, 2021

Uh oh!

Uh oh!

Uh oh!

miroslavstastny May 17, 2021

Choose a reason for hiding this comment

Uh oh!

ling1726 May 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

behowell May 17, 2021

Choose a reason for hiding this comment

Uh oh!

ling1726 May 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

msft-fluent-ui-bot commented May 19, 2021

Uh oh!

msft-fluent-ui-bot commented May 19, 2021

Uh oh!

msft-fluent-ui-bot commented May 19, 2021

Uh oh!

Uh oh!

ling1726 commented May 11, 2021 •

edited by khmakoto

Loading

codesandbox-ci bot commented May 11, 2021 •

edited

Loading

size-auditor bot commented May 12, 2021 •

edited

Loading

fabricteam commented May 12, 2021 •

edited

Loading

Perf Analysis (`@fluentui/react`)

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