Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

animate.js removeClass seems to call setClass with a wrong type #11268

Closed
thomasvs opened this issue Mar 7, 2015 · 12 comments
Closed

animate.js removeClass seems to call setClass with a wrong type #11268

thomasvs opened this issue Mar 7, 2015 · 12 comments

Comments

@thomasvs
Copy link

thomasvs commented Mar 7, 2015

I was getting tracebacks in my angular application.

I tracked it down to what I believe is removeClass calling setClass wrong:

https://github.com/angular/angular.js/blob/master/src/ngAnimate/animate.js#L1114

shows

return this.setClass(element, [], className, options);

while the second argument to setClass is supposed to be a string according to:

  • @param {string} add the CSS classes which will be added to the element

but instead a list ([]) is passed in; which throws an error in a foreach:

forEach(cssClasses.split(' '), function(cssClass) {

where this fails because you can't call split on an empty list.

@wesleycho
Copy link
Contributor

Looking at the source code, passing an array is fine - the array would get serialized into a string at worst here .

Can you give a fuller stack trace? It would be easier to figure out the error, since as far as I can tell, cssCasses should already be a string by the time it reaches jqLiteRemoveClass.

@thomasvs
Copy link
Author

TypeError: undefined is not a function
at jqLiteAddClass (angular.js:2572)
at angular.js:4436
at forEach (angular.js:328)
at Object.setClass (angular.js:4435)
at Object.setClass (angular-animate.js:980)
at Object.removeClass (angular-animate.js:944)
at Object.fn (ui-bootstrap-tpls.js:1689)
at Scope.$digest (angular.js:12715)
at Scope.$apply (angular.js:12980)
at done (angular.js:8496)

I'm not sure I follow - even if the type conversion could conceivably do the right thing (although I'm not sure why an empty array would get converted to a string) - why not simply call the function with the value already correct instead of wrong-but-potentially-converted-to-right-type ?

@petebacondarwin
Copy link
Contributor

@thomasvs - can you check if this is still an issue in the latest version of ngAnimate since it has been completed rewritten recently?

@Narretz
Copy link
Contributor

Narretz commented May 13, 2015

I'm gonna close this, as

  • it's likely that this is fixed in 1.4
  • there's no reproduce scenario available, so we aren't even sure if this is an error at all

@bartolkaruza
Copy link

I'm running into this same issue on the latest version ngAnimate, 1.4.3

My bower dependencies;

{
    "name": "myapp",
    "version": "0.0.1",
    "dependencies": {
        "angular": ">=1.4.*",
        "angular-resource": ">=1.4.*",
        "angular-animate": ">=1.4.*",
        "angular-cookies": ">=1.4.*",
        "angular-sanitize": ">=1.4.*",
        "angular-messages": ">=1.4.*",
        "angular-ui-router": "latest",
        "material-design-icons": ">=1.0.1",
        "lodash": "~2.4.1",
        "angular-material": "0.8.3",
        "angular-socket-io": "~0.6.0",
        "angular-i18n": "~1.3.14",
        "angular-moment": "~0.9.0",
        "angular-socket-io": "~0.6.0"
    },
    "devDependencies": {
        "angular-mocks": ">=1.2.*",
        "angular-scenario": ">=1.2.*",
        "should": "shouldjs/should.js#~4.4.1"
    },
    "resolutions": {
        "angular": ">=1.4.*",
        "angular-animate": ">=1.4.*"
    }
}

The project is generated using this yeoman generator generator and others are running into the same issue

Generating a project, using the generator-material-app should give you an easy reproduce scenario.

@matsko
Copy link
Contributor

matsko commented Jul 24, 2015

@bartolkaruza would you mind creating a new issue, explaining in detail as to what's going on, providing a demo which uses 1.4.3 (use http://plnkr.co/)? Please mention me in the issue so that I can fix it as soon as possible.

@jliang6
Copy link

jliang6 commented Jul 28, 2015

Just use Angular-Material;s startup project: https://github.com/angular/material-start

Run it with the instruction, and then you'll see this problem in the chrome console.

This happens even after jQuery is loaded before all angularJS libraries.

@jliang6
Copy link

jliang6 commented Jul 28, 2015

Note that I am using Angular version 1.4.3.

Turns on the console and if you switch to mobile, it will happen more often. If you use the desktop browser (Chrome), it will happen at the reload/start of the application.

Okay, the problem is somehow cssClasses @ angular.js:2897 is a function.

If JQLite is used (No jQuery or jQuery is loaded after), then the function is defined at angular.js:3505

If jQuery 2.1.4 is used, then the function is defined at jquery.js:7170 "addClass" function.

@matsko
Copy link
Contributor

matsko commented Jul 28, 2015

Would you mind testing against the snapshot: https://code.angularjs.org/snapshot/angular.js and https://code.angularjs.org/snapshot/angular-animate.js ? We did some fixes to class-based animations, but there hasn't been a full release just yet. The snapshot contains the most up to date code.

@jliang6
Copy link

jliang6 commented Jul 28, 2015

No, the latest snapshot does not work.

Same problem and it happens at angular.js:2923 with and without jquery 2.1.4.

@matsko
Copy link
Contributor

matsko commented Jul 28, 2015

OK. I'm having a hard time finding the issue myself.

Could you possibly put it together into a plnkr example? We could also pair on this via google hangouts to find the issue if that's easier for you.

@jliang6
Copy link

jliang6 commented Jul 28, 2015

Thanks Matsko's help! All you need to do is to update the version of angular-material to the latest version.

matsko added a commit to matsko/angular.js that referenced this issue Jul 28, 2015
matsko added a commit to matsko/angular.js that referenced this issue Jul 29, 2015
matsko added a commit that referenced this issue Jul 29, 2015
netman92 pushed a commit to netman92/angular.js that referenced this issue Aug 8, 2015
ggershoni pushed a commit to ggershoni/angular.js that referenced this issue Sep 29, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants