[fixed]:1015 - Fixed closing by click on layout #1018

ErikDeviashin · 2023-04-24T10:32:28Z

Changes proposed:

Fix incorrect behavior when you need to click twice on the overlay to close modal window if you click on something with event.stopPropagation() inside content block and add a test case for that.
Fix test cases for the overlay MD -> content MU && content MD -> overlay MU cases because they don't work correctly without the "click" itself.

Upgrade Path (for changed or removed APIs): -

Acceptance Checklist:

The commit message follows the guidelines in CONTRIBUTING.md.
Documentation (README.md) and examples have been updated as needed.
If this is a code change, a spec testing the functionality has been added.
If the commit message has [changed] or [removed], there is an upgrade path above.

- Fixed incorrect behavior when you need to click twice on the overlay to close modal window if you click on something with event.stopPropagation() inside content block. - Wrote a test case for closing modal by click on the overlay after click on something placed inside modal and containing stopPropagation for the "click" event. Fixed test cases for the overlay MD -> content MU && content MD -> overlay MU cases.

diasbruno · 2023-04-24T12:38:53Z

specs/Modal.events.spec.js

@@ -222,6 +222,7 @@ export default () => {
      withModal(props, null, modal => {
        mouseDownAt(moverlay(modal));
        mouseUpAt(mcontent(modal));
+        clickAt(mcontent(modal));


Here, mouse down and click at has the effect of a double click.

diasbruno · 2023-04-24T12:39:03Z

specs/Modal.events.spec.js

@@ -236,9 +237,40 @@ export default () => {
      withModal(props, null, modal => {
        mouseDownAt(mcontent(modal));
        mouseUpAt(moverlay(modal));
+        clickAt(moverlay(modal));


I tested this again by simply adding the log to every content and layout event. And right now only the mouseUp and mouseDown events fire, but in the code they don't have the logic to close the modal, so we have false "true" results, because of course the modal can't be closed.

For example I created this reference. As you can see in lines 18-19, I remove content mouseUp and mouseDown events and after that in browser window we have both issues with modal closing. But if I comment out this methods in source the tests will still give "true" results.

But if we add clickAt(moverlay(modal)) then we will have the correct results, which match the real results I got on the actual product before (and during) these tests, and than we will be able to catch these errors.

I hope this proofs is sufficient. Let me know what you think about it.
I'll fix line 225 because I used clickAt mcontent instead of moverlay but this isn't correct as I saw during the tests.

Ah now I remember it...

The mouse down and up are to test the scenario where you click and hold on a content and release (mouse up) on the overlay.

Yes that's it!
It just didn't work when I ran the tests, so I compared it to real behavior and suggesting a way to fix the tests. Now they works at least for me :)

I guess in the end the result is correct, but I'm not sure if the test "examplifies" the behavior.

I need to think about it.

This one too.

If I press down on the content, drag to the overlay, it must not close the modal.

Suggested change

clickAt(moverlay(modal));

- Fixed test case for the overlay MD -> content MU.

diasbruno · 2023-04-29T18:27:02Z

@ErikDeviashin Can you please rebase your PR please? Jut fixed the action issue.

ErikDeviashin · 2023-04-29T19:13:05Z

@ErikDeviashin Can you please rebase your PR please? Jut fixed the action issue.

@diasbruno Sure, did it

diasbruno · 2023-04-29T21:03:27Z

specs/Modal.events.spec.js

@@ -222,6 +222,7 @@ export default () => {
      withModal(props, null, modal => {
        mouseDownAt(moverlay(modal));
        mouseUpAt(mcontent(modal));
+        clickAt(moverlay(modal));


Here is the behavoir I was talking about.

This clickAt shouldn't be here.

Suggested change

clickAt(moverlay(modal));

diasbruno · 2023-04-29T21:06:26Z

specs/Modal.events.spec.js

+          mouseDownAt(innerButton);
+          mouseUpAt(innerButton);


Suggested change

mouseDownAt(innerButton);

mouseUpAt(innerButton);

diasbruno · 2023-04-29T21:06:47Z

specs/Modal.events.spec.js

+          mouseDownAt(moverlay(modal));
+          mouseUpAt(moverlay(modal));


Suggested change

mouseDownAt(moverlay(modal));

mouseUpAt(moverlay(modal));

diasbruno · 2023-04-29T21:08:07Z

I've made a few suggestions, I think that is what we are expecting from your new test.

diasbruno · 2023-04-29T21:11:20Z

Those 2 other tests, they need to stay the same to guarantee the behavoir when dragging from the content -> overlay and overlay -> content.

ErikDeviashin · 2023-05-06T12:29:37Z

Hi, @diasbruno, sorry fore disappearing! Intense week.

I almost ready to do whatever you say, but I just want to clarify one last time:
Without handling all the mouse events, the tests will not have behavior close to real. Here, I made one more example for you, just with logs on every events for the overlay and for the content. You can drag the mouse from the content to the overlay and vice versa and you will see that the onClick event is always fired on overlay in this scenario. That's why I think we need to handle this in the tests.

I can remove all events from tests, but then the tests will gave wrong results and wouldn't catch errors that they should catch as it was in case that I described earlier.

In case of the new test will also be incorrect behavior, because the very problem of 2 clicks on overlay appears from the logic in the mouseDown event. If this event is not in the test, the test will show that behavior is correct even now without my fixes but as we know, in fact there is a problem now.

I won't push anymore, but I do think it would be better to fix the tests.

diasbruno · 2023-05-06T23:17:22Z

Ok. I'll have a look when I have time.
Thanks so much for taking the time to help with this issue.

khmelevskii · 2023-06-17T21:35:21Z

@diasbruno it would be very cool if this fix will be merged to master. I have the same problem and I can't see any workaround to fix it

diasbruno · 2023-06-17T21:57:16Z

@khmelevskii This patch needs a better look to see if it is right...
If you have time, you can take this patch and give it a try...

What I expect:

When press and hold on the overlay and release on the content, it must not close
When press and hold on the content and release on the overlay, it must not close
See https://codesandbox.io/s/react-modal-layout-issue-xgfsvk and try to replicate it

khmelevskii · 2023-06-18T00:34:05Z

Thank you @diasbruno ! it will be pretty difficult to find time for this soon, but thank you for sharing correct expectations.

ErikDeviashin · 2023-06-23T11:49:31Z

@diasbruno @khmelevskii
I had to change the solution in our project to just using an extra variable like
this.preventOverlayMouseDown = false (ModalPortal.js 285:42)

This is used like
this.preventOverlayMouseDown = true; (in handleContentOnMouseDown func on ModalPortal.js 236:44 line)
and
if (_this.preventOverlayMouseDown) { _this.preventOverlayMouseDown = false; } else { _this.shouldClose = null; }
(In handleOverlayOnMouseDown func on ModalPortal.js 223:7 line)

This is better just because using event.stopPropagation(); broke our tooltips a bit, so it might conflict with different libs that use the mouseDown event.

@khmelevskii If you need some quick solution look at the patch-package library because as you can see it can take a while before it gets fixed. I am using patch-package in our project to apply the changes written above

khmelevskii · 2023-06-23T18:54:44Z

Thank you @diasbruno ! I will try

see2ever · 2024-01-04T11:51:18Z

@diasbruno @khmelevskii I had to change the solution in our project to just using an extra variable like this.preventOverlayMouseDown = false (ModalPortal.js 285:42)

This is used like this.preventOverlayMouseDown = true; (in handleContentOnMouseDown func on ModalPortal.js 236:44 line) and if (_this.preventOverlayMouseDown) { _this.preventOverlayMouseDown = false; } else { _this.shouldClose = null; } (In handleOverlayOnMouseDown func on ModalPortal.js 223:7 line)

This is better just because using event.stopPropagation(); broke our tooltips a bit, so it might conflict with different libs that use the mouseDown event.

@khmelevskii If you need some quick solution look at the patch-package library because as you can see it can take a while before it gets fixed. I am using patch-package in our project to apply the changes written above

because those line number are not match the source file, it would be more clear if the patch file pasted here.

diasbruno reviewed Apr 24, 2023

View reviewed changes

[fixed]:1015

32a395a

- Fixed test case for the overlay MD -> content MU.

ErikDeviashin requested a review from diasbruno April 24, 2023 14:44

Merge branch 'reactjs:master' into fix/F1015-layout-double-click-issue

492cb48

diasbruno reviewed Apr 29, 2023

View reviewed changes

diasbruno closed this May 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fixed]:1015 - Fixed closing by click on layout #1018

[fixed]:1015 - Fixed closing by click on layout #1018

ErikDeviashin commented Apr 24, 2023

diasbruno Apr 24, 2023

diasbruno Apr 24, 2023

ErikDeviashin Apr 24, 2023 •

edited

Loading

diasbruno Apr 29, 2023

ErikDeviashin Apr 29, 2023 •

edited

Loading

diasbruno Apr 29, 2023

diasbruno Apr 29, 2023

diasbruno Apr 29, 2023

diasbruno commented Apr 29, 2023

ErikDeviashin commented Apr 29, 2023 •

edited

Loading

diasbruno Apr 29, 2023

diasbruno Apr 29, 2023

diasbruno Apr 29, 2023

diasbruno Apr 29, 2023 •

edited

Loading

diasbruno commented Apr 29, 2023

diasbruno commented Apr 29, 2023

ErikDeviashin commented May 6, 2023 •

edited

Loading

diasbruno commented May 6, 2023

khmelevskii commented Jun 17, 2023

diasbruno commented Jun 17, 2023 •

edited

Loading

khmelevskii commented Jun 18, 2023

ErikDeviashin commented Jun 23, 2023

khmelevskii commented Jun 23, 2023

see2ever commented Jan 4, 2024

[fixed]:1015 - Fixed closing by click on layout #1018

[fixed]:1015 - Fixed closing by click on layout #1018

Conversation

ErikDeviashin commented Apr 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikDeviashin Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErikDeviashin Apr 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diasbruno commented Apr 29, 2023

ErikDeviashin commented Apr 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diasbruno Apr 29, 2023 • edited Loading

Choose a reason for hiding this comment

diasbruno commented Apr 29, 2023

diasbruno commented Apr 29, 2023

ErikDeviashin commented May 6, 2023 • edited Loading

diasbruno commented May 6, 2023

khmelevskii commented Jun 17, 2023

diasbruno commented Jun 17, 2023 • edited Loading

khmelevskii commented Jun 18, 2023

ErikDeviashin commented Jun 23, 2023

khmelevskii commented Jun 23, 2023

see2ever commented Jan 4, 2024

ErikDeviashin Apr 24, 2023 •

edited

Loading

ErikDeviashin Apr 29, 2023 •

edited

Loading

ErikDeviashin commented Apr 29, 2023 •

edited

Loading

diasbruno Apr 29, 2023 •

edited

Loading

ErikDeviashin commented May 6, 2023 •

edited

Loading

diasbruno commented Jun 17, 2023 •

edited

Loading