feat(modal): add scale animation when static backdrop is clicked

[jnizet]

fix #3768

Note: this is my first PR on animations, and I don't really master everything.
There are 3 points that I'm not sure about in the submitted code:

• should I pass 'continue' or 'stop'. I chose 'continue', and it seems to behave correctly, and the same way as the native bootstrap modal, but maybe I'm missing something

• in the unit tests, I don't know how to test that the modal-static class disappears at the end of the transition: nothing is emitted when that happens, and I didn't want to introduce an output only useful for tests. (see TODO in the test)

• I'm not sure exactly what the reduceMotion flag is supposed to do. I have noticed that the modal-static is not added when reduceMotion is true, and I guess it's a feature, but I'm not sure. (see TODO in the test)

• read and followed the CONTRIBUTING.md and DEVELOPER.md guide.

• built and tested the changes locally.

• added/updated any applicable tests.

• added/updated any applicable API documentation.

• added/updated any applicable demos.

[codecov-commenter]

Codecov Report

Merging #3771 into animations will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@              Coverage Diff               @@
##           animations    #3771      +/-   ##
==============================================
+ Coverage       91.78%   91.86%   +0.07%     
==============================================
  Files             107      107              
  Lines            3127     3133       +6     
  Branches          565      566       +1     
==============================================
+ Hits             2870     2878       +8     
+ Misses            189      188       -1     
+ Partials           68       67       -1     

Flag	Coverage Δ
#e2e	55.53% <100.00%> (+0.08%)	⬆️
#unit	89.05% <100.00%> (+0.18%)	⬆️

Impacted Files	Coverage Δ
src/modal/modal-window.ts	98.73% <100.00%> (+2.84%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e941d7...d6dbf82. Read the comment docs.

[maxokorokov]

Hey, @jnizet!

Cool, thanks for the PR!

LGTM for now, just:

• bump should also be triggered on Esc key press
• maybe move (click): bump($event) to the _enableEventHandling()
• please, clean up the // TODO: comments in tests (see my ideas below)

should I pass 'continue' or 'stop'. I chose 'continue', and it seems to behave correctly, and the same way as the native bootstrap modal, but maybe I'm missing something

The difference is when you start a new transition on an element that has another transition running already:

• 'continue' → will silently complete a new transition, emit nothing and continue running existing one
• 'stop' → will stop already running transition and go with the new one. You can also have context that would be kept between the two transitions.

Ex. for the accordion you might want to revert collapsing transition → 'stop'; in this 'bumping' modal case, I'd say that 'continue' seems fine (ex. in case of double clicks on the backdrop, second click will trigger a transition that will simply de ignored)

Here is the relevant piece of code → https://github.com/ng-bootstrap/ng-bootstrap/blob/animations/src/util/transition/ngbTransition.ts#L44-L52

in the unit tests, I don't know how to test that the modal-static class disappears at the end of the transition: nothing is emitted when that happens, and I didn't want to introduce an output only useful for tests. (see TODO in the test)

Yeah... not sure how to test this either. Apart from reading the transition duration with this → https://github.com/ng-bootstrap/ng-bootstrap/blob/animations/src/util/transition/util.ts#L1 and scheduling the check for this time + some delay. But I'm afraid this would be unstable.

I suppose you can also keep the reference to the modal element and check the class after modalRef.hidden emits. It would be detached at this point, but should still be accessible I guess. Not sure though.

I don't think we should introduce an output for this either.

I'm not sure exactly what the reduceMotion flag is supposed to do. I have noticed that the modal-static is not added when reduceMotion is true, and I guess it's a feature, but I'm not sure. (see TODO in the test)

The 'reduceMotion' will "simulate" during testing what bootstrap does with reduce motion media query, by setting all transtiions to 'none' → https://github.com/ng-bootstrap/ng-bootstrap/blob/animations/src/test/test-styles.css#L22-L24

Essentially all processing in the ngbRunTransition will become synchronous as if animation = false is turned on. The relevant code is here → https://github.com/ng-bootstrap/ng-bootstrap/blob/animations/src/util/transition/ngbTransition.ts#L59-L63

It will trigger start function and end function synchronously, so your check is fine:

if (reduceMotion) {
  // TODO check if this is normal or not
  expect(modalEl).not.toHaveClass('modal-static');
}

Everything will be done in the modalEl.click() handler.

---

[jnizet]

Thanks for the review @maxokorokov.

Since ngbRunTransition runs everything synchronously (i.e. the start and the end functions) when reduceMotion is true, my test already checks that the modal-static class disappears at the end of the transition, so I guess I'm fine there.

I'll do the remaining stuff, and add tests for the Escape key handling.
I'll submit a new commit to help with the review, but if it suits you, feel free to squash the two commits before merging of course.

[benouat]

I am 🆗 with the change, but a bit puzzled I must confess.
I would expect to see some else in here, and I don't.

Let me explain, when we are using backdrop="static", both click and Esc are noop.
So what about:

fromEvent<KeyboardEvent>(nativeElement, 'keydown')
  .pipe( /* ... */ )
  .subscribe(event => {
    if (this.backdrop === "static") {
      ngbRunTransition( /* ... */ )
    } else if (this.keyboard) {
      requestAnimationFrame( /* ... */ )
    }
  })

And kind of the same story for the click part...

Am I totally wrong on that one ?

[maxokorokov]

LGTM, but I think I'm with @benouat on the code structure comment.

I would prefer this as more readable, because all this.backdrop checks are in one place and clearly visible and _bumpBackdrop is just an utility fn to start transition with no logic:

// ESC handling
if (this.backdrop === 'static') {
  this._bumpBackdrop();
} else if (this.keyboard) {
  requestAnimationFrame(() => {
    // ...
  });
}

// click handing
if (nativeElement === target) {
  if (this.backdrop === 'static') {
    this._bumpBackdrop();
  } else if (this.backdrop === true && !preventClose) {
    // ...
  }
}

@jnizet

[jnizet]

I'm fine doing that, but it will change the behevior.
Currently, the if keyboard is true, pressing escape dismisses the modal whatever the backdrop value is.
With your suggested change, it will not dismiss the modal anymore if the backdrop is static.

I'm not sure what is the correct behavior, but my goal here was to introduce the animation, not to introduce a change of behavior (which could be seen as a breaking change).

Please tell me if this else should be added or not. To keep the current behavior (and to avoid adding the bump animation when the modal is dismissed), the code should be

// ESC handling
if (this.keyboard) {
  requestAnimationFrame(() => {
    // ...
  });
} else if (this.backdrop === 'static') {
  this._bumpBackdrop();
} 

// click handing
if (nativeElement === target) {
  if (this.backdrop === 'static') {
    this._bumpBackdrop();
  } else if (this.backdrop === true && !preventClose) {
    // ...
  }
}

@maxokorokov @benouat

[benouat]

Currently, the if keyboard is true, pressing escape dismisses the modal whatever the backdrop value is.
With your suggested change, it will not dismiss the modal anymore if the backdrop is static.

I'm not sure what is the correct behavior, but my goal here was to introduce the animation, not to introduce a change of behavior (which could be seen as a breaking change).

Correct, we apparently did not implement it correctly in the first place!
I just checked on BS side, backdrop static is actually a real modal dialog in a sense that only actions coming from the modal can close it. Esc does not work, neither does click.

So, return of the never ending story ... 🤷‍♂️ what should we do ? Non backward compatible change ? or simply bug fixing a broken behavior that should never have been here in the first place ?

Back to reality here! I would say we do not change the behaviour in this animation PR introduction. We will then fix it/change it on master once animations are merged.

[jnizet]

OK, thanks. I will then use the code I suggested above to keep the current buggy behavior.

[maxokorokov]

LGTM, thanks!