fix(ivy): setting up animation properties correctly (FW-643) (#27496)

Prior to this change, animation properties were defined as element attributes, which caused errors at runtime. Now all animation-related attributes are defined as element properties.

Also as a part of this update, we start to account for bindings used in animations, which was previously missing.

PR Close #27496

packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts

@@ -140,7 +140,7 @@ describe('compiler compliance: styling', () => {
           },
           encapsulation: 2,
           data: {
-            animations: [{name: 'foo123'}, {name: 'trigger123'}]
+            animation: [{name: 'foo123'}, {name: 'trigger123'}]
           }
         });
       `;
@@ -182,7 +182,7 @@ describe('compiler compliance: styling', () => {
           },
           encapsulation: 2,
           data: {
-            animations: []
+            animation: []
           }
         });
       `;
@@ -220,6 +220,8 @@ describe('compiler compliance: styling', () => {
         …
         MyComponent.ngComponentDef = $r3$.ɵdefineComponent({
           …
+          consts: 3,
+          vars: 1,
           template:  function MyComponent_Template(rf, $ctx$) {
             if (rf & 1) {
               $r3$.ɵelement(0, "div", $e0_attrs$);
@@ -227,7 +229,7 @@ describe('compiler compliance: styling', () => {
               $r3$.ɵelement(2, "div", $e2_attrs$);
             }
             if (rf & 2) {
-              $r3$.ɵelementAttribute(0, "@foo", $r3$.ɵbind(ctx.exp));
+              $r3$.ɵelementProperty(0, "@foo", $r3$.ɵbind(ctx.exp));
             }
           },
           encapsulation: 2

packages/compiler/src/render3/view/compiler.ts

@@ -307,10 +307,10 @@ export function compileComponentFromMetadata(
     definitionMap.set('encapsulation', o.literal(meta.encapsulation));
   }
 
-  // e.g. `animations: [trigger('123', [])]`
+  // e.g. `animation: [trigger('123', [])]`
   if (meta.animations !== null) {
     definitionMap.set(
-        'data', o.literalMap([{key: 'animations', value: meta.animations, quoted: false}]));
+        'data', o.literalMap([{key: 'animation', value: meta.animations, quoted: false}]));
   }
 
   // On the type side, remove newlines from the selector as it will need to fit into a TypeScript

packages/compiler/src/render3/view/template.ts

@@ -41,11 +41,11 @@ import {CONTEXT_NAME, IMPLICIT_REFERENCE, NON_BINDABLE_ATTR, REFERENCE_PREFIX, R
 function mapBindingToInstruction(type: BindingType): o.ExternalReference|undefined {
   switch (type) {
     case BindingType.Property:
+    case BindingType.Animation:
       return R3.elementProperty;
     case BindingType.Class:
       return R3.elementClassProp;
     case BindingType.Attribute:
-    case BindingType.Animation:
       return R3.elementAttribute;
     default:
       return undefined;
@@ -622,10 +622,11 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
       const instruction = mapBindingToInstruction(input.type);
       if (input.type === BindingType.Animation) {
         const value = input.value.visit(this._valueConverter);
-        // setAttribute without a value doesn't make any sense
+        // setProperty without a value doesn't make any sense
         if (value.name || value.value) {
+          this.allocateBindingSlots(value);
           const name = prepareSyntheticAttributeName(input.name);
-          this.updateInstruction(input.sourceSpan, R3.elementAttribute, () => {
+          this.updateInstruction(input.sourceSpan, R3.elementProperty, () => {
             return [
               o.literal(elementIndex), o.literal(name), this.convertPropertyBinding(implicit, value)
             ];

packages/core/src/render3/instructions.ts

@@ -39,7 +39,7 @@ import {isNodeMatchingSelectorList, matchingSelectorIndex} from './node_selector
 import {decreaseElementDepthCount, enterView, getBindingsEnabled, getCheckNoChangesMode, getContextLView, getCreationMode, getCurrentDirectiveDef, getElementDepthCount, getFirstTemplatePass, getIsParent, getLView, getPreviousOrParentTNode, increaseElementDepthCount, leaveView, nextContextImpl, resetComponentState, setBindingRoot, setCheckNoChangesMode, setCurrentDirectiveDef, setFirstTemplatePass, setIsParent, setPreviousOrParentTNode} from './state';
 import {createStylingContextTemplate, renderStyleAndClassBindings, setStyle, updateClassProp as updateElementClassProp, updateStyleProp as updateElementStyleProp, updateStylingMap} from './styling/class_and_style_bindings';
 import {BoundPlayerFactory} from './styling/player_factory';
-import {getStylingContext} from './styling/util';
+import {getStylingContext, isAnimationProp} from './styling/util';
 import {NO_CHANGE} from './tokens';
 import {getComponentViewByIndex, getNativeByIndex, getNativeByTNode, getRootContext, getRootView, getTNode, isComponent, isComponentDef, loadInternal, readElementValue, readPatchedLView, stringify} from './util';
 
@@ -745,10 +745,16 @@ function setUpAttributes(native: RElement, attrs: TAttributes): void {
       } else {
         // Standard attributes
         const attrVal = attrs[i + 1];
-        isProc ?
+        if (isAnimationProp(attrName)) {
-            (renderer as ProceduralRenderer3)
+          if (isProc) {
-                .setAttribute(native, attrName as string, attrVal as string) :
+            (renderer as ProceduralRenderer3).setProperty(native, attrName, attrVal);
-            native.setAttribute(attrName as string, attrVal as string);
+          }
+        } else {
+          isProc ?
+              (renderer as ProceduralRenderer3)
+                  .setAttribute(native, attrName as string, attrVal as string) :
+              native.setAttribute(attrName as string, attrVal as string);
+        }
         i += 2;
       }
     }
@@ -967,10 +973,12 @@ export function elementProperty<T>(
     // is risky, so sanitization can be done without further checks.
     value = sanitizer != null ? (sanitizer(value) as any) : value;
     ngDevMode && ngDevMode.rendererSetProperty++;
-    isProceduralRenderer(renderer) ?
+    if (isProceduralRenderer(renderer)) {
-        renderer.setProperty(element as RElement, propName, value) :
+      renderer.setProperty(element as RElement, propName, value);
-        ((element as RElement).setProperty ? (element as any).setProperty(propName, value) :
+    } else if (!isAnimationProp(propName)) {
-                                             (element as any)[propName] = value);
+      (element as RElement).setProperty ? (element as any).setProperty(propName, value) :
+                                          (element as any)[propName] = value;
+    }
   }
 }
 

packages/core/src/render3/styling/util.ts

@@ -19,6 +19,8 @@ import {getTNode} from '../util';
 
 import {CorePlayerHandler} from './core_player_handler';
 
+const ANIMATION_PROP_PREFIX = '@';
+
 export function createEmptyStylingContext(
     element?: RElement | null, sanitizer?: StyleSanitizeFn | null,
     initialStylingValues?: InitialStyles): StylingContext {
@@ -91,6 +93,10 @@ export function isStylingContext(value: any): value is StylingContext {
       typeof value[ACTIVE_INDEX] !== 'number';
 }
 
+export function isAnimationProp(name: string): boolean {
+  return name[0] === ANIMATION_PROP_PREFIX;
+}
+
 export function addPlayerInternal(
     playerContext: PlayerContext, rootContext: RootContext, element: HTMLElement,
     player: Player | null, playerContextIndex: number, ref?: any): boolean {

packages/core/test/bundling/hello_world/bundle.golden_symbols.json

@@ -2,6 +2,9 @@
   {
     "name": "ACTIVE_INDEX"
   },
+  {
+    "name": "ANIMATION_PROP_PREFIX"
+  },
   {
     "name": "BINDING_INDEX"
   },
@@ -344,6 +347,9 @@
   {
     "name": "invertObject"
   },
+  {
+    "name": "isAnimationProp"
+  },
   {
     "name": "isComponentDef"
   },

packages/core/test/bundling/todo/bundle.golden_symbols.json

@@ -2,6 +2,9 @@
   {
     "name": "ACTIVE_INDEX"
   },
+  {
+    "name": "ANIMATION_PROP_PREFIX"
+  },
   {
     "name": "BINDING_INDEX"
   },
@@ -875,6 +878,9 @@
   {
     "name": "invokeDirectivesHostBindings"
   },
+  {
+    "name": "isAnimationProp"
+  },
   {
     "name": "isComponent"
   },

packages/core/test/render3/integration_spec.ts

@@ -1784,7 +1784,7 @@ describe('render3 integration test', () => {
           consts: 0,
           vars: 0,
           data: {
-            animations: [
+            animation: [
               animA,
               animB,
             ],
@@ -1797,7 +1797,7 @@ describe('render3 integration test', () => {
       const rendererFactory = new ProxyRenderer3Factory();
       new ComponentFixture(AnimComp, {rendererFactory});
 
-      const capturedAnimations = rendererFactory.lastCapturedType !.data !['animations'];
+      const capturedAnimations = rendererFactory.lastCapturedType !.data !['animation'];
       expect(Array.isArray(capturedAnimations)).toBeTruthy();
       expect(capturedAnimations.length).toEqual(2);
       expect(capturedAnimations).toContain(animA);
@@ -1811,7 +1811,7 @@ describe('render3 integration test', () => {
           consts: 0,
           vars: 0,
           data: {
-            animations: [],
+            animation: [],
           },
           selectors: [['foo']],
           factory: () => new AnimComp(),
@@ -1821,7 +1821,7 @@ describe('render3 integration test', () => {
       const rendererFactory = new ProxyRenderer3Factory();
       new ComponentFixture(AnimComp, {rendererFactory});
       const data = rendererFactory.lastCapturedType !.data;
-      expect(data.animations).toEqual([]);
+      expect(data.animation).toEqual([]);
     });
 
     it('should allow [@trigger] bindings to be picked up by the underlying renderer', () => {
@@ -1876,13 +1876,13 @@ describe('render3 integration test', () => {
            });
          }
 
-         const rendererFactory = new MockRendererFactory(['setAttribute']);
+         const rendererFactory = new MockRendererFactory(['setProperty']);
          const fixture = new ComponentFixture(AnimComp, {rendererFactory});
 
          const renderer = rendererFactory.lastRenderer !;
          fixture.update();
 
-         const spy = renderer.spies['setAttribute'];
+         const spy = renderer.spies['setProperty'];
          const [elm, attr, value] = spy.calls.mostRecent().args;
          expect(attr).toEqual('@fooAnimation');
        });

packages/platform-browser/animations/test/animation_renderer_spec.ts

@@ -120,8 +120,7 @@ import {el} from '../../testing/src/browser_util';
       // these tests are only mean't to be run within the DOM
       if (isNode) return;
 
-      fixmeIvy(
+      fixmeIvy(`FW-800: Animation listeners are not invoked`)
-          `FW-643: Components with animations throw with "Failed to execute 'setAttribute' on 'Element'`)
           .it('should flush and fire callbacks when the zone becomes stable', (async) => {
             @Component({
               selector: 'my-cmp',
@@ -198,7 +197,7 @@ import {el} from '../../testing/src/browser_util';
          });
 
       fixmeIvy(
-          `FW-643: Components with animations throw with "Failed to execute 'setAttribute' on 'Element'`)
+          `FW-801: Components with animations throw with "Cannot read property 'hostElement' of undefined" error`)
           .it('should only queue up dom removals if the element itself contains a valid leave animation',
               () => {
                 @Component({
@@ -283,8 +282,7 @@ import {el} from '../../testing/src/browser_util';
       });
     });
 
-    fixmeIvy(
+    fixmeIvy(`FW-802: Animation 'start' and 'end' hooks are invoked twice`)
-        `FW-643: Components with animations throw with "Failed to execute 'setAttribute' on 'Element'`)
         .it('should provide hooks at the start and end of change detection', () => {
           @Component({
             selector: 'my-cmp',

packages/platform-browser/animations/test/noop_animations_module_spec.ts

@@ -16,11 +16,10 @@ import {fixmeIvy} from '@angular/private/testing';
   describe('NoopAnimationsModule', () => {
     beforeEach(() => { TestBed.configureTestingModule({imports: [NoopAnimationsModule]}); });
 
-    it('should be removed once FW-643 is fixed', () => { expect(true).toBeTruthy(); });
+    it('should be removed once FW-800 is fixed', () => { expect(true).toBeTruthy(); });
 
-    // TODO: remove the dummy test above ^ once the bug FW-643 has been fixed
+    // TODO: remove the dummy test above ^ once the bug FW-800 has been fixed
-    fixmeIvy(
+    fixmeIvy(`FW-800: Animation listeners are not invoked`)
-        `FW-643: Components with animations throw with "Failed to execute 'setAttribute' on 'Element'`)
         .it('should flush and fire callbacks when the zone becomes stable', (async) => {
           @Component({
             selector: 'my-cmp',
@@ -55,8 +54,7 @@ import {fixmeIvy} from '@angular/private/testing';
           });
         });
 
-    fixmeIvy(
+    fixmeIvy(`FW-800: Animation listeners are not invoked`)
-        `FW-643: Components with animations throw with "Failed to execute 'setAttribute' on 'Element'`)
         .it('should handle leave animation callbacks even if the element is destroyed in the process',
             (async) => {
               @Component({