Layer: Reintroduce Portals with Event Blocking #6211

JasonGore · 2018-09-04T17:29:46Z

Pull request checklist

Addresses an existing issue: Fixes Re-introduce Portals support #4849
Include a change request file using $ npm run change

Description of changes

Layer currently uses React's unstable_renderSubtreeIntoContainer, which will soon be removed. Additionally, it's incompatible with React 16 Context and has hampered our efforts to evolve theming with Layered components. As a result, we've had to prioritize reintroducing React portals use in Layer.

React Portals more accurately respect virtual hierarchy in allowing events to bubble up and trickle down as well as in processing mouse behavior. I've reintroduced Layer's use of React portals with event blocking added to more closely simulate Layer's existing behavior. Event bubbling is controlled via a new Layer prop eventBubblingEnabled and is disabled by default for backwards compatibility.

However, portals and this PR do introduce a couple of known behavior changes regarding the events onMouseEnter and onMouseLeave.

These events cannot be blocked without breaking backwards compatibility and invasive and hacky changes to how React operates. With portals, when Layered components are moused over, every onMouseEnter handler along the subtree from the common ancestor of the component left down to the Layered component will be triggered. (This didn't happen with old Layer because these events just traversed directly to the Layer subtree attached to root.)

For example, if a Tooltip contains a Modal and the Modal is open, the Tooltip will appear over the Modal when the mouse enters the browser window. This happens because Tooltip's onMouseEnter handler is now triggered with portals where it wasn't before. For these types of cases, a utilities helper portalContainsElement was added to help work around this specific example:

  private _onTooltipMouseEnter = (ev: React.MouseEvent<HTMLElement>): void => {
    ...
	
    if (ev.target && portalContainsElement(ev.target as HTMLElement, this._getTargetElement())) {
      // Do not show tooltip when target is inside a portal relative to TooltipHost.
      return;
    }

    ...
  };

If both TooltipHost and ev.target are contained within the same portal, the Tooltip will continue to appear as expected.

Likewise, onMouseLeave used to be generated when Layered components rendered. Now that Layered components are more accurately viewed as part of the hierarchy, onMouseLeave events may not get generated when rendering Layered content. Additionally, onMouseLeave events will now get generated for the entire root->content->Layer->Layered component subtree when the mouse leaves the browser window or any point in that subtree.

Focus areas to test

Unit tests added for event blocking and utility helper.

Microsoft Reviewers: Open in CodeFlow

* Use Portals when available for Callout * Update shrinkwrap * Set virtual parent for initial render * Only wait for target when required * Update shrinkwrap * update * Prevent markdown-to-jsx from upgrading * Add patch file for example app base change * Update Layer.tsx

…led by new prop.

cschleiden · 2018-09-04T18:44:25Z

packages/office-ui-fabric-react/src/components/Layer/Layer.types.ts

+   * Traditionally Layer has not had this behavior. This prop preserves backwards compatibility by
+   * default while allowing users to opt in to the new event bubbling functionality.
+   */
+  eventBubblingEnabled?: boolean;


Will this limit us going forward? Should we have a new Layer that does this by default and that we recommend people to use?

That's a good question. In many use cases (Modals, Panels, Dialogs, etc.) I don't think you'll ever want event bubbling behavior. While portals more accurately portray virtual hierarchy, I don't think it's how people will ever want these common use cases to behave.

I think I agree here.

The spirit of a layer is that you are rendering on a higher elevation than the default body. It really is an attached layer, like photoshop layers, which simply overlay and do not interact.

There are cases where you need to know the originating dom heirarchy, and we have virtual parents for that. But I've never run into a case where events should bubble into or out of layers. That would kind of break the spirit.

Now I have seen web sites which use DOM eventing to raise all sorts of custom action events, so that they can be mediated through the root dom. Kind of like redux and central state, but with respect to DOM structure to provide a framework agnostic but tree sensitive data flow. In that case? Sure I could see a need to propagate events.

cschleiden · 2018-09-05T22:19:03Z

packages/office-ui-fabric-react/src/components/Layer/Layer.base.tsx

+            eventBubblingEnabled ? (
+              <Fabric className={classNames.content}>{this.props.children}</Fabric>
+            ) : (
+              <Fabric


Shouldn't this be its own component? Something like EventBoundary?

I didn't see the need since it's pretty simple in implementation. It's just one set of JSX args and one handler. If there's a case for reuse it could always be broken out later pretty easily.

packages/utilities/src/dom.ts

kenotron · 2018-09-05T22:23:48Z

packages/utilities/src/dom.ts

  };
 }

+export const dataPortalAttribute = 'data-portal-element';


string constants like these probably should be CAPITALIZED_LIKE_THIS

kenotron · 2018-09-05T22:24:20Z

packages/utilities/src/dom.ts

+ * If both parent and child are within the same portal this function will return false.
+ */
+export function portalContainsElement(target: HTMLElement, parent?: HTMLElement): boolean {
+  let elementMatch = findElementRecursive(


I wonder why tslint didn't catch that?

kenotron · 2018-09-05T22:24:56Z

packages/utilities/src/dom.test.ts

+    setPortalAttribute(portal);
+  });
+
+  it('works correctly', () => {


can you describe what "works correctly" means in this so that readers can understand the "spec" of this?

kenotron · 2018-09-05T22:28:25Z

packages/office-ui-fabric-react/src/components/ComponentExamples.test.tsx


  beforeAll(() => {
+    // Mock createPortal to capture its component hierarchy in snapshot output.
+    ReactDOM.createPortal = jest.fn(element => {


pretty sure you're the only one testing with portals, so not an immediate issue here. But you should restore the implementation when you're all done inside an afterAll() block.

packages/office-ui-fabric-react/src/components/ComponentExamples.test.tsx

packages/office-ui-fabric-react/src/components/Layer/Layer.base.tsx

dzearing · 2018-09-05T23:36:43Z

packages/office-ui-fabric-react/src/components/Layer/Layer.base.tsx

-        const doc = getDocument(rootElement);
-
-        if (!doc || !rootElement) {
+        const doc = getDocument();


I believe getDocument takes an element, so if host is available, maybe pass that in?

@cschleiden Do you remember the rationale here?

kenotron

Okay my comments are resolved!

cliffkoh · 2018-09-07T00:23:21Z

👏

KevinTCoughlin · 2018-09-07T00:27:07Z

🥇

cschleiden and others added 6 commits August 28, 2018 14:44

Add event blocking to maintain traditional behavior that can be disab…

91a8051

…led by new prop.

Remove old LayerBase implementation.

8887ae9

Add portal utility helper and use in Tooltip.

f7434a2

Revert incorrect typing.

6c726c4

Change files.

6d82d75

JasonGore requested review from ThomasMichon, cschleiden, dzearing and kenotron September 4, 2018 17:29

JasonGore requested review from joschect and micahgodbolt as code owners September 4, 2018 17:29

cschleiden reviewed Sep 4, 2018

View reviewed changes

JasonGore added 4 commits September 4, 2018 11:57

Update users-cschleid-portalLayers_2018-05-03-15-52.json

93808d5

Merge branch 'master' into jg/portals-part-deux

edb0a51

Update snapshots for merge.

c1f7f03

Add utility unit tests.

6a0ca42

JasonGore added the Status: Do Not Merge 🚫 label Sep 4, 2018

JasonGore added 5 commits September 5, 2018 09:04

List dependency info for CI builds.

fe00b9e

Merge branch 'master' into jg/portals-part-deux

8981d24

Fix test break due to change in enzyme-adapter-react-16.

afaca8b

Add Layer event boundary unit tests.

6d8ad3b

Allow Tooltip to appear within portal when target is in same portal.

8a9a046

JasonGore removed the Status: Do Not Merge 🚫 label Sep 5, 2018

dzearing closed this Sep 5, 2018

dzearing reopened this Sep 5, 2018

cschleiden reviewed Sep 5, 2018

View reviewed changes

kenotron reviewed Sep 5, 2018

View reviewed changes

PR feedback.

4f9ad84

dzearing reviewed Sep 5, 2018

View reviewed changes

Clarifying comments.

923a66a

kenotron approved these changes Sep 6, 2018

View reviewed changes

dzearing approved these changes Sep 6, 2018

View reviewed changes

JasonGore merged commit a0d0b91 into microsoft:master Sep 6, 2018

JasonGore mentioned this pull request Sep 7, 2018

Customizer now uses React 16 context. (#5701) #6268

Merged

2 tasks

KevinTCoughlin mentioned this pull request Sep 10, 2018

Fix DatePicker re-rendering due to focus behavior change #6308

Merged

2 tasks

KevinTCoughlin mentioned this pull request Sep 17, 2018

When hitting Escape in IE, all ContextualMenus close instead of just closing the current submenu #4550

Closed

JasonGore mentioned this pull request Sep 27, 2018

Layer: Fix FocusTrapZone detection of Layered children #6489

Merged

2 tasks

KevinTCoughlin mentioned this pull request Sep 27, 2018

Layer: Focus/blur listeners not triggered for components on Panel #6490

Closed

natalieethell mentioned this pull request Jun 19, 2019

ToolTipHost closes when content hovered #9464

Closed

microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019

Layer: Reintroduce Portals with Event Blocking #6211

Layer: Reintroduce Portals with Event Blocking #6211

Uh oh!

Conversation

JasonGore commented Sep 4, 2018 • edited 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

Microsoft Reviewers: Open in CodeFlow

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kenotron left a comment

Choose a reason for hiding this comment

Uh oh!

cliffkoh commented Sep 7, 2018

Uh oh!

KevinTCoughlin commented Sep 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JasonGore commented Sep 4, 2018 •

edited

Loading