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

Collapse does not expand if div is hidden #1774

Closed
functionportal opened this issue Feb 10, 2014 · 12 comments
Closed

Collapse does not expand if div is hidden #1774

functionportal opened this issue Feb 10, 2014 · 12 comments

Comments

@functionportal
Copy link

If a div is hidden when collapse value is toggled from true to false, the div is not expanded, and is stuck in collapsed state with a class 'collapsing'. This can be easily reproduced by setting collapse to true, setting ng-show to false, then set collapse value to false, then set ng-show to true. The div will remain collapsed even though collapse value is now false.

@mvhecke
Copy link
Contributor

mvhecke commented Feb 10, 2014

This is default behavior because ngShow uses display: none!important; and overrules the collapse. You can find more about this behavior in the Angular docs.

@pkozlowski-opensource
Copy link
Member

@craftyInstigator I think @Gamemaniak is totally right here. You would have to provide the exact reproduce scenario using http://plnkr.co/ to confirm.

@functionportal
Copy link
Author

display setting and ng-show has absolutely nothing to do with this problem. This is caused by the animation failing to complete and never triggering .then callback to finalize the expand. As a result the class is never switched to "collapse in" and is stuck at "collapsing" and the height remains explicitly set to 0 on the element. The bug is in collapse.js:

doTransition({ height: element[0].scrollHeight + 'px' }).then(expandDone);

when hidden, element[0].scrollHeight evaluates to 0, expandDone callback passed to .then is not invoked. I was able to fix the problem with the following code change:

function expand() {
          if (initialAnimSkip || element[0].scrollHeight==0) {
            initialAnimSkip = false;
            expandDone();
          } else {
            element.removeClass('collapse').addClass('collapsing');
            doTransition({ height: element[0].scrollHeight + 'px' }).then(expandDone);
          }
        }

This way I avoid the animation while the scrollHeight is 0 (hidden)

I also to make sure update the collapse class to do a similar thing:

function collapse() {
          if (initialAnimSkip || element[0].scrollHeight==0) {
            initialAnimSkip = false;
            collapseDone();
          } else {
            // CSS transitions don't work with height: auto, so we have to manually change the height to a specific value
            element.css({ height: element[0].scrollHeight + 'px' });
            //trigger reflow so a browser realizes that height was updated from auto to a specific value
            var x = element[0].offsetWidth;

            element.removeClass('collapse in').addClass('collapsing');

            doTransition({ height: 0 }).then(collapseDone);
          }
        }

        function collapseDone() {
          element.removeClass('collapsing');
          element.addClass('collapse');
          element.css({height: 0});
        }

@functionportal
Copy link
Author

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

  1. Click Toggle collapse to true (collapses fine)
  2. Click Toggle visibility to false (hides fine)
  3. Click Toggle collapse to false (hidden, so you can't see)
  4. Click Toggle visibility to true (still collapsed despite collapse being false)

@chrisirhc
Copy link
Contributor

Thanks for the test case, @craftyInstigator . This is a limitation of the $transition service. This issue should be resolved by #1444 when it lands.

@bvaughn
Copy link

bvaughn commented Jun 21, 2014

FYI, I ran into the same problem as originally described. Turns out to have been caused by a race condition in the code. Setting a transition time greater than 0 seconds resolves the problem.

@juniorplenty
Copy link

@bvaughn - thanks for the tip. Could you be more specific tho? Trying to work around this but not sure where to set this transition time -

@bvaughn
Copy link

bvaughn commented Jun 25, 2014

@juniorplenty Yeah if I recalled it seemed like a race condition where the component isn't expecting a transition to complete synchronously. Should be able to work around it by adding the following:

.collapsing{
  position: relative;
  height: 0;
  overflow: hidden;
  -webkit-transition: height .35s ease;
  transition: height .35s ease;
}

@porjo
Copy link

porjo commented Oct 29, 2014

What's the status on this? I've updated the plnkr to Angular 1.3 and boostrap-ui to 0.11.2, but issue still occurs:

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

@jamesbrobb
Copy link

I'm also still having the same issue with all the latest versions.

As @craftyInstigator pointed out, this issue's caused when the value of the elements scrollHeight property (which is being used to trigger the height change), is the same as the elements current height. In this instance, they're both 0. Due to this the transitionend event is never fired. So the elements class and its height value (in the case of expandDone) are never set to their final values.

You can see it in action in this simple example. If you try to set the height of the block to it's existing height, then transitionend never get's fired. As per the w3c spec.

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

So are there 2 issues here?

  1. Why does scrollHeight return 0 in this instance? I'm fairly new to css/html but am presuming that it's the expected behaviour depending on the display or overflow css values of the element? Or in my case its container.

  2. Should $transition handle this issue? So if trigger is an object, all of it's property values should be checked against the existing element values and if all match, the promise should be resolved immediately, forcing the corresponding done function to fire.

Here's an example of a simplistic solution for 2)

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

I say simplistic, as there are potential issues depending on the usage of $transition by all components within bootstrap ui. The value passed in must match the existing value exactly - so obviously 0 won't match 0px. So in the collapse method of the directive, this line,

doTransition({ height: 0 }).then(collapseDone);

would need to be changed to this

doTransition({ height: 0 + 'px' }).then(collapseDone);

My idea for the solution came from the SO post below, in which the answer suggests setting the element css to it's new value and then comparing it to the original. This should help this issue, but i was unable to get it to work, as even though the new value has been set on the element, the old value is still returned.

http://stackoverflow.com/questions/9548723/css-transition-event-not-fired-in-jquery-when-there-is-no-property-change

@aeharding
Copy link
Contributor

I'm interested in this too (I've encountered the problem as well). For angular purposes, the easy solution is to use ngIf instead of ngShow/ngHide.

Maybe if it's hidden then resolve immediately?

@chrisirhc chrisirhc self-assigned this Mar 23, 2015
@chrisirhc chrisirhc modified the milestones: 0.13.0, ngAnimate Mar 23, 2015
chrisirhc added a commit to chrisirhc/angular-ui-bootstrap that referenced this issue Mar 23, 2015
- Animations are now opt-in, include ngAnimate to see collapse
  animations

- ngAnimate handles initial state and doesn't animate if first
  reflow hasn't occurred.
  angular/angular.js@cc58460

- Tests may need more work. Right now they test for 'in' class.

Fixes angular-ui#1774
Fixes angular-ui#2821
Fixes angular-ui#2836
Closes angular-ui#1274
Closes angular-ui#1444
chrisirhc added a commit to chrisirhc/angular-ui-bootstrap that referenced this issue Mar 23, 2015
- Animations are now opt-in, include ngAnimate to see collapse
  animations

- ngAnimate handles initial state and doesn't animate if first
  reflow hasn't occurred.
  angular/angular.js@cc58460

- Tests may need more work. Right now they test for 'in' class.

Fixes angular-ui#1774
Fixes angular-ui#2821
Fixes angular-ui#2836
Closes angular-ui#1274
Closes angular-ui#1444
ayakovlevgh pushed a commit to ayakovlevgh/bootstrap that referenced this issue Mar 24, 2015
- Animations are now opt-in, include ngAnimate to see collapse
  animations

- ngAnimate handles initial state and doesn't animate if first
  reflow hasn't occurred.
  angular/angular.js@cc58460

- Tests may need more work. Right now they test for 'in' class.

Fixes angular-ui#1774
Fixes angular-ui#2821
Fixes angular-ui#2836
Closes angular-ui#1274
Closes angular-ui#1444
@psldevelopers
Copy link

Can this fix to be applied to 0.12? We who are still with Angular 1.2.X need this fix as well.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests