Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cf223f7
[fixed]:1015
ErikDeviashin Apr 24, 2023
32a395a
[fixed]:1015
ErikDeviashin Apr 24, 2023
492cb48
Merge branch 'reactjs:master' into fix/F1015-layout-double-click-issue
ErikDeviashin Apr 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions specs/Modal.events.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ export default () => {
withModal(props, null, modal => {
mouseDownAt(moverlay(modal));
mouseUpAt(mcontent(modal));
clickAt(moverlay(modal));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the behavoir I was talking about.

This clickAt shouldn't be here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
clickAt(moverlay(modal));

requestCloseCallback.called.should.not.be.ok();
});
});
Expand All @@ -236,9 +237,40 @@ export default () => {
withModal(props, null, modal => {
mouseDownAt(mcontent(modal));
mouseUpAt(moverlay(modal));
clickAt(moverlay(modal));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Author

@ErikDeviashin ErikDeviashin Apr 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Screenshot 2023-04-24 at 16 39 54

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.

Screenshot 2023-04-24 at 16 50 02

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.

Screenshot 2023-04-24 at 17 09 58

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@ErikDeviashin ErikDeviashin Apr 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one too.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
clickAt(moverlay(modal));

requestCloseCallback.called.should.not.be.ok();
});
});

it("click on button containing stopPropagation inside modal shouldn't block closing", () => {
const requestCloseCallback = sinon.spy();
let innerButton = null;
let innerButtonRef = ref => {
innerButton = ref;
};
function click(event) {
event.stopPropagation();
}

withModal(
{
isOpen: true,
onRequestClose: requestCloseCallback
},
<button ref={innerButtonRef} onClick={click} />,
modal => {
// imitate regular click with all mouse events on button inside modal
mouseDownAt(innerButton);
mouseUpAt(innerButton);
Comment on lines +263 to +264
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
mouseDownAt(innerButton);
mouseUpAt(innerButton);

clickAt(innerButton);
// imitate regular click with all mouse events on modal overlay
mouseDownAt(moverlay(modal));
mouseUpAt(moverlay(modal));
Comment on lines +267 to +268
Copy link
Collaborator

@diasbruno diasbruno Apr 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
mouseDownAt(moverlay(modal));
mouseUpAt(moverlay(modal));

clickAt(moverlay(modal));
requestCloseCallback.called.should.be.ok();
}
);
});
});

it("should not stop event propagation", () => {
Expand Down
4 changes: 3 additions & 1 deletion src/components/ModalPortal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,13 +322,15 @@ export default class ModalPortal extends Component {
if (!this.props.shouldCloseOnOverlayClick && event.target == this.overlay) {
event.preventDefault();
}
this.shouldClose = null;
};

handleContentOnClick = () => {
this.shouldClose = false;
};

handleContentOnMouseDown = () => {
handleContentOnMouseDown = event => {
event.stopPropagation();
this.shouldClose = false;
};

Expand Down