Skip to content

complete() never gets called in modalmanager if there is a fast transition #58

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

Closed
wants to merge 2 commits into from

Conversation

thurloat
Copy link

Description

complete() in appendModal doesn't get fired after the modal appears when transitions are enabled and the transition finishes before the JS reaches that point. It does however fire complete() and therefore setFocus as soon as a user triggers any webkit animation (i.e. textbox highlight on focus in bootstrap), then it re-focuses everything again causing weird input focussing bugs. Since it is a .one binding, it doesn't happen over and over -- only annoyingly makes you select an input twice or selects the first element when you click on the second one.

The Fix

If the transition is already complete (element is shown) then fire complete immediately.

@jschr
Copy link
Owner

jschr commented Jan 23, 2013

Ah thanks, just spend 5 minutes googling what .is('hidden') would do :).

Is it easy to recreate a jsfiddle showing the problem? Just trying to wrap my head around what exactly is going on.

Are you trying to set focus to an input when the modal is opened and setFocus is overriding it?

@thurloat
Copy link
Author

I'll see what I can do to isolate the bug.

here's a more concise rundown of what I believe is going on:

  • the modal manager appends the modal
  • transitions are enabled
  • the transition completes before modalmanager binds the webkitTransitionEnd listener to the modal
  • therefore the complete method doesn't get called once the modal appears.

...wait

  • user clicks an element in the modal
  • bootstrap uses an animation to add the glow around the textbox, thereby finally triggering webkitTransitionEnd
  • the regular focus happens
  • complete() finally gets called and triggers setFocus()
  • it overrides the focus with either modal.$element or the element matching options.focusOn

more clear I hope.

@jschr
Copy link
Owner

jschr commented Jan 23, 2013

Thanks. My only hesitation is now the 'shown' event is called right away even if the modal is still transitioning wouldn't it? This is would be a change in the way bootstrap's default modals work and would breaking change at least in my application.

I haven't run into this bug yet in my application so I'm curious as to what exactly would be cause the transitionEnd event not to fire at all.

@jschr
Copy link
Owner

jschr commented Jan 23, 2013

I wonder if it's related to commit dffb499.

Are you able to try adding this back in:

if (transition) {       
    //modal.$element[0].style.display = 'run-in';       
    modal.$element[0].offsetWidth;
    //modal.$element.one($.support.transition.end, function () { modal.$element[0].style.display = 'block' });  
}

If that doesn't work try uncomenting those lines

@thurloat
Copy link
Author

Looks like you're correct about shown being true even during transition, perhaps a different approach is required.

Looks like @fat fixed it here twbs/bootstrap#298 a year ago and now I'm having the opposite issue.

@jschr
Copy link
Owner

jschr commented Jan 23, 2013

Yep, took it out to test pull request #47 and forgot why it was there. I'm hoping all that needs to be put back is modal.$element[0].offsetWidth;

@thurloat
Copy link
Author

I added it back in and it didn't appear to do anything different (complete still gets called only after I click an element)

@jschr
Copy link
Owner

jschr commented Jan 23, 2013

Even with the commented lines added back in as well? Could the file be cached?

@thurloat
Copy link
Author

👊ing myself ... was a bug in my application that cancelled the animation all together, and since it checks for 'fade' which IS there to determine whether it should wait for transitions, it doesn't check if in is there already which would make it not animate.

Also, a PITA to debug with breakpoints since the animations run outside the scope of the paused JS, the webkitAnimationEnd never gets called. .

Consider this one closed :)

@thurloat thurloat closed this Jan 23, 2013
@jschr
Copy link
Owner

jschr commented Jan 23, 2013

Well I put back the reflow code just in case :). Thanks for the followup.

@jschr jschr mentioned this pull request Jan 25, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants