EventGroup.ts preventing callbacks from being invoked on components in Modals

[mattmazzola]

Seems to be a bug in EventGroup.ts that blocks events from within a Modal.

Can see in the video below that buttons receive the click event but somewhere in EventGroup.js it is terminated. I was surprised even the native button is blocked.

https://microsoft.sharepoint.com/:v:/t/convsysresearch/ETulIpY7KP9Nl9AfDLebWxsBC545qYHqZwLAe1jvbcx3hg?e=LInPgn

It seems to be related to Modal stacking order. Odd thing is this modal is on top. I wonder if this means the events are being captures instead of bubbled in order for it to be intercepted.

Environment Information

"office-ui-fabric-react": "5.100.0", Chrome: Version 72.0.3626.121 (Official Build) (64-bit)

Please provide a reproduction of the bug in a codepen:

Actual behavior:

Callbacks are not invoked.

Expected behavior:

Callbacks should be invoked.

Priorities and help requested:

Are you willing to submit a PR to fix? (No)

Requested priority: (High)

Products/sites affected: ConversationLearner

[KevinTCoughlin]

Thanks for reporting this @mattmazzola. It would be helpful to have this isolated in a Codepen / CodeSandbox for us to be able to debug and fix quickly.

Would you be able to provide an isolated repro so that I can ensure the fix is correct given your scenario?

Its important to note that this is reported against 5.100.0 and so its also worth I investigate if this is still an issue in 6.x.

[mattmazzola]

I can try to recreate. How do you create a codepen that uses Modals in 5.x?

I was starting here: https://codepen.io/mattmazzola-the-reactor/pen/xBEwrZ?editors=1010

Can see <Modal> is commented out, if you uncomment there are errors and it fails to render.

I am using: <script src="//unpkg.com/office-ui-fabric-react@5.100.0/dist/office-ui-fabric-react.min.js"></script> But this module doesn't export the Modal component. I had mentioned this somewhere else as it seems like a first-class component that should be exported.

In code we have to do this:

And deep linking inside modules not good practice.

[KevinTCoughlin]

Verification against 6.x

Hrmm, I don't see this callback issue in 6.x, see https://codepen.io/kevintcoughlin/pen/pYZEbL?editors=1010.

Verification against 5.x

I don't see an issue with inner component callbacks (native nor OUFR) in 5.x, see the following branch & diff:

Diff

diff --git a/packages/office-ui-fabric-react/src/components/Modal/examples/Modal.Basic.Example.tsx b/packages/office-ui-fabric-react/src/components/Modal/examples/Modal.Basic.Example.tsx
index 896b8eb99..2d645a8ef 100644
--- a/packages/office-ui-fabric-react/src/components/Modal/examples/Modal.Basic.Example.tsx
+++ b/packages/office-ui-fabric-react/src/components/Modal/examples/Modal.Basic.Example.tsx
@@ -1,6 +1,6 @@
 import * as React from 'react';
 import { Modal } from 'office-ui-fabric-react/lib/Modal';
