Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

feat(transition): emulateTransitionEnd implementation (BLOCKS #934) #991

Closed
wants to merge 1 commit into from

Conversation

caitp
Copy link
Contributor

@caitp caitp commented Sep 11, 2013

TWBS uses this fallback path to provide functional behaviour on browsers which will not use a transition.

I have found that this sort of behaviour is required particularly for Firefox releases prior to version 16,
because Bootstrap's stock CSS does not provide -moz-prefixed transition properties, and prefixless transition
is not supported until FF16.

Using this emulateTransitionEnd functionality, we can have functional behaviour on browsers which do not
support transitions, even though we do detect that we support transitionend events. This can prevent a lot of
buggy behaviour.

I'd like to provide automated tests for this functionality, and am working on that now.

@caitp
Copy link
Contributor Author

caitp commented Sep 11, 2013

Okay, one test in place, which is passing on Chrome at least (and from testing in #934 I've seen that this is working in pre v16 FireFox. Let me know if there's anything that could be improved upon

I guess it's passing on FF19 too, yay.


So, things that could use some clearing up / discussing:

  • Does it make sense to stick this function onto the promise returned from $transition? This seems a bit weird.
  • Theoretically this should make it nice and chainable, but I wasn't actually able to chain it in the unit test (which is weird!) so this might be slightly broken. But it's not a major inconvenience really.
  • Does it hurt performance instantiating this function each time $transition is called? This might not be ideal

I guess support for chaining emulateTransitionEnd is probably not going to work out, because I don't really want to decorate $q with this function. It's doable to be sure, but I'm not sure it's a great idea?

This is used by TWBS to provide support for browsers which do not have
transition support, despite theoretically providing it.
caitp pushed a commit to caitp/ui-bootstrap that referenced this pull request Sep 13, 2013
caitp pushed a commit to caitp/ui-bootstrap that referenced this pull request Sep 13, 2013
@pkozlowski-opensource
Copy link
Member

@caitp Just a question - do you personally care about FFox < 16? I mean, do you need to support it in your project? The thing is that it is > 1 year old and with the auto-update it should have negligible market share. So I don't think it makes sense to spend much time on adding this support and penalize modern browser users with additional code.

WDYT?

@caitp
Copy link
Contributor Author

caitp commented Sep 13, 2013

While older versions of FF do have a very, very, very tiny marketshare, they do technically exist, and new browsers with incomplete transition support or partial transition support will likely exist in the future (lets take bets on when NetSurf gets there!)

Personally, my feeling is that it doesn't really hurt a whole lot to provide a fallback for these browsers so that they can at least consume the website without some of the frills, and I would argue that the single function call doesn't hurt maintainability too much, if at all.

I think that, since TWBS feels this is worth doing, it probably is, but who knows.

@pkozlowski-opensource
Copy link
Member

@caitp I hear what you are saying and I really do appreciate your efforts. It is rare to find people digging into obscure browser incompatibility issues. But let me offer the bigger picture here: soon we are going to move to AngularJS 1.2 and start to use built-in animations support. I'm not super clear what will be the impact but I would expect that the $transition service will go away as soon as we move to AngularJS 1.2. In this sense I'm not sure we do the best investment spending time on changing $transition to support browsers with market share < 0.5. And honestly, even if animations don't work on those browsers, it is not the whole end of the world as soon as a directive functions properly.

Anyway, just sharing my thoughts here.

@caitp
Copy link
Contributor Author

caitp commented Sep 13, 2013

I appreciate that. What is happening though is, if we think we support transitions, but no transition is going to take place, then transitionend is never fired and thus the directive effectively does not work.

I'm not sure if ngAnimation will support some kind of fallback or not, but what this means is that for these browsers, they do not get a functional experience because the state transition never actually occurs. So, in these (perhaps rare, admittedly) cases, we actually don't have a functioning directive, which might mean that the entire app's interface is broken.

So yeah, there are ways around this, like serving a different site based on whatever the user agent says (terrible idea), but I feel like it's not a totally good thing.


Having said that, what I'm most concerned with is just getting collapse working -at all-, so, if we end up leaving outdated FF behind, that's too bad. This will also affect versions of Opera prior to 12.5, as well, which may hurt a big-ish portion of the embedded not-iOS market.

I'm not necessarily opposed to ignoring these browsers, but if we are going to guarantee support for them, it's going to have to involve something like this, or else forcing users to override some CSS (which sucks). -- Speaking of overriding CSS, this would also prevent brokenness if someone decided to remove the transition properties from .collapsing (or similar) in the first place.

@caitp
Copy link
Contributor Author

caitp commented Sep 23, 2013

@pkozlowski-opensource I don't suppose we could merge this temporarily, until we're ready to move everything over to ngAnimate? I mean, this only really helps older Firefox releases, and maybe a few older Opera releases, so it could be omitted at their expense.

But I'd really like to get everything in order to get collapse working, and then start moving Collapse over to ngAnimate

It's really between this, #1036 (which may eventually be unneeded thanks to angular/angular.js#4078 (if this ever is merged, no guarantee of that) -- If #4078 is actually merged in the future, then #1036 could be thrown away. And it's likely that eventually, ngAnimate will remove the need for this particular hack to support older browsers with vendor prefixes.

But until that actually happens, we'd all like to see ui.bootstrap.collapse playing nice

colthreepv pushed a commit to TopCS/bootstrap-ui that referenced this pull request Nov 1, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants