From 79d334b1385685d0fd1451534df2ed3cbaefa0b6 Mon Sep 17 00:00:00 2001 From: dario-piotrowicz Date: Sun, 27 Feb 2022 17:55:53 +0000 Subject: [PATCH] feat(animations): provide warnings for non-animatable CSS properties (#45212) warn developers when they are trying to animate non-animatable CSS properties so that can more easily understand why something is not being animated as they would expect it to resolves #27577 PR Close #45212 --- .../public-api/animations/browser/browser.md | 2 + .../animations/browser/testing/testing.md | 2 + goldens/size-tracking/aio-payloads.json | 2 +- .../browser/src/dsl/animation_ast_builder.ts | 22 +- .../browser/src/render/animation_driver.ts | 2 + .../animations/browser/src/render/shared.ts | 6 + .../web_animations/animatable_props_set.ts | 214 ++++++++++++++++++ .../web_animations/web_animations_driver.ts | 13 +- packages/animations/browser/src/util.ts | 2 +- .../animations/browser/src/warning_helpers.ts | 12 +- .../browser/test/dsl/animation_spec.ts | 54 ++++- .../testing/src/mock_animation_driver.ts | 8 + 12 files changed, 329 insertions(+), 10 deletions(-) create mode 100644 packages/animations/browser/src/render/web_animations/animatable_props_set.ts diff --git a/goldens/public-api/animations/browser/browser.md b/goldens/public-api/animations/browser/browser.md index 44ceb43f61d..cf895239c77 100644 --- a/goldens/public-api/animations/browser/browser.md +++ b/goldens/public-api/animations/browser/browser.md @@ -20,6 +20,8 @@ export abstract class AnimationDriver { // (undocumented) abstract query(element: any, selector: string, multi: boolean): any[]; // (undocumented) + abstract validateAnimatableStyleProperty?: (prop: string) => boolean; + // (undocumented) abstract validateStyleProperty(prop: string): boolean; } diff --git a/goldens/public-api/animations/browser/testing/testing.md b/goldens/public-api/animations/browser/testing/testing.md index 25f03291ac1..39c6d223cd5 100644 --- a/goldens/public-api/animations/browser/testing/testing.md +++ b/goldens/public-api/animations/browser/testing/testing.md @@ -26,6 +26,8 @@ export class MockAnimationDriver implements AnimationDriver { // (undocumented) query(element: any, selector: string, multi: boolean): any[]; // (undocumented) + validateAnimatableStyleProperty(prop: string): boolean; + // (undocumented) validateStyleProperty(prop: string): boolean; } diff --git a/goldens/size-tracking/aio-payloads.json b/goldens/size-tracking/aio-payloads.json index ec7ce775955..23ea5e730a3 100755 --- a/goldens/size-tracking/aio-payloads.json +++ b/goldens/size-tracking/aio-payloads.json @@ -15,7 +15,7 @@ "master": { "uncompressed": { "runtime": 4343, - "main": 450268, + "main": 450342, "polyfills": 33869, "styles": 70416, "light-theme": 77582, diff --git a/packages/animations/browser/src/dsl/animation_ast_builder.ts b/packages/animations/browser/src/dsl/animation_ast_builder.ts index c86879e8830..f146d4a0a65 100644 --- a/packages/animations/browser/src/dsl/animation_ast_builder.ts +++ b/packages/animations/browser/src/dsl/animation_ast_builder.ts @@ -11,7 +11,7 @@ import {invalidDefinition, invalidKeyframes, invalidOffset, invalidParallelAnima import {AnimationDriver} from '../render/animation_driver'; import {getOrSetDefaultValue} from '../render/shared'; import {convertToMap, copyObj, extractStyleParams, iteratorToArray, NG_ANIMATING_SELECTOR, NG_TRIGGER_SELECTOR, normalizeAnimationEntry, resolveTiming, SUBSTITUTION_EXPR_START, validateStyleParams, visitDslNode} from '../util'; -import {pushUnrecognizedPropertiesWarning} from '../warning_helpers'; +import {pushNonAnimatablePropertiesWarning, pushUnrecognizedPropertiesWarning} from '../warning_helpers'; import {AnimateAst, AnimateChildAst, AnimateRefAst, Ast, DynamicTimingAst, GroupAst, KeyframesAst, QueryAst, ReferenceAst, SequenceAst, StaggerAst, StateAst, StyleAst, TimingAst, TransitionAst, TriggerAst} from './animation_ast'; import {AnimationDslVisitor} from './animation_dsl_visitor'; @@ -80,6 +80,15 @@ export class AnimationAstBuilderVisitor implements AnimationDslVisitor { [...context.unsupportedCSSPropertiesFound.keys()], ); } + + if ((typeof ngDevMode === 'undefined' || ngDevMode) && + context.nonAnimatableCSSPropertiesFound.size) { + pushNonAnimatablePropertiesWarning( + warnings, + [...context.nonAnimatableCSSPropertiesFound.keys()], + ); + } + return ast; } @@ -311,6 +320,16 @@ export class AnimationAstBuilderVisitor implements AnimationDslVisitor { return; } + if ((typeof ngDevMode === 'undefined' || ngDevMode) && + this._driver.validateAnimatableStyleProperty) { + if (!this._driver.validateAnimatableStyleProperty(prop)) { + context.nonAnimatableCSSPropertiesFound.add(prop); + // note: non animatable properties are not removed for the tuple just in case they are + // categorized as non animatable but can actually be animated + return; + } + } + // This is guaranteed to have a defined Map at this querySelector location making it // safe to add the assertion here. It is set as a default empty map in prior methods. const collectedStyles = context.collectedStyles.get(context.currentQuerySelector!)!; @@ -514,6 +533,7 @@ export class AnimationAstBuilderContext { public collectedStyles = new Map>(); public options: AnimationOptions|null = null; public unsupportedCSSPropertiesFound: Set = new Set(); + public readonly nonAnimatableCSSPropertiesFound: Set = new Set(); constructor(public errors: Error[]) {} } diff --git a/packages/animations/browser/src/render/animation_driver.ts b/packages/animations/browser/src/render/animation_driver.ts index 6654d9cd35a..71aa51a9a57 100644 --- a/packages/animations/browser/src/render/animation_driver.ts +++ b/packages/animations/browser/src/render/animation_driver.ts @@ -56,6 +56,8 @@ export abstract class AnimationDriver { abstract validateStyleProperty(prop: string): boolean; + abstract validateAnimatableStyleProperty?: (prop: string) => boolean; + /** * @deprecated No longer in use. Will be removed. */ diff --git a/packages/animations/browser/src/render/shared.ts b/packages/animations/browser/src/render/shared.ts index b4bf952648a..963fe3af459 100644 --- a/packages/animations/browser/src/render/shared.ts +++ b/packages/animations/browser/src/render/shared.ts @@ -11,6 +11,8 @@ import {AnimationStyleNormalizer} from '../../src/dsl/style_normalization/animat import {AnimationDriver} from '../../src/render/animation_driver'; import {animationFailed} from '../error_helpers'; +import {ANIMATABLE_PROP_SET} from './web_animations/animatable_props_set'; + // We don't include ambient node types here since @angular/animations/browser // is meant to target the browser so technically it should not depend on node // types. `process` is just declared locally here as a result. @@ -209,6 +211,10 @@ export function validateStyleProperty(prop: string): boolean { return result; } +export function validateWebAnimatableStyleProperty(prop: string): boolean { + return ANIMATABLE_PROP_SET.has(prop); +} + export function getBodyNode(): any|null { if (typeof document != 'undefined') { return document.body; diff --git a/packages/animations/browser/src/render/web_animations/animatable_props_set.ts b/packages/animations/browser/src/render/web_animations/animatable_props_set.ts new file mode 100644 index 00000000000..5da2a3b9029 --- /dev/null +++ b/packages/animations/browser/src/render/web_animations/animatable_props_set.ts @@ -0,0 +1,214 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +/** + * Set of all animatable CSS properties + * + * @see https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_animated_properties + */ +export const ANIMATABLE_PROP_SET = new Set([ + '-moz-outline-radius', + '-moz-outline-radius-bottomleft', + '-moz-outline-radius-bottomright', + '-moz-outline-radius-topleft', + '-moz-outline-radius-topright', + '-ms-grid-columns', + '-ms-grid-rows', + '-webkit-line-clamp', + '-webkit-text-fill-color', + '-webkit-text-stroke', + '-webkit-text-stroke-color', + 'accent-color', + 'all', + 'backdrop-filter', + 'background', + 'background-color', + 'background-position', + 'background-size', + 'block-size', + 'border', + 'border-block-end', + 'border-block-end-color', + 'border-block-end-width', + 'border-block-start', + 'border-block-start-color', + 'border-block-start-width', + 'border-bottom', + 'border-bottom-color', + 'border-bottom-left-radius', + 'border-bottom-right-radius', + 'border-bottom-width', + 'border-color', + 'border-end-end-radius', + 'border-end-start-radius', + 'border-image-outset', + 'border-image-slice', + 'border-image-width', + 'border-inline-end', + 'border-inline-end-color', + 'border-inline-end-width', + 'border-inline-start', + 'border-inline-start-color', + 'border-inline-start-width', + 'border-left', + 'border-left-color', + 'border-left-width', + 'border-radius', + 'border-right', + 'border-right-color', + 'border-right-width', + 'border-start-end-radius', + 'border-start-start-radius', + 'border-top', + 'border-top-color', + 'border-top-left-radius', + 'border-top-right-radius', + 'border-top-width', + 'border-width', + 'bottom', + 'box-shadow', + 'caret-color', + 'clip', + 'clip-path', + 'color', + 'column-count', + 'column-gap', + 'column-rule', + 'column-rule-color', + 'column-rule-width', + 'column-width', + 'columns', + 'filter', + 'flex', + 'flex-basis', + 'flex-grow', + 'flex-shrink', + 'font', + 'font-size', + 'font-size-adjust', + 'font-stretch', + 'font-variation-settings', + 'font-weight', + 'gap', + 'grid-column-gap', + 'grid-gap', + 'grid-row-gap', + 'grid-template-columns', + 'grid-template-rows', + 'height', + 'inline-size', + 'input-security', + 'inset', + 'inset-block', + 'inset-block-end', + 'inset-block-start', + 'inset-inline', + 'inset-inline-end', + 'inset-inline-start', + 'left', + 'letter-spacing', + 'line-clamp', + 'line-height', + 'margin', + 'margin-block-end', + 'margin-block-start', + 'margin-bottom', + 'margin-inline-end', + 'margin-inline-start', + 'margin-left', + 'margin-right', + 'margin-top', + 'mask', + 'mask-border', + 'mask-position', + 'mask-size', + 'max-block-size', + 'max-height', + 'max-inline-size', + 'max-lines', + 'max-width', + 'min-block-size', + 'min-height', + 'min-inline-size', + 'min-width', + 'object-position', + 'offset', + 'offset-anchor', + 'offset-distance', + 'offset-path', + 'offset-position', + 'offset-rotate', + 'opacity', + 'order', + 'outline', + 'outline-color', + 'outline-offset', + 'outline-width', + 'padding', + 'padding-block-end', + 'padding-block-start', + 'padding-bottom', + 'padding-inline-end', + 'padding-inline-start', + 'padding-left', + 'padding-right', + 'padding-top', + 'perspective', + 'perspective-origin', + 'right', + 'rotate', + 'row-gap', + 'scale', + 'scroll-margin', + 'scroll-margin-block', + 'scroll-margin-block-end', + 'scroll-margin-block-start', + 'scroll-margin-bottom', + 'scroll-margin-inline', + 'scroll-margin-inline-end', + 'scroll-margin-inline-start', + 'scroll-margin-left', + 'scroll-margin-right', + 'scroll-margin-top', + 'scroll-padding', + 'scroll-padding-block', + 'scroll-padding-block-end', + 'scroll-padding-block-start', + 'scroll-padding-bottom', + 'scroll-padding-inline', + 'scroll-padding-inline-end', + 'scroll-padding-inline-start', + 'scroll-padding-left', + 'scroll-padding-right', + 'scroll-padding-top', + 'scroll-snap-coordinate', + 'scroll-snap-destination', + 'scrollbar-color', + 'shape-image-threshold', + 'shape-margin', + 'shape-outside', + 'tab-size', + 'text-decoration', + 'text-decoration-color', + 'text-decoration-thickness', + 'text-emphasis', + 'text-emphasis-color', + 'text-indent', + 'text-shadow', + 'text-underline-offset', + 'top', + 'transform', + 'transform-origin', + 'translate', + 'vertical-align', + 'visibility', + 'width', + 'word-spacing', + 'z-index', + 'zoom', +]); diff --git a/packages/animations/browser/src/render/web_animations/web_animations_driver.ts b/packages/animations/browser/src/render/web_animations/web_animations_driver.ts index e67be7d2b93..dcc124363b4 100644 --- a/packages/animations/browser/src/render/web_animations/web_animations_driver.ts +++ b/packages/animations/browser/src/render/web_animations/web_animations_driver.ts @@ -7,9 +7,9 @@ */ import {AnimationPlayer, ɵStyleDataMap} from '@angular/animations'; -import {allowPreviousPlayerStylesMerge, balancePreviousStylesIntoKeyframes, copyStyles, normalizeKeyframes} from '../../util'; +import {allowPreviousPlayerStylesMerge, balancePreviousStylesIntoKeyframes, camelCaseToDashCase, copyStyles, normalizeKeyframes} from '../../util'; import {AnimationDriver} from '../animation_driver'; -import {containsElement, getParentElement, invokeQuery, validateStyleProperty} from '../shared'; +import {containsElement, getParentElement, invokeQuery, validateStyleProperty, validateWebAnimatableStyleProperty} from '../shared'; import {packageNonAnimatableStyles} from '../special_cased_styles'; import {WebAnimationsPlayer} from './web_animations_player'; @@ -19,6 +19,15 @@ export class WebAnimationsDriver implements AnimationDriver { return validateStyleProperty(prop); } + validateAnimatableStyleProperty(prop: string): boolean { + // Perform actual validation in dev mode only, in prod mode this check is a noop. + if (ngDevMode) { + const cssProp = camelCaseToDashCase(prop); + return validateWebAnimatableStyleProperty(cssProp); + } + return true; + } + matchesElement(_element: any, _selector: string): boolean { // This method is deprecated and no longer in use so we return false. return false; diff --git a/packages/animations/browser/src/util.ts b/packages/animations/browser/src/util.ts index 7c78b800754..71cef2841f8 100644 --- a/packages/animations/browser/src/util.ts +++ b/packages/animations/browser/src/util.ts @@ -275,7 +275,7 @@ export function dashCaseToCamelCase(input: string): string { return input.replace(DASH_CASE_REGEXP, (...m: any[]) => m[1].toUpperCase()); } -function camelCaseToDashCase(input: string): string { +export function camelCaseToDashCase(input: string): string { return input.replace(/([a-z])([A-Z])/g, '$1-$2').toLowerCase(); } diff --git a/packages/animations/browser/src/warning_helpers.ts b/packages/animations/browser/src/warning_helpers.ts index d93ca15dd7e..7d51e80b64d 100644 --- a/packages/animations/browser/src/warning_helpers.ts +++ b/packages/animations/browser/src/warning_helpers.ts @@ -36,8 +36,14 @@ export function triggerParsingWarnings(name: string, warnings: string[]): void { export function pushUnrecognizedPropertiesWarning(warnings: string[], props: string[]): void { if (ngDevMode && props.length) { - warnings.push( - `The provided CSS properties are not recognized properties supported for animations: ${ - props.join(', ')}`); + warnings.push(`The following provided properties are not recognized: ${props.join(', ')}`); + } +} + +export function pushNonAnimatablePropertiesWarning(warnings: string[], props: string[]): void { + if (props.length) { + warnings.push(`The following provided properties are not animatable: ${ + props.join( + ', ')}\n (see: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_animated_properties)`); } } diff --git a/packages/animations/browser/test/dsl/animation_spec.ts b/packages/animations/browser/test/dsl/animation_spec.ts index 4249b13c5fa..ab2f32612d1 100644 --- a/packages/animations/browser/test/dsl/animation_spec.ts +++ b/packages/animations/browser/test/dsl/animation_spec.ts @@ -222,7 +222,7 @@ function createDiv() { const steps = [animate(1000, style({abc: '500px'}))]; expect(getValidationWarningsForAnimationSequence(steps)).toEqual([ - 'The provided CSS properties are not recognized properties supported for animations: abc' + 'The following provided properties are not recognized: abc' ]); }); @@ -236,7 +236,57 @@ function createDiv() { ]; expect(getValidationWarningsForAnimationSequence(steps)).toEqual([ - 'The provided CSS properties are not recognized properties supported for animations: 123, abc, xyz' + 'The following provided properties are not recognized: 123, abc, xyz' + ]); + }); + + it('should provide a warning if a non-animatable CSS property is used in the animation', + () => { + const steps = [animate(1000, style({display: 'block'}))]; + + expect(getValidationWarningsForAnimationSequence(steps)).toEqual([ + 'The following provided properties are not animatable: display' + + '\n (see: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_animated_properties)' + ]); + }); + + it('should not provide a non-animatable warning if an animatable CSS property is used in the animation', + () => { + const steps = [animate('1.5s', style({borderRadius: '1rem'}))]; + + expect(getValidationWarningsForAnimationSequence(steps)).toEqual([]); + }); + + it('should provide a warning if multiple non-animatable CSS properties are used in the animation', + () => { + const steps = [ + state('state', style({ + display: 'block', + })), + style({textAlign: 'right'}), animate(1000, style({'font-family': 'sans-serif'})) + ]; + + expect(getValidationWarningsForAnimationSequence(steps)).toEqual([ + 'The following provided properties are not animatable: display, textAlign, font-family' + + '\n (see: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_animated_properties)' + ]); + }); + + it('should provide a warnings if both invalid and non-animatable CSS properties are used in the animation', + () => { + const steps = [ + state('state', style({ + display: 'block', + abc: 123, + })), + style({textAlign: 'right'}), + animate(1000, style({'font-family': 'sans-serif', xyz: 456})) + ]; + + expect(getValidationWarningsForAnimationSequence(steps)).toEqual([ + 'The following provided properties are not recognized: abc, xyz', + 'The following provided properties are not animatable: display, textAlign, font-family' + + '\n (see: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_animated_properties)' ]); }); diff --git a/packages/animations/browser/testing/src/mock_animation_driver.ts b/packages/animations/browser/testing/src/mock_animation_driver.ts index f42bd30d130..46fd394b6d6 100644 --- a/packages/animations/browser/testing/src/mock_animation_driver.ts +++ b/packages/animations/browser/testing/src/mock_animation_driver.ts @@ -8,6 +8,9 @@ import {AnimationPlayer, AUTO_STYLE, NoopAnimationPlayer, ɵStyleDataMap} from '@angular/animations'; import {AnimationDriver, ɵallowPreviousPlayerStylesMerge as allowPreviousPlayerStylesMerge, ɵcontainsElement as containsElement, ɵgetParentElement as getParentElement, ɵinvokeQuery as invokeQuery, ɵnormalizeKeyframes as normalizeKeyframes, ɵvalidateStyleProperty as validateStyleProperty,} from '@angular/animations/browser'; +import {validateWebAnimatableStyleProperty} from '../../src/render/shared'; +import {camelCaseToDashCase} from '../../src/util'; + /** * @publicApi */ @@ -18,6 +21,11 @@ export class MockAnimationDriver implements AnimationDriver { return validateStyleProperty(prop); } + validateAnimatableStyleProperty(prop: string): boolean { + const cssProp = camelCaseToDashCase(prop); + return validateWebAnimatableStyleProperty(cssProp); + } + matchesElement(_element: any, _selector: string): boolean { return false; }