feat(collapse): Bootstrap3 compatibility/bugfixes

[caitp]

Same issues as #934:

• Make code more concise (I could use help with this, as I've said)
• Remove now-unused fixUpHeight routine

---

Short of this, tests are passing, and from what I've seen, we're getting
good behaviour from all browsers. So this is good.

See #934 for other work on this same problem, I'd like to get this merged
in, for the benefit of my own apps and everyone elses :]

---

Passing on Travis-CI

[hallister]

I'll take a look at this tomorrow evening. Things have been a bit crazy with some other projects, but I really want to see collapse working :).

[caitp]

As far as "de-coupling from Bootstrap CSS" is concerned, I guess other code in the library has/had stuff like config objects with classes to add/remove in specific states and stuff -- But I had thought this was sort of frowned upon. So, if it's desirable we could do that (and this might shrink the code a small bit), but I'm not sure if it's really desirable

[caitp]

I might be able to shrink this down a lot by relying on $position for calculating element width/height -- however, this is still not necessarily a robust solution in the presence of certain CSS property values.

Would people hate me if I submitted a patch to get more reliable dimensions out of $position?

I've opened an issue about this (#1036)

[Routhinator]

@caitp Everything works great with this. I've found one behaviour I find odd but it may be intended.

When the button is pressed on a mobile device and then again to close the menu, the button remains highlighted after the drawer closes. It seems to me something should be added to deselect the button after the rollup animation completes.

It does deselect after you click away onto something else however.

[caitp]

@routh this would depend on what you're doing exactly, however .navbar-toggle uses the pseudoclass :hover and :focus to highlight the button (in stock bootstrap3 css). If you're focusing on the element in mobile, it's going to have that effect (assuming you're using .navbar-toggle). To my knowledge, we don't add any extra logic to blur the button for you!

It's hard to say what's actually happening without a little more diagnostic info there, though.

[Morgul]

Noticed in #1153 that you said this wasn't getting much review, so for what it's worth, here's a comment from a user:

I've tested this out myself and it seems to work fine on both iPad and on Chrome/Safari. Haven't run into any issues with it at all; animations seem smooth, and it works as expected. Would love to see this merged in.

[mvhecke]

I've ran in to an error while running test. There is also a strange behavior (Chrome 30 and FF 25 on Ubuntu 13.04) where the element that should be collapse doesn't appear at once, but a guess this has to do with the transitioning that is a work in progress?

Chrome 30.0 (Linux) collapse directive should collapse if isCollapsed = true with animation on subsequent use FAILED
    Expected 20 to be 0.
    Error: Expected 20 to be 0.
        at null.<anonymous> (/home/marcellino/bootstrap-test/src/collapse/test/collapse.spec.js:37:30)
Chrome 30.0 (Linux): Executed 472 of 472 (1 FAILED)

[caitp]

Testing on my Ubuntu 13.04 machine, I'm getting all green tests. It might be flaky (but it shouldn't be due to flushing $timeout).

So you're saying you can actually see the element not appearing at once, but with no animation? Eg you click, there is a latency, and then the element suddenly is shown? Or do you just mean in the test you're apparently getting that behaviour

[mvhecke]

To check if I did anything wrong re-cloned your branch and tried it again but I get the same result. To properly show you the effect I've made a screen capture of it in Chrome and Firefox. http://youtu.be/Ow8oFCITsVI

[caitp]

does the documentation include the bootstrap 3 css? The only browsers you should see this effect with, using the stock Bootstrap3 css, is Firefox prior to version 17, and Opera prior to whichever version they unprefixed in. And IE pre-9 or 10, I guess.

Looks like the docs are (unless you modified them) loading http://netdna.bootstrapcdn.com/twitter-bootstrap/2.3.1/css/bootstrap.css

This CSS works a bit differently from the v3.0.0 CSS, so it makes sense that you're seeing this behaviour in the demo

[mvhecke]

Having a facepalm moment, forgot to check that. I will check it again tomorrow.

[mvhecke]

Working for long hours is still counter productive, but I've tested it again and it seems to work in Chrome. When I tested it in Firefox and IE10 I noticed that the opening goes with a transition, but the closing does not and it does collapse immediately. Testing IE9 was not possible because it crashes when I opened my Plunk each time.. http://plnkr.co/edit/qO3YNH?p=preview

[caitp]

Yeah, I've seen this working correctly on FF17+ (prior versions would not animate with the stock bootstrap CSS due to requiring the -moz- prefix'd transition) -- so I'll have to investigate your markup a bit. I'm seeing the opposite issue, where closing animations are shown, but we are opening instantaneously (tested in FF17 and FF26, same issue both times).

On 2013-10-16, at 7:49 AM, Marcellino van Hecke [email protected] wrote:

Working for long hours is still counter productive, but I've tested it again and it seems to work in Chrome. When I tested it in Firefox and IE10 I noticed that the opening goes with a transition, but the closing does not and it does collapse immediately. Testing IE9 was not possible because it crashes when I opened my Plunk each time.. http://plnkr.co/edit/qO3YNH?p=preview

—
Reply to this email directly or view it on GitHub.

[caitp]

Interestingly, if you add class="container" to the collapse element, you get the "appropriate" behaviour in FF.

This is a CSS issue, because the JS does not care about the "container" class being present. I guess we need to ensure that the appropriate properties are available

[colthreepv]

@Gamemaniak to use Plunker on IE9 just use the run URI, in your case http://run.plnkr.co/plunks/qO3YNH/ , it's a known issue 😦

Anyway this issue is much needed, 👍 for all your efforts

[pkozlowski-opensource]

@caitp, all - finally I had a bit of time to look into the collapse thing, both for BS2 and BS3. Here is my take:
http://plnkr.co/edit/j4CZx4ZQa3HlnnS84edk?p=preview

I'm probably skipping some important use-cases since my code is rather concise. I'm especially worried about this @caitp comment about scrollHeight not being quite reliable. @caitp could you please elaborate?

Guys, could you please have a look at the provided code and help me spot any deficiencies? This code still needs some love to remove code duplication and add unit tests but it seems to work ok, at least in modern browsers.

[caitp]

So, it's not so much "unreliable" as it is "not necessarily the appropriate measure". At this point though, if the solution is many fewer lines of code, I'm all for it.

I'm happy to poke around it today and see if I can break it with anything.

On the way out, here are some MDN snippets

---

Lets read MDN (where the editors seem to not be entirely sure about this either! maybe someone can find dbaron or something and ask him)

offsetHeight:
Typically, an element's offsetHeight is a measurement which includes the element borders, the element vertical padding, the element horizontal scrollbar (if present, if rendered) and the element CSS height.

Non-scrollable elements (CSS overflow not set or set to visible) will have equal offsetHeight and scrollHeight (is this right? scrollHeight doesn't include the border, while offsetHeight would include border).

For the document body object, the measurement includes total linear content height instead of the element CSS height. Floated elements extending below other linear content are ignored.

---

scrollHeight:
An element's scrollHeight is a measurement of the height of an element's content including content not visible on the screen due to overflow.

If the element's content generated a vertical scrollbar, the scrollHeight value is equal to the minimum clientHeight the element would require in order to fit all the content in the viewpoint without using a vertical scrollbar. When an element's content does not generate a vertical scrollbar, then its scrollHeight property is equal to its clientHeight property. This can mean either the content is too short to require a scrollbar or that the the element has CSS style overflow value of visible (non-scrollable).

[pkozlowski-opensource]

@caitp not sure what has happened but GitHub decided to close this PR when I've pushed a rebased version of the bootstrap3 branch :-( In any case I'm going to open a new PR with a cleaned up version of the code from the plunker I've attached earlier today.

Thnx for all the work on this, I really appreciate!

[Morgul]

@pkozlowski-opensource I just did some testing on the bootstrap_collapse branch, and I noticed that if you start off with your content collapsed, the first time it opens there's no animation; just just pops open. After that initial time, it always animates.

Oddly, if you start with your content expanded, it always animates.

This can be seen here: http://plnkr.co/edit/ixOYAn4yqwNBeSECQLTo?p=preview (forked from your previous plunkr post.)

[pkozlowski-opensource]

@Morgul yup, this impl still needs some love, working on it as we speak. Here is a plunk with a fixed version:
http://plnkr.co/edit/jfmOsQ6ojnRb1AQ001EE?p=preview

[Morgul]

Thanks! That worked great.

[caitp]

I'm working on a slimmed down version which will use ngAnimate rather than $transition, to enhance on the already merged work. It's a bit shaky so far, but I'll try to finish that up tomorrow

[vkelman]

Good thing is that a plunk by Pawel Kozlowski still works after changing AngularJS version to 1.1.15 and adding the reference to ui-bootstrap-tpls-0.6.0.js.
I just wanted to match our current working environment and ensure that adding transition.js and collapse.js won't break JavaScript.
http://plnkr.co/edit/w5i59zmvrQXrXiSB0cv7?p=preview