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

"show" class is added to modals too late #4761

Closed
philipmountifield opened this issue Feb 13, 2020 · 13 comments · Fixed by #4766, #4777, mariazevedo88/hash-generator-js#35, tghelere/CL#15 or tghelere/CL#17

Comments

@philipmountifield
Copy link
Contributor

@philipmountifield philipmountifield commented Feb 13, 2020

Describe the bug

When a modal is shown, it uses a transition, however at the moment isShow is set in onAfterEnter which doesn't happen until the end of the transition. This means you get sequential transitions or a perceived delay, also the buttons which should show disabled during the transition are always enabled.

Steps to reproduce the bug

  1. Extend the transition time of fade to make it easier to see
  2. Trigger open of a modal
  3. You will see the backdrop fade in
  4. Then after that the show class is added and you see the modal show transition

Expected behavior

The show class needs to be added at the same point as a enter-to class would be added, i.e. the very next frame after enter.

Versions

Libraries:

  • BootstrapVue: 2.4.1
  • Bootstrap: 4.4.1
  • Vue: 2.6.10

Fix

To solve the issue I changed the following here:

onEnter() {
    this.isBlock = true;
    window.requestAnimationFrame(() => {
        window.requestAnimationFrame(() => {
            this.isShow = true;
        });
    });
}

And also remove this.isShow = true; from onAfterEnter

Ideally I'd like to use nextFrame(() => {this.isShow = true;}) from the built in transition lib in Vue but I couldn't see a way to import that definition. https://github.com/vuejs/vue/blob/4de4649d9637262a9b007720b59f80ac72a5620c/src/platforms/web/runtime/transition-util.js#L67

Demo link for problem and solution

https://codepen.io/sonicisthebest/pen/LYVGpvJ

@philipmountifield
Copy link
Contributor Author

@philipmountifield philipmountifield commented Feb 13, 2020

Apologies for the partial submit initially, form glitch!

@philipmountifield
Copy link
Contributor Author

@philipmountifield philipmountifield commented Feb 13, 2020

Added demo for problem and solution as a codepen.

@tmorehouse
Copy link
Member

@tmorehouse tmorehouse commented Feb 13, 2020

The show is added "twice", once as the enter-to-class (which Vue adds after a requestAnimationFrame when the transition starts, but then removes when the transition completes. We then manually add the show class with on-after-enter hook to keep it there.

@philipmountifield
Copy link
Contributor Author

@philipmountifield philipmountifield commented Feb 14, 2020

That isn't true for the modal which has it's own transition which adds no classes at all.

modal = h(
    'transition',
    {
      props: {
        enterClass: '',
        enterToClass: '',
        enterActiveClass: '',
        leaveClass: '',
        leaveActiveClass: '',
        leaveToClass: ''
      },
      on: {
        beforeEnter: this.onBeforeEnter,
        enter: this.onEnter,
        afterEnter: this.onAfterEnter,
        beforeLeave: this.onBeforeLeave,
        leave: this.onLeave,
        afterLeave: this.onAfterLeave
      }
    },
    [modal]
  )

See here

enterToClass: '',

You can see what I mean in the reproduction link also https://codepen.io/sonicisthebest/pen/LYVGpvJ

@tmorehouse
Copy link
Member

@tmorehouse tmorehouse commented Feb 14, 2020

Ah, yes...

A fix is on it's way.

@jacobmllr95 jacobmllr95 added this to To do in 2.4.2 via automation Feb 14, 2020
2.4.2 automation moved this from To do to Done Feb 14, 2020
@philipmountifield
Copy link
Contributor Author

@philipmountifield philipmountifield commented Feb 15, 2020

Thanks chaps! Very quick turn around :)

@philipmountifield
Copy link
Contributor Author

@philipmountifield philipmountifield commented Feb 17, 2020

@tmorehouse I've just been testing this in the full nuxt app rather than the codepen, and while calling requestAF a single time seems to sufficient to make the basic codepen example work correctly, when the modal is embedded within a more complex app I still need to call requestAF twice to ensure the transitions work. Using vanilla 2.4.2 I'm now seeing no transitions on modal opening.

@philipmountifield
Copy link
Contributor Author

@philipmountifield philipmountifield commented Feb 17, 2020

I just had a look at the modal docs example too here https://bootstrap-vue.org/docs/components/modal and I found that some of them do the show transition, and others don't when you click the example buttons.

I found these ones didn't generally show the transition, but even they sometimes do and sometimes don't:

https://bootstrap-vue.org/docs/components/modal#using-thisbvmodalshow-and-thisbvmodalhide-instance-methods
https://bootstrap-vue.org/docs/components/modal#using-show-hide-and-toggle-component-methods
https://bootstrap-vue.org/docs/components/modal#variants
https://bootstrap-vue.org/docs/components/modal#example-modal-using-custom-scoped-slots

@jacobmllr95
Copy link
Member

@jacobmllr95 jacobmllr95 commented Feb 17, 2020

@tmorehouse Maybe we need to add a second requestAF() or $nextTick() in there to ensure the transition is shown.

@jacobmllr95 jacobmllr95 reopened this Feb 17, 2020
@philipmountifield
Copy link
Contributor Author

@philipmountifield philipmountifield commented Feb 17, 2020

Yes that's what I've done locally, and it seems to be the only way to guarantee the right behaviour. I assume this is part of the reason why the built in Vue transition API does that. I think because requestAnimationFrame is called "before the next repaint" the only way to ensure it's the next frame is to wait for the requestAnimationFrame second call to come in, then you can be certain.

@philipmountifield
Copy link
Contributor Author

@philipmountifield philipmountifield commented Feb 17, 2020

Also in previous testing I tried a few combinations of nextTick and requestAnimationFrame and found that the only consist working combo was 2x requestAnimationFrame

@jacobmllr95
Copy link
Member

@jacobmllr95 jacobmllr95 commented Feb 17, 2020

@philipmountifield Thanks for the research! Do you want't to come up with a PR to add the second requestAF() in yourself?

@philipmountifield
Copy link
Contributor Author

@philipmountifield philipmountifield commented Feb 17, 2020

Sure I'll take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment