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

Transition: Disable animationEnd callback on transition reset. #532

Merged
merged 1 commit into from
Jan 15, 2014

Conversation

dos1
Copy link
Contributor

@dos1 dos1 commented Jan 9, 2014

Fixes rare race condition with modals where animationEnd callback on dimmer could fire before modal's one, calling .transition('reset') on modal right before animationEnd event for it arrives, causing module.complete callback to restore incorect conditions, making modal visible when it shouldn't be, causing next modals to misbehave.

Fixes rare race condition with modals where animationEnd callback on dimmer could fire before modal's one, calling .transition('reset') on modal right before animationEnd event for it arrives, causing module.complete callback to restore incorect conditions, making modal visible when it shouldn't be, causing next modals to misbehave.
@jlukic
Copy link
Member

jlukic commented Jan 15, 2014

Cautiously merging this, luckily only modal is using transition reset currently, so regression possibilities are minimal.

Previously we used a fix like this which made sure there were no race conditions.

But after transition rewrite, I thought this bug would be gone since I could not reproduce.

jlukic added a commit that referenced this pull request Jan 15, 2014
Transition: Disable animationEnd callback on transition reset.
@jlukic jlukic merged commit 6cedde9 into Semantic-Org:master Jan 15, 2014
@dos1
Copy link
Contributor Author

dos1 commented Jan 15, 2014

It's actually similar, but different bug - this one is harder to trigger and instead of closing modal without closing the dimmer, it makes previous modal to not close at all (transition module thinks it's already hidden, but modal and dimmer think it's still there).

After .transition('reset') the modal was made hidden (correct), but when animationEnd arrived after that, it was made visible again (wrong). It seems to me like it's rather caused by the browser than race condition in the code, as I'd think that after reset this callback shouldn't actually arrive at all.

On one PC I could reproduce this one easily by dragging the tab out of window approximately the same time animations were about to finish, but on the other one it required many tries to trigger it. I wasn't able to reproduce it by modifying Semantic code to change the order of called instructions manually, so again, it smells to me like a "race condition" in the browser, which reports animationEnd callback for animation that was already cancelled right before that (which when I think about it may be explained by JS thread being separated from rendering threads in modern browsers - event may be already queued to be triggered when animation is reseted, so it will be called only when JS thread is back to event loop)

I hope I'm not too hard to understand :D The commit message has already revealed my tendency to make overly complicated sentences ;)

@jlukic
Copy link
Member

jlukic commented Jan 15, 2014

Your fix landed in 0.12.1. Seeing as reset is only called when using modal, and it appears to fix the issue in your testing. I trust that it will be fine.

My only concern was about breaking queueing which uses animationEnd to trigger the next queued animation.

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.

None yet

2 participants