fix(ngAnimate): ensure that all jqLite elements are deconstructed properly

[matsko]

Prior to this fix if a form DOM element was fed into parts of the
ngAnimate queuing code it would attempt to detect if it is a jqLite
object in an unstable way which would allow a form element to return an
inner input element by index. This patch ensures that jqLite instances
are properly detected using a helper method.

Closes #11658

[chirayuk]

I have two comments—one about the naming of getDomElement and another about a naming convention for jQuery objects vs DOM nodes.

On naming getDomElement

As an example, when you replaced (in animateQueue.js:411) clearElementAnimationState(element) with clearElementAnimationState(getFirstNode(element)), to the reader, it looks like you already had an element and then you got the first node from it. The reader will typically wonder if element is a jQuery object or some DOM node collection or if getFirstNode returns some kind of child node. If instead, one writes, clearElementAnimationState(asDomNode(element)) / clearElementAnimationState(toDomNode(element)) / clearElementAnimationState(ensureDomNode(element)) or some such call, then it becomes very clear that while element itself might be a DOM node or a jQuery element, the result of the call is not a jQuery element but an actual plain DOM node.

Naming conventions for jQuery instances vs DOM nodes

There are parts of the code that require DOM nodes, some that require jQuery (or angular.element) instances, and some that can work with either. To the reader (and all future maintainers), it can become pretty hard to keep track of this given the lack of a type safe language. Perhaps we should use a naming convention—element always means it's a jQuery instance, node always means it's a DOM node, and perhaps elementOrNode or some such name is used for the few places where it could be either. That would greatly simplify the cognitive load for the reader, allow one to get rid of gratuitous wrapping/unwrapping calls and help catch bug more easily. Thoughts?

[chirayuk]

What are a and b here—jQuery instances of DOM nodes? Do they need to be wrapped/unwrapped before calling isMatchingElement at all call sites?

[chirayuk]

In areAnimationsAllowed(element, parent, event), I take it that element and parent are always jQuery instances. What functions/methods require DOM nodes instead of jQuery elements? If it's the rare function that requires a raw DOM node, maybe just those parameters/variables can use a naming convention to indicate that (e.g. with a prefix like domNode, domParent, etc.)?

[gkalpak]

👍 for having some naming conventions to indicate whether the value is expected to be a DOM node, JQLite object or any.

I am not sure that element vs node will cut it though, since there are cases where we need to reference a parent or container.

I some projects I have tried prefixing JQLite variables with jq. E.g.:
DOM elements: element, node, body, parent
JQLite elements: jqElement, jqNode, jqBody, jqParent

(But somehow (and without being sure why) I wasn't too happy about it.)

[petebacondarwin]

I thought that the convention in the angular code base was that "wrapped" DOM nodes (i.e. jqLite elements always start with a $? See https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L20

[gkalpak]

I don't think this is consistently the case in all files (maybe just compaile.js).

In practice, I don't like that convention (although it would be indeed very convenient), because everything $-prefixed is supposed to be Angular stuff (i.e. usually injectable stuff). So having to write function someFunc($element) while $element is not injectable makes me very unhappy.

_{I particularly hate DDOs with link: function ($scope, $element) {...} even if the arguments are not injectable. E.g. compare it to controller: function ($scope, $element) {...} where the arguments are injectable (and order does not make a difference).}

But it's a matter of personal preferrence I guess and it is very common (even in Angular core 😢).

[matsko]

Merged as 64d0518

[matsko]

We'll have another PR in RC2+ which renames things properly.