Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
56f3974
fix(ivy): setting up animation properties correctly (FW-643)
AndrewKushnir Dec 5, 2018
2740e09
fixup! fix(ivy): setting up animation properties correctly (FW-643)
AndrewKushnir Dec 6, 2018
53036e9
fixup! fix(ivy): setting up animation properties correctly (FW-643)
AndrewKushnir Dec 6, 2018
7c8a9a9
fixup! fix(ivy): setting up animation properties correctly (FW-643)
AndrewKushnir Dec 6, 2018
39c966a
fixup! fix(ivy): setting up animation properties correctly (FW-643)
AndrewKushnir Dec 6, 2018
5342f0f
fixup! fix(ivy): setting up animation properties correctly (FW-643)
AndrewKushnir Dec 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 5 additions & 3 deletions packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts
View file
Open in desktop
Expand Up @@ -140,7 +140,7 @@ describe('compiler compliance: styling', () => {
},
encapsulation: 2,
data: {
animations: [{name: 'foo123'}, {name: 'trigger123'}]
animation: [{name: 'foo123'}, {name: 'trigger123'}]
}
});
`;
Expand Down Expand Up @@ -182,7 +182,7 @@ describe('compiler compliance: styling', () => {
},
encapsulation: 2,
data: {
animations: []
animation: []
}
});
`;
Expand Down Expand Up @@ -220,14 +220,16 @@ 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$);
$r3$.ɵelement(1, "div", $e1_attrs$);
$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
Expand Down
4 changes: 2 additions & 2 deletions packages/compiler/src/render3/view/compiler.ts
View file
Open in desktop
Expand Up @@ -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
Expand Down
7 changes: 4 additions & 3 deletions packages/compiler/src/render3/view/template.ts
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
];
Expand Down
26 changes: 17 additions & 9 deletions packages/core/src/render3/instructions.ts
View file
Open in desktop
Expand Up @@ -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';

Expand Down Expand Up @@ -745,10 +745,16 @@ function setUpAttributes(native: RElement, attrs: TAttributes): void {
} else {
// Standard attributes
const attrVal = attrs[i + 1];
isProc ?
(renderer as ProceduralRenderer3)
.setAttribute(native, attrName as string, attrVal as string) :
native.setAttribute(attrName as string, attrVal as string);
if (isAnimationProp(attrName)) {
if (isProc) {
(renderer as ProceduralRenderer3).setProperty(native, attrName, attrVal);
ocombe marked this conversation as resolved.
Show resolved Hide resolved
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't have an else here, this won't throw for animation triggers when no animations module is present (as it may not be a procedural renderer then). This is a deviation from ViewEngine.

Would it be an idea to introduce an instruction for animations and not consider them as regular attributes? That offers more freedom in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @JoostK. At this very moment we don't know exactly how the @animation attrs will be passed into the non-renderer case (when only document is active). We don't want to introduce anything temporary until the design is figured out therefore the single if statement case is the best we can do for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matsko That makes sense, thanks for the reply!

} else {
isProc ?
(renderer as ProceduralRenderer3)
.setAttribute(native, attrName as string, attrVal as string) :
native.setAttribute(attrName as string, attrVal as string);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider rewriting this logic to become the same as elementProperty, we could reduce the two isProc checks and calls to a single one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing this PR. You are right, there are few places where this logic exists in instructions.ts, but I'd prefer to have another cleanup/refactor PR to separate these changes (I'll take a note on that). Thank you.

i += 2;
}
}
Expand Down Expand Up @@ -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) ?
renderer.setProperty(element as RElement, propName, value) :
((element as RElement).setProperty ? (element as any).setProperty(propName, value) :
(element as any)[propName] = value);
if (isProceduralRenderer(renderer)) {
renderer.setProperty(element as RElement, propName, value);
} else if (!isAnimationProp(propName)) {
((element as RElement).setProperty ? (element as any).setProperty(propName, value) :
AndrewKushnir marked this conversation as resolved.
Show resolved Hide resolved
(element as any)[propName] = value);
}
}
}

