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

Modals dismissed by mouse release when trying to scroll the window #3518

Closed
penguintheorem opened this issue Dec 16, 2019 · 3 comments · Fixed by #3531
Closed

Modals dismissed by mouse release when trying to scroll the window #3518

penguintheorem opened this issue Dec 16, 2019 · 3 comments · Fixed by #3531

Comments

@penguintheorem
Copy link

penguintheorem commented Dec 16, 2019

Bug description:

When you try to scroll the modal window by clicking and dragging the scrollbar, the modal gets dismissed on mouse release. The following should clarify the issue:

chrome-capture (21)

Link to minimally-working StackBlitz that reproduces the issue:

here the example

Versions of Angular, ng-bootstrap and Bootstrap:

Angular: 8.2.13

ng-bootstrap: 5.1.4

Bootstrap: 4.3.1

@benouat
Copy link
Member

benouat commented Dec 17, 2019

Hey @penguintheorem , thanks to reach out !

Regarding the issue you report, I am a little bit confused. Modal are designed to actually hide the window scrollbar when opened.

I can't even reproduce it on your stackblitz. As soon as modal is opened, I don't have any scrollbar I can grab to scroll around...


EDIT: Ok I get it, you have a very narrow screen, which is smaller than the modal height itself!

@peterblazejewicz
Copy link
Contributor

@benouat
Can you check below diff change?

diff --git a/src/modal/modal-window.ts b/src/modal/modal-window.ts
index 3554383..4a06d47 100644
--- a/src/modal/modal-window.ts
+++ b/src/modal/modal-window.ts
@@ -65,13 +65,10 @@ export class NgbModalWindow implements OnInit,
                        }
                      }));
 
-      const mouseDowns$ = fromEvent<MouseEvent>(this._elRef.nativeElement, 'mousedown')
-                              .pipe(
-                                  takeUntil(this.dismissEvent),
-                                  map(e => this.backdrop === true && this._elRef.nativeElement === e.target));
-
-      fromEvent<MouseEvent>(this._elRef.nativeElement, 'mouseup')
-          .pipe(takeUntil(this.dismissEvent), withLatestFrom(mouseDowns$), filter(([_, shouldClose]) => shouldClose))
+      fromEvent<Event>(this._elRef.nativeElement, 'click')
+          .pipe(
+              filter(e => this.backdrop === true && this._elRef.nativeElement === e.target),
+              takeUntil(this.dismissEvent))
           .subscribe(() => this._zone.run(() => this.dismiss(ModalDismissReasons.BACKDROP_CLICK)));
     });
   }

or something along those lines. The problem occurs when the cotnent of the modal upscale the modal body in such way, the scrollbars appears. When scrolled via scrollbars, current mouse-events are triggerred, dismissing the modal.

@benouat
Copy link
Member

benouat commented Dec 20, 2019

Yes this is it ☹️
autoclose in modal is a specific one, we do not reuse the one we have for popup (datepicker, dropdown and typeahead) and unfortunately the implementation rely on mousedown

So when the modal itself is bigger than the viewport, we do a scrollbar, a legit one, which leads to this bug. when you click it (you mousedown it...) so we think we are outside

It's a actual real bug that need a proper fix.

@maxokorokov maxokorokov added this to the 5.1.5 milestone Jan 6, 2020
maxokorokov added a commit to maxokorokov/ng-bootstrap that referenced this issue Jan 6, 2020
maxokorokov added a commit to maxokorokov/ng-bootstrap that referenced this issue Jan 7, 2020
maxokorokov added a commit to maxokorokov/ng-bootstrap that referenced this issue Jan 7, 2020
maxokorokov added a commit that referenced this issue Jan 7, 2020
Using `mousedown` - `mouseup` - `click` combination to handle closing correctly.

Fixes #3518
maxokorokov added a commit that referenced this issue Jan 7, 2020
Using `mousedown` - `mouseup` - `click` combination to handle closing correctly.

Fixes #3518
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants