fix(dialog): allow click events to propagate #869
Conversation
|
Hey @jonathanzong, Thanks for the PR! It looks like some of the tests are failing. As soon as that gets fixed up I'll review. From the output in Travis CI, it looks like there are two tests failing:
The first one we can remove since we're scrapping
fwiw, if you run a quick Thanks again! |
Codecov Report
@@ Coverage Diff @@
## master #869 +/- ##
==========================================
+ Coverage 99.93% 99.93% +<.01%
==========================================
Files 68 68
Lines 3270 3271 +1
Branches 402 403 +1
==========================================
+ Hits 3268 3269 +1
Misses 2 2
Continue to review full report at Codecov.
|
|
Upon further inspection I'm realizing that the intent of the removed test is to make sure click events don't propagate to items behind / unrelated to the dialog (see your comment here in #225). My solution is to stop propagation when the |
|
Hi @amsheehan, I just updated the tests and the CI checks have passed, would appreciate your feedback! |
This isn't emulating the natural dialog element though. Natural HTML |
packages/mdc-dialog/foundation.js
Outdated
| @@ -52,7 +52,14 @@ export default class MDCDialogFoundation extends MDCFoundation { | |||
| constructor(adapter) { | |||
| super(Object.assign(MDCDialogFoundation.defaultAdapter, adapter)); | |||
| this.isOpen_ = false; | |||
| this.componentClickHandler_ = () => this.cancel(true); | |||
| this.componentClickHandler_ = (evt) => { | |||
| if (this.adapter_.eventTargetHasClass(evt.currentTarget, cssClasses.ROOT)) { | |||
There was a problem hiding this comment.
As Garbee mentioned, events should be allowed to propagate down to the document.
| @@ -185,14 +185,17 @@ test('#cancel calls notifyCancel when shouldNotify is set to true', () => { | |||
| td.verify(mockAdapter.notifyCancel()); | |||
| }); | |||
|
|
|||
| test('on dialog surface click calls evt.stopPropagation() to prevent click from propagating to background el', () => { | |||
| test('on dialog surface click calls evt.stopPropagation() to prevent click from propagating past the root', () => { | |||
There was a problem hiding this comment.
Once the event is allowed to reach the document, this test should be removed.
|
|
||
| td.verify(mockAdapter.removeClass(cssClasses.OPEN)); | ||
| td.verify(mockAdapter.notifyCancel()); | ||
| }); | ||
|
|
||
| test('on click does not close or notify cancellation if event target is not the backdrop', () => { |
There was a problem hiding this comment.
nit: might be worth mentioning for future devs that this effectively means a click on the surface does not close the dialog
| handlers.click(evt); | ||
|
|
||
| td.verify(mockAdapter.removeClass(cssClasses.OPEN), {times: 0}); | ||
| td.verify(mockAdapter.notifyCancel(), {times: 0}); |
There was a problem hiding this comment.
It will be enough to test that the removeClass() adapter method isn't called.
|
Thanks @amsheehan, yes I did sanity check this on Chrome/Safari/Firefox and events do propagate. |
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
This PR fixes #794. I implemented the short fix proposed by @amsheehan in that thread and confirmed that listeners on the list items receive click events. cc @jverkoey