fix(ngAnimate): safe-guard against missing document #14633
Conversation
In tests, the $document service might be mocked without providing a real document, which will lead to errors. With this fix, we provide an empty object as a default document to safe-guard against this. If the empty object is used, no animations will run, as the animated node won't have a body as a parent. This commit also changes the check for document.hidden slightly. It should be accessed independently of the current animationsEnabled state. Since animations are only enabled after two digests, it's possible that some tests never reach the animationsEnabled = true state and therefore aren't actually checking the document.hidden state, which means that the previous fix only works if no more than two digests happen in the test.
| @@ -331,7 +334,7 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) { | |||
|
|
|||
| var isStructural = ['enter', 'move', 'leave'].indexOf(event) >= 0; | |||
|
|
|||
| var documentHidden = animationsEnabled && $document[0].hidden; | |||
| var documentHidden = rawDocument.hidden; | |||
There was a problem hiding this comment.
Do you also need to include animationsEnabled ?
There was a problem hiding this comment.
@ThomasBurleson - that was a hack by @IgorMinar to get the tests passing in G3
There was a problem hiding this comment.
You mean "not need"? animationsEnabled was added by Igor to fix a test in the g3 sync, but as I wrote in the commit message, this doesn't actually prevent the access to the document in all cases. If animations are enabled, $document[0] is still checked, and it might still be undefined
|
LGTM |
| @@ -583,7 +586,7 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) { | |||
| * d) the element is not a child of the $rootElement | |||
| */ | |||
| function areAnimationsAllowed(element, parentElement, event) { | |||
| var bodyElement = jqLite($document[0].body); | |||
| var bodyElement = jqLite(rawDocument.body); | |||
There was a problem hiding this comment.
IIRC there is one more access to $document[0].body. Should we change that too ?
|
does it really make sense to run animations when $document is mocked out? what's the use-case for that? I think that all of the animation code assumes that either the animations enabled and document is not mocked out (or is mocked out by a special animations-compatible mock used for unit testing of the animations code) or the animations are disabled. With all these changes we are adding a forth scenario - animations are enabled but $document is mocked out by third-party that is not aware of the $document requirements by the animation code. |
|
I think the assumption we should make is that if someone mocks out $document then they don't want animations to run. |
|
We can make this assumption but this might not necessarily be what someone else expects because it's not clear that animations need a document. But i like it since it makes the code simpler. |
|
Shouldn't tests just mock the document properly? Seems weird putting that mock in the actual code... |
|
To clarify, even in the current form, without a document no animations are performed, as the areAnimationsAllowed check won't find a body element: https://github.com/angular/angular.js/pull/14633/files#diff-c15d87c15661cba4a721501ba4fda5b2R589 So I'd be okay with making this explicit if we find no document in the first place. |
|
I just realised that this actually checks the opposite... if the document is mocked out (and Perhaps we should set |
|
Yes that would short-circuit the animations earlier and be more explicit. Sounds good to me. |
|
I've updated the PR to use an object {hidden: true} when no document is available. This will prevent the animator from attempting to run any animations |
|
LGTM |
A different fix for the mocked document problem (in 1.4.x) that also fixes another possibly breaking access to the document.
One caveat: since we are now failing gracefully, and a missing document means no animations fire, tests that test animations and mock out document will not run animations, and this will be opaque to developers. However, I assume that most tests that mock document are actually very basic, as lots of other things should break when $document is mocked
Please check if the PR fulfills these requirements