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.

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

Fix modal event-listeners during dismiss click #36863

Merged
merged 3 commits into from Sep 7, 2022

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Jul 29, 2022

Partially regression of #36401.

Fixes modal listeners in order to:

  • ignore clicks on scrollbar
    • mousedown, mouseup listen to scrollbar click
    • click ignores scrollbar click
  • ignores clicks that starts inside .modal-dialog, but ends outside of it
  • react on clicks that start & end outside .modal-dialog

closes: #36855

alternative approach #36855 (comment)

@GeoSot GeoSot requested a review from a team as a code owner July 29, 2022 23:46
js/src/modal.js Outdated Show resolved Hide resolved
@GeoSot GeoSot force-pushed the gs-fix-modal-scrollbar-click branch from bdb569b to d6c2aab Compare August 6, 2022 09:14
@GeoSot GeoSot force-pushed the gs-fix-modal-scrollbar-click branch from 9300215 to a41d71f Compare August 31, 2022 13:53
@mdo mdo force-pushed the gs-fix-modal-scrollbar-click branch from a41d71f to d710920 Compare September 1, 2022 01:24
@mdo
Copy link
Member

mdo commented Sep 1, 2022

Is this good to merge @GeoSot or do you need a review from someone?

@GeoSot
Copy link
Member Author

GeoSot commented Sep 1, 2022

I would appreciate a review just to be sure

Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

I prefer this one compared to the alternative approach.

Tested the whole Modals page on macOS with different browsers (Firefox, Chrome, Edge and Safari) and couldn't spot any regressions.

LGTM! 🚀

@GeoSot GeoSot force-pushed the gs-fix-modal-scrollbar-click branch from d710920 to cf6b611 Compare September 7, 2022 07:25
@GeoSot GeoSot force-pushed the gs-fix-modal-scrollbar-click branch from cf6b611 to 3d0c07b Compare September 7, 2022 08:49
@GeoSot GeoSot merged commit 23fb7a7 into main Sep 7, 2022
@GeoSot GeoSot deleted the gs-fix-modal-scrollbar-click branch September 7, 2022 08:56
@ghost ghost mentioned this pull request Sep 12, 2022
3 tasks
@neoteknic
Copy link

@GeoSot Any 5.2.2 release ? Can you disable 5.2.1 because it broke many projets. Have to force 5.2.0 for now.

@folknor
Copy link

folknor commented Oct 1, 2022

@GeoSot Any 5.2.2 release ? Can you disable 5.2.1 because it broke many projets. Have to force 5.2.0 for now.

Yes, please do a 5.2.2 soon. Thank you!

@GeoSot
Copy link
Member Author

GeoSot commented Oct 1, 2022

I can understand you on this. I personally, have done a lot of refactoring to bring our codebase to a better condition, preparing it for v6, and unfortunately sometimes mistakes can be done. I apologize for this.
Furthermore, as a team, we are trying our best, working on our free time, so please make some patience and we will try to have a release soon

@XhmikosR
Copy link
Member

XhmikosR commented Oct 11, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Scrollbar click closes modal with long content
6 participants