Fixes #911 - ui.bootstrap.collapse TWBS 3.0 compatibility (DEPENDS ON #991) #934

caitp · 2013-09-03T01:23:20Z

edit Demo

In line with issue #911, I'm trying to get CSS class-switching behaviour
(and behaviour in general) in line with the stock TWBS 3.0 code.

This solution is more or less working, with the exception that there is
a brief flicker for the first expand() call (unless isCollapsed is initiallized
to false).

If anyone has any ideas about that, I'm all ears. And of course this code could
be cleaned up a lot, and tests could be added, for sure.

caitp · 2013-09-03T01:35:41Z

https://gist.github.com/caitp/0b8a463d43b5eaa696d9 I've been doing some basic testing with this file, aside from the spec tests

JasonTheAdams · 2013-09-03T01:36:35Z

I don't see the flicker you're talking about. It works perfectly fine to me.

caitp · 2013-09-03T01:40:02Z

I have only been testing in Canary, so it may be a browser-specific bug, but it should appear on the very first call to expand(), if and only if expand() is the first to be called (as opposed to collapse()) -- So basically it will only be seen if isCollapsed is initiallized to true. I'll try in FF and see if it reproduces ...

Okay, so in FF26.0a1, the first expand() isn't animated, but there is no flicker. Subsequent expansions work correctly.

So there's still a bug in the first expand(), which I'm not too sure about

JasonTheAdams · 2013-09-03T01:43:54Z

I'm viewing in Chrome. I tried it both with it collapsed and closed by default.

Here's where I threw it together: http://jsfiddle.net/fRULW/15/

caitp · 2013-09-03T01:47:13Z

Mmm, I'm still seeing the flicker in your jsfiddle (Stable Chrome 29.0.1547.65 on OSX 10.8.4)

JasonTheAdams · 2013-09-03T01:51:10Z

Huh... Same version of Chrome, on Windows 8. :)

caitp · 2013-09-03T02:07:53Z

Cleaned up code a little bit, so I guess there are only like 3 blocks of changes -- should be easier to spot whatever the error is now :>

hallister · 2013-09-03T03:35:11Z

I'm liking this @caitp :).

I'm not getting the flicker you said you're seeing on Ubuntu Chrome or Firefox. However, in Firefox the initial animation doesn't happen. The first expand() doesn't actually animate, but any collapse() or expand() does. Looking at the console, it appears on the initial expand() the classes on the collapsing element are collapsed collapsing, in is never added.

caitp · 2013-09-03T05:06:20Z

@hall5714: Yes, in FF (Nightly, at least), the first expand() transition never completes (it is cancelled when a subsequent collapse() is called...) I'm trying to figure out why. There's got to be a better way, hmm

rtucker88 · 2013-09-03T05:46:28Z

@hall5714 I duplicated the Firefox problem you saw in Ubuntu. It looks fine using Chrome and Firefox under Windows 8 though. The menu is stuck expanded after the first click on Internet Explorer 11 on my Windows 8 system once the menu is expanded.

Great job though, @caitp! I'm anxiously awaiting this Bootstrap 3 update!

caitp · 2013-09-03T13:58:20Z

Alright, it's looking pretty good -- I'm getting non-glitchy behaviour in FF Nightly and Chrome Stable here -- I am currently failing one test, but I am not sure if the test is really 'correct' (Actually, it's entirely incorrect according to the way TWBS behaves, but nevermind that)

caitp · 2013-09-03T14:26:21Z

This version seems to be working awesomely in the browser (FF/Chrome), but it does fail that one test -- I've tried to find a way to fix it but it hasn't made itself apparent just yet, any thoughts?

Changes:

Initial support for horizontal collapse (a feature of TWBS) -- However, I'm not $watching the scrollWidth as well as scrollHeight, so that's a bit tricky
expand() and collapsed() code adapted directly from TWBS now (so they are a bit bigger) -- I think some things could be factored back out, but it's working so nicely atm
Failing one test -- However as noted above, these tests do not reflect the behaviour of TWBS, which presumably should be the expected behaviour for ui-bootstrap. It might be worth changing the test?

Hmmm, I guess it's 2 failures on Travis, and one of them probably can't be safely ignored :(

hallister · 2013-09-03T14:53:10Z

@caitp collapse.spec.js:55 should be passing, as it is setting isCollapsed to false and $digesting. So the failure suggests that the elements height is zero and it really shouldn't be. I don't have time this morning to take a look but I will a little later if you haven't fixed it by then.

I'm not really sure how TWBS does collapse but personally I think the way we do it is more intuitive.

caitp · 2013-09-03T15:02:26Z

TWBS's technique can't make use of the 'angular magic', for obvious reasons, but it makes up for this by doing some great stuff (deselecting other expanded panels in a set when a new one is selected, which is pretty cool even though it imposes some markup semantics, handling both horizontal and vertical collapse, not requiring expensive $watching, etc)

So, all tests are passing with the Chrome runner, which is good. As for the rest of it, I'll see what I can do but it seems to be working fairly well so far, no noticeable glitches here (although I have not tested on Opera or IE).

When I am saying that that particular test doesn't make sense, I'm really saying the expectations don't match TWBS's behaviour -- In the TWBS code, we are primarily concerned with the presence of 'in' and 'collapse' (with 'collapsed' as a bonus, maybe for compat) -- In ui-bootstrap we have the extra state 'isCollapsed', which is separate from the collapse expression, and is also separate from the CSS class state -- So there are numerous places for this to 'break', in ways which are not actually meaningful. A single source of truth would simplify things a lot, and there isn't really such a thing in the spec currently (and we end up with even more 'truths' than TWBS)

So, I dunno, I think it would probably be good to refactor a lot of this stuff, but as it stands it is fairly close, and not glitchy on any browsers I've tested with yet. But I'll see if I can make travis a bit happier

hallister · 2013-09-03T17:56:47Z

@caitp I think I see what your saying here. Because we have collapsed and in states already, isCollapsed has become redundant.

Here's the tricky part. Eventually this project is going to be unaware of the framework and we'll start supporting Foundation, Bootstrap, etc. In order to maintain a framework-agnostic approach the collapse directive needs to be aware of the state itself, without being dependent on Bootstrap, Foundation, SuperCoolFramework.

I'm definitely not opposed to using a method other than isCollapsed, but we can't use a purely class based approach for the future, because Foundation, for example, doesn't have a collapse, so we are going to have to rely on styles.

I don't know. @pkozlowski-opensource do you think we should just refactor this without state information now and fix it later or keep the state information?

caitp · 2013-09-03T19:36:39Z

@hall5714 --- Okay, I've adjusted the code to not consider the CSS classes to be important to the truthiness of the isCollapsed state, and still get desirable behaviour in FF and chromium. However, the test suite still reports a failure on line 37 for FF, and I am not seeing why this is the case. hmm.

As far as being "framework agnostic", how does that work, per se? The CSS model for Foundation differs from the model for Bootstrap, for instance -- Are people expected to set this up in a config block or something? I think once you start trying to support "a whole bunch of front-end frameworks", it's probably going to get ugly (but that's just my opinion, if it's something people really want, then who am I to say no o_o) To me though, it seems like it's probably more sensible to target bootstrap in particular, and some other project like angular-ui/foundation or something could deal with Foundation. Like, when it comes to wrapping front-end frameworks, I don't see how it can really be done /maintainably/ in a framework-neutral manner. That sounds like a nightmare waiting to happen :(

caitp · 2013-09-04T15:27:45Z

https://gist.github.com/caitp/3faaa626ee7d7c6d88fc -- Updated test HTML to support adding/deleting elements to list, to test the resize behaviour in the browser

Seeing non-glitchy, expected behaviour in Firefox Nightly and Stable Chrome

plunker demo

Obviously, in the demo, adding/removing items is happening within angular time, and this does not reflect the nature of the failing tests, which use/require explicit $digests. It would be fun to add some extra buttons which add content outside of angular, and a separate button to trigger a recompile, just to see what sort of behaviour we get there.

hallister · 2013-09-05T05:30:14Z

So I've had a chance to play with it a little bit and here's a few issues (all based on the demo) for Ubuntu:

Chrome 28.0

Clicking collapse during an animation doesn't cancel the collapse and breaks subsequent animations. To test rapidly press "collapse" in the demo 2 times. The element will now have collapse collapsed in states all at the same time (the state it should be depends on whether you interrupt a collapse or an expand).

Firefox 23.0

Animation never takes place. The element only ever gets the collapsing class attached to it. It never has collapsed or in classes.

caitp · 2013-09-05T11:55:59Z

Last night I did some testing with FF23.01 and didn't see that, but that's interesting.that would mean that the complete() function is never called after initialAnimSkip, which sounds like an issue with $transition

Yeah, just testing with FF23.01 on OSX 10.8.4, I can't reproduce the "not animating" thing, and have recorded the screen to show it

I also haven't been able to reproduce the issue you mention on Chrome 29, however I don't have version 28 available so I can't say for certain

The reason for failure seems to be that $timeout.flush() is returning before the $transition timeout is resolved.

caitp · 2013-09-05T14:23:18Z

Got each test passing on FF23 here, but there were two failures on travis so lets see what happens :(

hallister · 2013-09-06T15:01:24Z

@caitp Still have the same issues on Chrome 28 and FF 23. Testing both on Ubuntu and Firefox only animates on the first click, than no longer animates or even hide/shows. On FF after the initial collapse in the demo, all subsequent clicks on collapse result in element having collapsed collapsing or collapsed as classes. collapsed is never removed and in is never added.

On Chrome 28, if you click collapse while an animation is occurring it removes all classes from the collapsible element, so any subsequent clicking of collapse just has strange behaviors.

I haven't had time just yet to look at this in the console, but a guess might be that:

if (transition) {
    transition.cancel();
    transition = undefined;
}

The problem is if the transition is canceled error() is called. So our state looks like this (pseudoish code):

element.addClass('collapsing');
transition.start()
transition.cancel()
error();
    element.removeClass('collapsing');
transition.end()

Now isCollapse is all kinds of messed up (since technically it's neither) and the element has no classes which causes transitions to look really weird.

Now the element has no class information and the isCollapsed state is all kinds of messed up. So I'm thinking the better approach might be to implement this as Bootstrap does and instead of bouncing back we just ignore the click until transition completes. Maybe after than we can implement a means for it to bounce back AFTER the animation completes, since the state will be as expected. Thoughts?

caitp · 2013-09-06T15:11:17Z

Well admittedly I haven't been able to produce such behaviour (but have only been testing on a mac, ymmv) -- My understanding of transitions is that the browser is /supposed/ to revert to its initial state, one way or another, in the event of a cancellation. Twitter's strategy is to simply not permit cancellations (show() and hide() will abort if this.transitioning === truthy, or if the element contains or does not contain the class 'in'), and this is not a bad way around it I think (is it really going to hurt UX that much?)

Having said that, I am having a lot of trouble getting this to behave with FF19, or even look remotely not-weird. The transition behaviour over there seems really quirky (And it's not technically a supported browser anymore, so it's not certain whether it's worth fussing over it)

So anyways, yeah, my proposal is to simply not permit cancellation of transitions, this should help avoid the whole "bogus state" issue, and would match TWBS' behaviour more closely.

hallister · 2013-09-06T15:20:46Z

@caitp I tend to agree. I think there are too many browsers that do quirky things when we try to end a transition and reverse it. Let's try with the TWBS method and see if we can actually get this thing working across browsers/os's before we worry about UX intricacies.

caitp · 2013-09-06T21:23:44Z

Okay, so we're still failing tests (they probably need to change at this point due to removed functionality), but in browser trials, things are looking good on OSX 10.8.4:

Chrome29.0.1547.65
FireFox17.0.8
FireFox19.02
FireFox23.01
FireFox26.0a1 (2013-09-04)
Opera 12.15

Buggy in:

FireFox12.0 -- transitionend event not fired after first expand() -- This seems like a bug in $transition, for FF12 we get quirky behaviour and should simply resolve the trigger immediately, but this does not happen

Let me know if there are any glitches, Updated plnkr

caitp · 2013-09-10T17:19:13Z

Lesson learned:

Firefox doesn't support the un-prefixed 'transition' property until version 16 (according to https://developer.mozilla.org/en-US/docs/Web/CSS/transition), and the stock Bootstrap CSS only includes the -webkit vendor prefixed version and the unprefixed version.

Adding a separate -moz-transition version in a separate CSS file solves this problem on FF12. However, somewhat curiously on stock Bootstrap, FF12 will take the "no-transition" path through the code (and it's not clear to me why we aren't taking it, since our transition-support detection is pretty much identical to bootstrap's). So, is it better to say "silly web developers, you need to include -moz- prefixes for your bootstrap CSS, or to just somehow force FF <v16 to use the no-transition path, as it does in stock bootstrap? I tend to prefer the latter, particularly if .collapsing doesn't provide the -moz- transition prefix, but I'm not totally sure how to make that work.

caitp · 2013-09-11T19:30:06Z

Blocked by #991 -- We really need #991 to get around the issues mentioned above, so that's a thing.

This is used by TWBS to provide support for browsers which do not have transition support, despite theoretically providing it.

Nice looking transition animations with the following browsers, on OSX 10.8.4: Chrome29.0.1547.65 FireFox19.02 FireFox23.01 FireFox26.0a1 (2013-09-04) No 'bogus state' issues with unfinished animations (that I've seen, as of yet). Unfortunately, there are some failing tests (on each browser), because some functionality had to be dropped to work well. These tests have not been rectified yet. There are some helpers for providing more robust information about the dimensions of the collapsing object, which I've included in a 'helpers' object. These should probably be either discarded, or else moved to a more central location where they can be shared by other components. Just a thought. (The implementations of these functions has been adapted from Prototype, and may not be quite as robust as jQuery).

caitp · 2013-09-13T02:41:36Z

Just a quick test to see if we're passing everything with #991 merged and the functionality added here (Local tests on Chrome show that we are)

edit passing on travis

edit I've also updated the plonk with the emulateTransitionEnd code and verified this as working correctly on chrome and FF -- Nice transitions with FF23, and no transition animations on FF12 (or other pre-v16 versions) -- Nice transitions in Chrome, and also nice transitions in Opera 12.15

So the only thing left really, is to try to make this code a bit more concise and more suitable for inclusion, at least as far as I can tell. From what I am seeing, all of the "bugs" are resolved here.

I would appreciate some help/guidance in how to tidy this up, EG where can we put the more robust dimensions-querying stuff, or what should they be replaced with if it's not okay to include it?

I do hope someone has some answers there .___.

fvicente · 2013-11-05T23:48:15Z

Hi there,
Just wanted to share that I was able to simulate the collapse effect using only CSS transitions.

.collapse {
    display: block;
    overflow: hidden;
    max-height: 0px;
    -webkit-transition: max-height .3s ease;
    -moz-transition: max-height .3s ease;
    -o-transition: max-height .3s ease;
    transition: max-height .3s ease;
}
div.navbar-collapse.collapse.in {
    max-height: 2000px;
}

http://plnkr.co/edit/HkwfL9?p=preview

azenalex · 2013-12-18T22:15:38Z

I started using this since I needed the transition. I noticed it doesn't handle padding on the div (mine have 15px on top and bottom). I made a bit of a hack to remove and replace padding before show/hide and that sort of works. Far from perfect though since I can see the contents jump when the padding is removed. Hopefully someone else can come up with something better.

      var events = {
        beforeShow: function(dimension, dimensions) {
          element
            .removeClass('collapse')
            .removeClass('collapsed')
            .addClass('collapsing');
          helpers[dimension](element, 0);

          //restore top and bottom padding
          element.css('padding-top', paddingTop);
          element.css('padding-bottom', paddingBottom);
        },

        beforeHide: function(dimension, dimensions) {
          // Read offsetHeight and reset height:
          helpers[dimension](element, dimensions[dimension] + "px");
          var unused = element[0].offsetWidth,
              unused2 = element[0].offsetHeight;
          element
            .addClass('collapsing')
            .removeClass('collapse')
            .removeClass('in');

          //store current top and bottom padding
          paddingTop = element.css('padding-top');
          paddingBottom = element.css('padding-bottom');
          //set padding top and bottom to 0
          element.css('padding-top', '0px');
          element.css('padding-bottom', '0px');
        },

caitp mentioned this pull request Sep 3, 2013

Bootstrap 3.0 Collapse Update #911

Closed

hallister mentioned this pull request Sep 5, 2013

Bootstrap3 Collapse broken #945

Closed

caitp mentioned this pull request Sep 9, 2013

collapse does not toggle #971

Closed

caitp mentioned this pull request Sep 11, 2013

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

Closed

feat(transition): emulateTransitionEnd implementation

30aedcf

This is used by TWBS to provide support for browsers which do not have transition support, despite theoretically providing it.

caitp mentioned this pull request Sep 12, 2013

Prevent collapsed style overflow: hidden from obscuring the typeahead co... #994

Closed

feat(collapse): rebased ontop of angular-ui#991

ccab3d7

This was referenced Sep 13, 2013

feat(collapse): Bootstrap3 compatibility/bugfixes #1001

Closed

feat(position): Access width and height for hidden fields #1036

Closed

pkozlowski-opensource closed this Sep 27, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #911 - ui.bootstrap.collapse TWBS 3.0 compatibility (DEPENDS ON #991) #934

Fixes #911 - ui.bootstrap.collapse TWBS 3.0 compatibility (DEPENDS ON #991) #934

caitp commented Sep 3, 2013

caitp commented Sep 3, 2013

JasonTheAdams commented Sep 3, 2013

caitp commented Sep 3, 2013

JasonTheAdams commented Sep 3, 2013

caitp commented Sep 3, 2013

JasonTheAdams commented Sep 3, 2013

caitp commented Sep 3, 2013

hallister commented Sep 3, 2013

caitp commented Sep 3, 2013

rtucker88 commented Sep 3, 2013

caitp commented Sep 3, 2013

caitp commented Sep 3, 2013

hallister commented Sep 3, 2013

caitp commented Sep 3, 2013

hallister commented Sep 3, 2013

caitp commented Sep 3, 2013

caitp commented Sep 4, 2013

hallister commented Sep 5, 2013

caitp commented Sep 5, 2013

caitp commented Sep 5, 2013

hallister commented Sep 6, 2013

caitp commented Sep 6, 2013

hallister commented Sep 6, 2013

caitp commented Sep 6, 2013

caitp commented Sep 10, 2013

caitp commented Sep 11, 2013

caitp commented Sep 13, 2013

fvicente commented Nov 5, 2013

azenalex commented Dec 18, 2013

Fixes #911 - ui.bootstrap.collapse TWBS 3.0 compatibility (DEPENDS ON #991) #934

Fixes #911 - ui.bootstrap.collapse TWBS 3.0 compatibility (DEPENDS ON #991) #934

Conversation

caitp commented Sep 3, 2013

caitp commented Sep 3, 2013

JasonTheAdams commented Sep 3, 2013

caitp commented Sep 3, 2013

JasonTheAdams commented Sep 3, 2013

caitp commented Sep 3, 2013

JasonTheAdams commented Sep 3, 2013

caitp commented Sep 3, 2013

hallister commented Sep 3, 2013

caitp commented Sep 3, 2013

rtucker88 commented Sep 3, 2013

caitp commented Sep 3, 2013

caitp commented Sep 3, 2013

hallister commented Sep 3, 2013

caitp commented Sep 3, 2013

hallister commented Sep 3, 2013

caitp commented Sep 3, 2013

caitp commented Sep 4, 2013

hallister commented Sep 5, 2013

caitp commented Sep 5, 2013

caitp commented Sep 5, 2013

hallister commented Sep 6, 2013

caitp commented Sep 6, 2013

hallister commented Sep 6, 2013

caitp commented Sep 6, 2013

caitp commented Sep 10, 2013

caitp commented Sep 11, 2013

caitp commented Sep 13, 2013

fvicente commented Nov 5, 2013

azenalex commented Dec 18, 2013