Temporary drawer stops propagation of all child elements' click events.

[Hardtack]

Bugs

Follow the template below to ensure the quickest and most accurate response to your issue.

What MDC-Web Version are you using?

0.1.1

What browser(s) is this bug affecting?

Safari, Chrome

What OS are you using?

MacOS

What are the steps to reproduce the bug?

1. Add temporary drawer using foundation.

2. Add click event to child element of drawer (mdc-temporary-drawer__drawer).

3. Child element's click event handler doesn't fire because of interaction handler added by MDCTemporaryDrawerFoundation
   

   material-components-web/packages/mdc-drawer/temporary/foundation.js

   Line 102 in 7fa157c

   this.adapter_.registerDrawerInteractionHandler('click', this.drawerClickHandler_);

What is the expected behavior?

It should fire click event of elements in drawer.

What is the actual behavior?

It doesn't. because of evt.stopPropagation();

Any other information you believe would be useful?

I'm writing react wrapper of material-components-web and I'm writing example with drawer.

Since I'm using hash-history with react-router. Page can be routed without re-rendering drawer. So we should close drawer manually when menu in drawer is clicked like this.

<Drawer>
  ...
  <Content>
    <Link to="/button" onClick={() => this.setState(drawerOpen: false)}>
      Button Examples
    </Link>
  </Content>
</Drawer>

Since evt.stopPropagation() is applied in mdc-temporary-drawer__drawer, onClick={...} will never be fired.

[traviskaufman]

Are you sure that evt.stopPropagation is the reason why the click handler doesn't fire?

We call stopPropgatation within the bubbling phase of the event, so unless the <Link> component is a parent of the mdc-temporary-drawer__drawer element, it should activate that event handler before the propagation is stopped.

Could you link us to a codepen / jsbin / plunker with a reproduction of the issue? Feel free to use your implementation in the repro.

[Hardtack]

Oh I misunderstood mechanisms of stopPropagation
I thought that stopPropagation is the reason because it works when I remove interaction handler from temporary-drawer__drawer
I will check again and give you some code that reproduce the issue

[Hardtack]

It seems that react is not working well with native events. Plz see this code
I expected that line 9, evt.stopPropagation(), doesn't affect on handleClick. But it does.

Unfortunately, By the open issue in react repository facebook/react#8693
stopPropagation() should be avoided in react for now.
But MDCTemporaryDrawerFoundation uses it and mdc was not designed only for react.
What should I do? :(

For detail, I created a gist of my usage https://gist.github.com/Hardtack/a1544e48bb77a821904da245adac5082
I'm sorry that I don't know how to setup live online code with my custom packages.

[breakds]

I am running into this problem as well. React router's Link actually directly refresh the page, which is not the desired behavior.

I use ref and addEventListener to work this around - hope this helps those who are facing the same issue.

For example, if what you want is

// Within temporary drawer
<a onClick={...} />

You can simply rewrite as

// Within temporary drawer
<a ref={(theLink) => { theLink.addEventListener("click", ....); } />

[maoziliang]

@breakds It's really existing. Because react event system is not same as the DOM. React listen on the top level of DOM tree and invoke the specified listener, which can reduce DOM event listeners. And the solution here is use ref to resolve, but don't forget to removeEventListener from the DOM element.

[breakds]

@maoziliang Thanks for the note! Yeah the event should be removed with removeEventListener.

[traviskaufman]

Closing as it looks like @breakds has a viable workaround until the issue is fixed upstream.

[flunderpero]

There is the same problem with MDCDialog. Both issues could be fixed (in the sense of "making it work with current React") quite easily. This is for MDCDialogFoundation:

componentClickHandler_(evt) {
  if (evt.target && evt.target.classList && evt.target.classList.contains("mdc-dialog__backdrop")) {
      this.cancel(true);
  }
}

handleDialogClick_(evt) {
  // Just remove the following line ...
  // evt.stopPropagation();
  const {target} = evt;
  if (this.adapter_.eventTargetHasClass(target, cssClasses.ACCEPT_BTN)) {
    this.accept(true);
  } else if (this.adapter_.eventTargetHasClass(target, cssClasses.CANCEL_BTN)) {
    this.cancel(true);
  }
}

I think the same strategy could be applied to MDCTemporaryDrawerFoundation, too.

I could set up a PR for this if needed/wanted.

[mhuebert]

+1 for something like @flunderpero's suggestion.

Maybe I'm missing something, but I think the issue is broader than React, and could affect any application which uses event delegation. I'm not sure why MDC is stopping propagation of events that it isn't concerned with (eg. all onClick events on child nodes).

[amsheehan]

MDC-Dialog and MDC-Temporary-Drawer are modal components, so clicks should not be allowed to propagate down to elements in the background while they are open.

MDC isn't stopping any events on all child nodes in the case of MDC-Dialog or MDC-Temporary-Drawer. What we are doing is stopping events whose targets are the surface or backdrop of the component from propagating down to background elements. This will not have any effect on event delegation within the context of the dialog or temporary-drawer.

Please let us know if there is anything else we can clarify or help out with on our Discord server!

[mhuebert]

What we are doing is stopping events whose targets are the surface or backdrop of the component

Correct me if I'm wrong, but the code here doesn't appear check the target of the event, so it stops propagation of clicks from all elements in the drawer, even if the intent is only to capture events on the surface or backdrop:

//https://github.com/material-components/material-components-web/blob/master/packages/mdc-drawer/slidable/foundation.js#L61
// L61
this.drawerClickHandler_ = (evt) => evt.stopPropagation();
//L91
this.adapter_.registerDrawerInteractionHandler('click', this.drawerClickHandler_);

This means that if we have delegated handling of a relevant click event to an element above the drawer, eg. document root, the application won't see these events. This component may be modal, but events generated from child elements may need to be received by top-level application listeners.

@flunderpero's approach is to verify that a click was on the appropriate target before reacting, which seems reasonable:

if (evt.target && evt.target.classList && evt.target.classList.contains("mdc-dialog__backdrop")) { ...}

* also - it looks like the Discord invite is expired?

Thanks!

[mhuebert]

In #579, we can see this issue affecting another MDC component (Menus use document listeners).

I think we can fix, and it's quite simple:

1. Remove the click handler from the drawer (no need to stopPropagation).
2. Add a check for e.target == e.currentTarget in the "click" handler that closes the component, to ensure that the click was on the background surface and not a child element .

I have made these changes in a fork, here: master...mhuebert:patch-1

Testing locally, it seems to work as expected, and fixes #579. Here is a screen capture of behaviour with the fix:

https://www.youtube.com/watch?v=A5Ngzt7tUrQ

I can submit it as a PR if desired. I wasn't involved in the original implementation, so I'm not sure why adding a listener on the drawer with stopPropagation was considered necessary. Maybe there are other issues related to clicks that need to be considered.

[jamesmfriedman]

Very interested in this issue, just ran into the same problem. It happens on both Persistent and Temporary drawers. I'm using React as well.