Expand Down
6 changes: 6 additions & 0 deletions packages/core/src/render3/styling/util.ts
View file
Open in desktop
Expand Up @@ -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 {
Expand Down Expand Up @@ -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.startsWith(ANIMATION_PROP_PREFIX);
AndrewKushnir marked this conversation as resolved.
Show resolved Hide resolved
}

export function addPlayerInternal(
playerContext: PlayerContext, rootContext: RootContext, element: HTMLElement,
player: Player | null, playerContextIndex: number, ref?: any): boolean {
Expand Down
6 changes: 6 additions & 0 deletions packages/core/test/bundling/hello_world/bundle.golden_symbols.json
View file
Open in desktop
Expand Up @@ -2,6 +2,9 @@
{
"name": "ACTIVE_INDEX"
},
{
"name": "ANIMATION_PROP_PREFIX"
},
{
"name": "BINDING_INDEX"
},
Expand Down Expand Up @@ -344,6 +347,9 @@
{
"name": "invertObject"
},
{
"name": "isAnimationProp"
},
{
"name": "isComponentDef"
},
Expand Down
6 changes: 6 additions & 0 deletions packages/core/test/bundling/todo/bundle.golden_symbols.json
View file
Open in desktop
Expand Up @@ -2,6 +2,9 @@
{
"name": "ACTIVE_INDEX"
},
{
"name": "ANIMATION_PROP_PREFIX"
},
{
"name": "BINDING_INDEX"
},
Expand Down Expand Up @@ -875,6 +878,9 @@
{
"name": "invokeDirectivesHostBindings"
},
{
"name": "isAnimationProp"
},
{
"name": "isComponent"
},
Expand Down
4 changes: 2 additions & 2 deletions packages/core/test/render3/integration_spec.ts
View file
Open in desktop
Expand Up @@ -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');
});
Expand Down
8 changes: 3 additions & 5 deletions packages/platform-browser/animations/test/animation_renderer_spec.ts
View file
Open in desktop
Expand Up @@ -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(
`FW-643: Components with animations throw with "Failed to execute 'setAttribute' on 'Element'`)
fixmeIvy(`FW-800: Animation listeners are not invoked`)
.it('should flush and fire callbacks when the zone becomes stable', (async) => {
@Component({
selector: 'my-cmp',
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -283,8 +282,7 @@ import {el} from '../../testing/src/browser_util';
});
});

fixmeIvy(
`FW-643: Components with animations throw with "Failed to execute 'setAttribute' on 'Element'`)
fixmeIvy(`FW-802: Animation 'start' and 'end' hooks are invoked twice`)
.it('should provide hooks at the start and end of change detection', () => {
@Component({
selector: 'my-cmp',
Expand Down
10 changes: 4 additions & 6 deletions packages/platform-browser/animations/test/noop_animations_module_spec.ts
View file
Open in desktop
Expand Up @@ -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(); });
ocombe marked this conversation as resolved.
Show resolved Hide resolved

// TODO: remove the dummy test above ^ once the bug FW-643 has been fixed
fixmeIvy(
`FW-643: Components with animations throw with "Failed to execute 'setAttribute' on 'Element'`)
// TODO: remove the dummy test above ^ once the bug FW-800 has been fixed
fixmeIvy(`FW-800: Animation listeners are not invoked`)
.it('should flush and fire callbacks when the zone becomes stable', (async) => {
@Component({
selector: 'my-cmp',
Expand Down Expand Up @@ -55,8 +54,7 @@ import {fixmeIvy} from '@angular/private/testing';
});
});

fixmeIvy(
`FW-643: Components with animations throw with "Failed to execute 'setAttribute' on 'Element'`)
fixmeIvy(`FW-800: Animation listeners are not invoked`)
.it('should handle leave animation callbacks even if the element is destroyed in the process',
(async) => {
@Component({
Expand Down