-import { DefaultButton } from 'office-ui-fabric-react/lib/Button';
+import { DefaultButton, PrimaryButton } from 'office-ui-fabric-react/lib/Button';
 import './Modal.Basic.Example.scss';

 export class ModalBasicExample extends React.Component<{}, {
@@ -37,6 +37,9 @@ export class ModalBasicExample extends React.Component<{}, {
             <p>Sed condimentum ultricies turpis convallis pharetra. Sed sagittis quam pharetra luctus porttitor. Cras vel consequat lectus. Sed nec fringilla urna, a aliquet libero. Aenean sed nisl purus. Vivamus vulputate felis et odio efficitur suscipit. Ut volutpat dictum lectus, ac rutrum massa accumsan at. Sed pharetra auctor finibus. In augue libero, commodo vitae nisi non, sagittis convallis ante. Phasellus malesuada eleifend mollis. Curabitur ultricies leo ac metus venenatis elementum. </p>
             <p>Aenean egestas quam ut erat commodo blandit. Mauris ante nisl, pellentesque sed venenatis nec, aliquet sit amet enim. Praesent vitae diam non diam aliquet tristique non ut arcu. Pellentesque et ultrices eros. Fusce diam metus, mattis eu luctus nec, facilisis vel erat. Nam a lacus quis tellus gravida euismod. Nulla sed sem eget tortor cursus interdum. Sed vehicula tristique ultricies. Aenean libero purus, mollis quis massa quis, eleifend dictum massa. Fusce eu sapien sit amet odio lacinia placerat. Mauris varius risus sed aliquet cursus. Aenean lectus magna, tincidunt sit amet sodales a, volutpat ac leo. Morbi nisl sapien, tincidunt sit amet mauris quis, sollicitudin auctor est. </p>
             <p>Nam id mi justo. Nam vehicula vulputate augue, ac pretium enim rutrum ultricies. Sed aliquet accumsan varius. Quisque ac auctor ligula. Fusce fringilla, odio et dignissim iaculis, est lacus ultrices risus, vitae condimentum enim urna eu nunc. In risus est, mattis non suscipit at, mattis ut ante. Maecenas consectetur urna vel erat maximus, non molestie massa consequat. Duis a feugiat nibh. Sed a hendrerit diam, a mattis est. In augue dolor, faucibus vel metus at, convallis rhoncus dui.</p>
+            <PrimaryButton text="Ok" onClick={() => console.log('Ok', 'onClick')} />
+            <DefaultButton text="Cancel" onClick={() => console.log('Cancel', 'onClick')} />
+            <button onClick={() => console.log('Native Button', 'onClick')}>Click Me</button>
           </div>
         </Modal>
       </div>

Output

Native Button onClick
Modal.Basic.Example.tsx:44 Cancel onClick
Modal.Basic.Example.tsx:43 Ok onClick
Modal.Basic.Example.tsx:44 Cancel onClick
Modal.Basic.Example.tsx:45 Native Button onClick

Modal as top-level export in 5.x

Regarding:

But this module doesn't export the Modal component. I had mentioned this somewhere else as it seems like a first-class component that should be exported.

I'm unsure why 5.x omits Modal exports as it predates me. We can follow-up once the initial reported issue is addressed. However I doubt we will export a new API surface for a previous major release as we head towards 7.x.

[KevinTCoughlin]

@mattmazzola I'm unable to repro your issue with component callbacks not firing when rendered within a Modal. See my findings above, https://github.com/OfficeDev/office-ui-fabric-react/issues/8264#issuecomment-474131092.

If you believe this issue to be related to OUFR, can you please fork the 5.x branch I've created to demonstrate the library bug?

Here's the branch: https://github.com/KevinTCoughlin/office-ui-fabric-react/tree/keco/debug-modal-callbacks-v5.

FWIW, I forked from office-ui-fabric-react@5.135.0 not 5.100.0 which I can also do.

[mattmazzola]

Yea, I wasn't able to recreate it either. I also have separate repo which tried to similar the scenario from our app more closely but wasn't able to reproduce. The modals worked as expected then.

To be clear the issue isn't simply callbacks not being invoked when in modals. We have many modals in our app which work fine. It's something to do with the stacking order and other components that interfere with normal behavior. For example there was issue we had while back with help panels not being closable do to the same issue but we didn't know about EventGroup back then.

To help me diagnose, can you explain what EvenGroup.ts does and why it's needed? Is there a way for us to disable it or our app and to let all events through?

We've had previous complications with etc now with the EventGroup.ts being another issue and would want to know why Office fabric is affecting these core browser functions? I had expected it to be a collection of themed components, but it is doing much more. I assume they're necessary for some accessibility feature but when would I require them?

I would say you don't need to try to reproduce again. We ended up working around the issue by nesting the modals. (Modal C must be declared inside Modal A and Modal B even though it's shared. We know it's poor design and not very scalable but our only work around at the moment and indistinguishable from user point of view.

If I get time I will try to update my sample to try to reproduce again.

[KevinTCoughlin]

To help me diagnose, can you explain what EventGroup.ts does and why it's needed?

EventGroup is used for a number of scenarios in office-ui-fabric-react, some examples are:

1. Auto disposal of bound callbacks to prevent memory leaks via _events, particularly in BaseComponent.
2. Normalize event behaviors, particularly w.r.t IE-compatibility.
3. Custom eventing abstraction for component communications

See all usages at https://github.com/OfficeDev/office-ui-fabric-react/search?l=TypeScript&q=EventGroup.

Is there a way for us to disable it or our app and to let all events through?

I'm unaware of a global way to disable EventGroup aside from overriding it with an empty module or somesuch which I wouldn't advocate.

---

Ultimately I defer to @dzearing when discussing the usefulness of EventGroup within the context of OUFR.

[dzearing]

EventGroup is simply an abstraction on addEventListener. Your issue has nothing to do with EventGroup, and more to what is receiving the callback and then calling stopPropagation, if I understand correctly.

But all that said, I'm really having a hard time understanding what the repro is.

Fabric 6, I put a button in a modal, executes onClick no problem. https://codepen.io/dzearing/pen/MxBmMe?editors=1010

I can see if I spot something in Fabric 5...

[dzearing]

I just downgraded node.js to build Fabric 5 and found this does not repro in Fabric 5.

You can try on your own:

install node 8 git clone the fabric repo switch to branch 5 npm install npm start

Go edit Modal.BasicExample.ts Add <button onClick={ ()=>alert('works') }>Clickme</button> inside of the modal.

Go click it and see the alert.

In your project, something is calling preventDefault or stopPropagation, likely on a captured click event. You can debug this by doing something like this in the console:

Event.prototype.stopPropagation = () => console.trace();

Should spit out a callstack whenever the event is stopped.

[KevinTCoughlin]

Closing as per:

I would say you don't need to try to reproduce again.

Feel free to ping if you wish to re-open this @mattmazzola.

[mattmazzola]

I only intended to save you spending time reproducing work, not to close. It's still an issue that people should see as unresolved.

I have created a video using David's suggesting above and you can see the stopPropagation coming from the FocusTrapZone.

Video: https://microsoft.sharepoint.com/:v:/t/convsysresearch/EV3GnuTstDdIkeb5HiN8YGsBTC41nfUYaVZhpH6VHcFVGA?e=SLP1fg

Please re-open this issue. I guess it could be better named as FocusTrapZone preventing callbacks than EventGroup but it seems they are related.

If I recall we've have/had other issues with FocusTrapZone related to changing the focus of inputs from the one the user chose. If I wanted to test with these turns off I assume I would need to do more than simple make it an empty module as other things on Office Fabric are depending on these functions. What's the best approach? This is now affecting use regardless if the modal affected is nested (duplicated) or shared which was our only way around this before.

[mattmazzola]

Can you clarify intended behavior of FocusTrapZone?

What is it surrounding? When should it be added to the page?

There was talk in Teams about the issue occurring when multiple zones are "fighting" for control, I assume one stops the events for the other and ends up breaking everything that relied on the event.

In the particular case I have there are 3 and I don't know why. My understanding is there should really only ever be 1 shared for all Fabric components?

Hover over each of Focus Zones to highlight area. See small one in middle doesn't correspond to either modal.

[dzearing]

The FocusTrapZone is used to keep the focus from going outside of a bounding box. It's primary usage is for modal scenarios where hitting TAB should cycle within a dialog, and not suddenly jump behind the semi-opaque background in your screenshot.

That's why Dialog and Panel typically have this. The scenario you're running into is a layered scenario, where you have a dialog on top of a dialog on top of a dialog. I remember in Fabric 5 (which you are using) we had some issues with stacked FocusTrapZones.

@JasonGore just fixed numerous bugs within FocusTrapZone in Fabric 6, so I'd love to make sure we're good with layered scenarios like this in the latest version.

Is there anything preventing you from upgrading to a more recent release of Fabric?

[JasonGore]

I've deleted a couple of comments that I could see greatly increasing confusion. Sorry if you've received notifications for them.

I think the best thing to do here is work from examples of what you're doing to help us understand if FocusTrapZone is adequate for your scenario.

The main thing we don't know here is whether the FocusTrapZones are nested or siblings.

FocusTrapZone should work appropriately in nested scenarios, which our component page shows an example of.

If, however, if the FocusTrapZones are siblings, the behavior may or may not be predictable. It would depend on their props values.

The only way to really figure out which scenario is applicable here is to get a concrete scenario. Can you share your implementation or perhaps put it in a CodePen?

[mattmazzola]

Related to "Upgrading to 6.x to inherit fixes for FocusTrapZone": I think this is possibility. If it fixes the modal issues we will definately move it up priority. It's just harder to overlay my changes so it contains 6.x upgrade and the modal issue in same change to test to see if it fixes anything.

1. Types issue on children which has been fixed. https://github.com/OfficeDev/office-ui-fabric-react/issues/8177

2. Modal / DOM change issue is: https://github.com/OfficeDev/office-ui-fabric-react/issues/8178 Although I have changed our CSS to try to accomodate and I think it will work.

3. Seems to be type issue with fonts, but can fix by using number directly: Seems to be using string "100" instead of number 100 which is accepted by React types. Not sure which is more correct.

4. Other cosmetic things I've noticed: I think this can also be overcome with changes to our CSS.

Going back to FocusTrapZone issues:

Can you share your implementation

Yes. Our front-end is open source. I pushed the branch I was working on here so you can see although I imagine it's hard to pinpoint the issue amognst all the unrelated code https://github.com/Microsoft/ConversationLearner-UI/tree/mattm/log-dialog-conflicts

or perhaps put it in a CodePen?

I don't think 5.x allows modals in code pens. The component isn't exported. Been issue I complained about a while back. It seems fixed in 6.x.
https://github.com/OfficeDev/office-ui-fabric-react/issues/8264#issuecomment-472970568

I have tried to recreate the scenario in separate repo with similar multiple modals and issue doesn't occur. There is something different about our app but I don't know what it is yet.

[mattmazzola]

The main thing we don't know here is whether the FocusTrapZones are nested or siblings.

How can I tell? React Dev tools seems to flatten the list, although I think they are nested in the DOM.

FocusTrapZone should work appropriately in nested scenarios

This is would explain why duplicating/nesting the modals fixed part of the issue, but I don't think it's a good practice/requirement. The modal is supposed to be shared / pop up under many conditions and it would require copying it to every other modal that it can layer on top of. This isn't intuitive or scalable.

It would depend on their props values.

My understanding was FocusTrapZones were side affects of using other components such as Modals or Pandels. We never get to set props of them directly. What affects them?

[mattmazzola]

Following up with discussions elsewhere so other people can see.

• Confirmed using 6.x version does fix the issue.
• Seems to be fixed in this release: https://github.com/OfficeDev/office-ui-fabric-react/releases/tag/office-ui-fabric-react_v6.43.0
• From this PR: https://github.com/OfficeDev/office-ui-fabric-react/pull/5351

The fix/bug was related to how the hierarchy of modals affected focus trap zones behavior so we started investigating more. There is an issue if the modals that are open are not matching the hierarchy of them in the DOM. By correcting this I was able to make it work even on 5.x (5.100).

I will try to explain the issue although there might be technical inaccuracies If you are using a release without the fix (Pre 6.43): DOM order must match visibility Order

Issue:

|
+--- Modal A (open second)
|
+--- Modal B (open first)
  |
  +--- Modal C (open third)

Because Modal A was opened before Modal C and it was higher in the hierarchy it was assumed to have higher priority even though it was not visible since Modal C was on top of it. This meant Modal A's FocusTrapZone would be preventing interaction from the child Modal C.

In our case the fix was simply to not leave Modal A open if it can be closed without interrupting the UI removing information/context the user needs.

If the modal was needed, the hierarchy would have to match like this:

|
+--- Modal A
|
+--- Modal B (open first)
  |
  +--- Modal A (open second)
    |
    +--- Modal C (open third)

If you are on a release containing this fix (Post 6.34) you should not have to worry about this hierarchy issue. The trap zones automatically know which was opened last which makes it the top most or visible modal and it will naturally have the correct focus / event blocking behavior.

To recap solutions:

1. Fix hierarchy
2. Upgrade to 6.x

[dzearing]

Thank you for the followup @mattmazzola!