UX: Only close modal for full 'click' events outside (#23566)

Previously we were using 'mouseup', which meant that if you started the click inside, and then dragged to outside the modal, it would still close. This kind of dragging action is common when selecting text, and having it close the modal can be very frustrating.

Simply switching to a 'click' listener doesn't totally solve the problem, because when a click event involves dragging from one element to another, the browser will fire the event on "the most specific ancestor element that contained both elements". For modals, the most specific common ancestor was still the `modal-middle-container`, which would cause the modal to close.

Therefore, this commit sets the modal containers to have `pointer-events: none`, and sets up the click listener on the `.modal-backdrop` element, which is **adjacent** to the modal in the DOM. That means that click events fired on any ancestors of the modal will not accidentally trigger closure.
This commit is contained in:
David Taylor 2023-09-25 14:23:59 +01:00 committed by GitHub
parent 6ad810c991
commit 88ec2320dc
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 13 additions and 12 deletions

View File

@ -19,7 +19,6 @@
...attributes
{{did-insert this.setupListeners}}
{{will-destroy this.cleanupListeners}}
{{on "mouseup" this.handleMouseUp}}
>
<div class="modal-outer-container">
<div class="modal-middle-container">
@ -98,6 +97,6 @@
</div>
</this.dynamicElement>
{{#unless @inline}}
<div class="modal-backdrop"></div>
<div class="modal-backdrop" {{on "click" this.handleWrapperClick}}></div>
{{/unless}}
</ConditionalInElement>

View File

@ -60,7 +60,7 @@ export default class DModal extends Component {
}
@action
handleMouseUp(e) {
handleWrapperClick(e) {
if (e.button !== 0) {
return; // Non-default mouse button
}
@ -69,14 +69,9 @@ export default class DModal extends Component {
return;
}
if (
e.target.classList.contains("modal-middle-container") ||
e.target.classList.contains("modal-outer-container")
) {
return this.args.closeModal?.({
initiatedBy: CLOSE_INITIATED_BY_CLICK_OUTSIDE,
});
}
return this.args.closeModal?.({
initiatedBy: CLOSE_INITIATED_BY_CLICK_OUTSIDE,
});
}
@action

View File

@ -54,7 +54,7 @@ acceptance("Modal service: component-based API", function () {
assert.dom(".d-modal .title h3").hasText("Hello World");
assert.dom(".d-modal .modal-body").hasText("Modal content is working");
await click(".modal-outer-container");
await click(".modal-backdrop");
assert.dom(".d-modal").doesNotExist("disappears on click outside");
assert.deepEqual(
await promise,

View File

@ -1,5 +1,10 @@
// Modal wrappers
.fixed-modal:not(#discourse-modal) {
// new <DModal /> only, not <DModalLegacy />
pointer-events: none; // Allow clicks through wrappers so they hit the adjacent backdrop element
}
.modal-outer-container {
width: 100%;
height: 100%;
@ -12,6 +17,7 @@
}
.modal-inner-container {
pointer-events: auto;
--modal-max-width: 47em; // set in ems to scale with user font-size
box-sizing: border-box;
flex: 0 1 auto;
@ -84,6 +90,7 @@
}
.modal-backdrop {
user-select: none;
position: fixed;
top: 0;
right: 0;