Skip to content

Commit

Permalink
refactor(animations): remove unnecessary code (#44378)
Browse files Browse the repository at this point in the history
* The `TransitionAnimationEngine` had a fallback where it would store classes directly on a node if it doesn't have a `classList`. Presumably this is to support old browsers or if an animation is set on something like `ng-container`. This information was never used for anything since `containsClass` was never called. These changes simplify the logic to just a null check.
* Deprecates the `AnimationDriver.matchesElement` method, because it was only used in one place which can be replaced with `classList.contains`. We can't remove the method completely, because `AnimationDriver` is a public API. We also can't turn it into a method on the base class in order to remove it from the sub-classes, because it can break apps using `noImplicitOverride` while extending `AnimationDriver`.

PR Close #44378
  • Loading branch information
crisbeto authored and alxhub committed Dec 8, 2021
1 parent 80ab604 commit 6df314f
Show file tree
Hide file tree
Showing 16 changed files with 32 additions and 72 deletions.
3 changes: 1 addition & 2 deletions goldens/public-api/animations/browser/browser.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export abstract class AnimationDriver {
abstract computeStyle(element: any, prop: string, defaultValue?: string): string;
// (undocumented)
abstract containsElement(elm1: any, elm2: any): boolean;
// (undocumented)
// @deprecated (undocumented)
abstract matchesElement(element: any, selector: string): boolean;
// (undocumented)
static NOOP: AnimationDriver;
Expand All @@ -24,7 +24,6 @@ export abstract class AnimationDriver {
abstract validateStyleProperty(prop: string): boolean;
}


// (No @packageDocumentation comment for this package)

```
3 changes: 1 addition & 2 deletions goldens/public-api/animations/browser/testing/testing.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export class MockAnimationDriver implements AnimationDriver {
// (undocumented)
static log: AnimationPlayer[];
// (undocumented)
matchesElement(element: any, selector: string): boolean;
matchesElement(_element: any, _selector: string): boolean;
// (undocumented)
query(element: any, selector: string, multi: boolean): any[];
// (undocumented)
Expand Down Expand Up @@ -68,7 +68,6 @@ export class MockAnimationPlayer extends NoopAnimationPlayer {
reset(): void;
}


// (No @packageDocumentation comment for this package)

```
2 changes: 1 addition & 1 deletion packages/animations/browser/src/private_export.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export {NoopAnimationDriver as ɵNoopAnimationDriver} from './render/animation_d
export {AnimationEngine as ɵAnimationEngine} from './render/animation_engine_next';
export {CssKeyframesDriver as ɵCssKeyframesDriver} from './render/css_keyframes/css_keyframes_driver';
export {CssKeyframesPlayer as ɵCssKeyframesPlayer} from './render/css_keyframes/css_keyframes_player';
export {containsElement as ɵcontainsElement, invokeQuery as ɵinvokeQuery, matchesElement as ɵmatchesElement, validateStyleProperty as ɵvalidateStyleProperty} from './render/shared';
export {containsElement as ɵcontainsElement, invokeQuery as ɵinvokeQuery, validateStyleProperty as ɵvalidateStyleProperty} from './render/shared';
export {supportsWebAnimations as ɵsupportsWebAnimations, WebAnimationsDriver as ɵWebAnimationsDriver} from './render/web_animations/web_animations_driver';
export {WebAnimationsPlayer as ɵWebAnimationsPlayer} from './render/web_animations/web_animations_player';
export {allowPreviousPlayerStylesMerge as ɵallowPreviousPlayerStylesMerge} from './util';
10 changes: 7 additions & 3 deletions packages/animations/browser/src/render/animation_driver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import {AnimationPlayer, NoopAnimationPlayer} from '@angular/animations';
import {Injectable} from '@angular/core';

import {containsElement, invokeQuery, matchesElement, validateStyleProperty} from './shared';
import {containsElement, invokeQuery, validateStyleProperty} from './shared';

/**
* @publicApi
Expand All @@ -19,8 +19,9 @@ export class NoopAnimationDriver implements AnimationDriver {
return validateStyleProperty(prop);
}

matchesElement(element: any, selector: string): boolean {
return matchesElement(element, selector);
matchesElement(_element: any, _selector: string): boolean {
// This method is deprecated and no longer in use so we return false.
return false;
}

containsElement(elm1: any, elm2: any): boolean {
Expand Down Expand Up @@ -51,6 +52,9 @@ export abstract class AnimationDriver {

abstract validateStyleProperty(prop: string): boolean;

/**
* @deprecated No longer in use. Will be removed.
*/
abstract matchesElement(element: any, selector: string): boolean;

abstract containsElement(elm1: any, elm2: any): boolean;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {AnimationPlayer, ɵStyleData} from '@angular/animations';

import {allowPreviousPlayerStylesMerge, balancePreviousStylesIntoKeyframes, computeStyle} from '../../util';
import {AnimationDriver} from '../animation_driver';
import {containsElement, hypenatePropsObject, invokeQuery, matchesElement, validateStyleProperty} from '../shared';
import {containsElement, hypenatePropsObject, invokeQuery, validateStyleProperty} from '../shared';
import {packageNonAnimatableStyles} from '../special_cased_styles';

import {CssKeyframesPlayer} from './css_keyframes_player';
Expand All @@ -25,8 +25,9 @@ export class CssKeyframesDriver implements AnimationDriver {
return validateStyleProperty(prop);
}

matchesElement(element: any, selector: string): boolean {
return matchesElement(element, selector);
matchesElement(_element: any, _selector: string): boolean {
// This method is deprecated and no longer in use so we return false.
return false;
}

containsElement(elm1: any, elm2: any): boolean {
Expand Down
18 changes: 0 additions & 18 deletions packages/animations/browser/src/render/shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,6 @@ export function parseTimelineCommand(command: string): [string, string] {
}

let _contains: (elm1: any, elm2: any) => boolean = (elm1: any, elm2: any) => false;
let _matches: (element: any, selector: string) => boolean = (element: any, selector: string) =>
false;
let _query: (element: any, selector: string, multi: boolean) => any[] =
(element: any, selector: string, multi: boolean) => {
return [];
Expand All @@ -174,21 +172,6 @@ if (_isNode || typeof Element !== 'undefined') {
};
}

_matches = (() => {
if (_isNode || Element.prototype.matches) {
return (element: any, selector: string) => element.matches(selector);
} else {
const proto = Element.prototype as any;
const fn = proto.matchesSelector || proto.mozMatchesSelector || proto.msMatchesSelector ||
proto.oMatchesSelector || proto.webkitMatchesSelector;
if (fn) {
return (element: any, selector: string) => fn.apply(element, [selector]);
} else {
return _matches;
}
}
})();

_query = (element: any, selector: string, multi: boolean): any[] => {
let results: any[] = [];
if (multi) {
Expand Down Expand Up @@ -246,7 +229,6 @@ export function getBodyNode(): any|null {
return null;
}

export const matchesElement = _matches;
export const containsElement = _contains;
export const invokeQuery = _query;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -836,7 +836,7 @@ export class TransitionAnimationEngine {
this._onRemovalComplete(element, details.setForRemoval);
}

if (this.driver.matchesElement(element, DISABLED_SELECTOR)) {
if (element.classList?.contains(DISABLED_CLASSNAME)) {
this.markElementAsDisabled(element, false);
}

Expand Down Expand Up @@ -1712,37 +1712,12 @@ function buildRootMap(roots: any[], nodes: any[]): Map<any, any[]> {
return rootMap;
}

const CLASSES_CACHE_KEY = '$$classes';
function containsClass(element: any, className: string): boolean {
if (element.classList) {
return element.classList.contains(className);
} else {
const classes = element[CLASSES_CACHE_KEY];
return classes && classes[className];
}
}

function addClass(element: any, className: string) {
if (element.classList) {
element.classList.add(className);
} else {
let classes: {[className: string]: boolean} = element[CLASSES_CACHE_KEY];
if (!classes) {
classes = element[CLASSES_CACHE_KEY] = {};
}
classes[className] = true;
}
element.classList?.add(className);
}

function removeClass(element: any, className: string) {
if (element.classList) {
element.classList.remove(className);
} else {
let classes: {[className: string]: boolean} = element[CLASSES_CACHE_KEY];
if (classes) {
delete classes[className];
}
}
element.classList?.remove(className);
}

function removeNodesAfterAnimationDone(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {AnimationPlayer, ɵStyleData} from '@angular/animations';
import {allowPreviousPlayerStylesMerge, balancePreviousStylesIntoKeyframes, copyStyles} from '../../util';
import {AnimationDriver} from '../animation_driver';
import {CssKeyframesDriver} from '../css_keyframes/css_keyframes_driver';
import {containsElement, invokeQuery, isBrowser, matchesElement, validateStyleProperty} from '../shared';
import {containsElement, invokeQuery, isBrowser, validateStyleProperty} from '../shared';
import {packageNonAnimatableStyles} from '../special_cased_styles';

import {WebAnimationsPlayer} from './web_animations_player';
Expand All @@ -23,8 +23,9 @@ export class WebAnimationsDriver implements AnimationDriver {
return validateStyleProperty(prop);
}

matchesElement(element: any, selector: string): boolean {
return matchesElement(element, selector);
matchesElement(_element: any, _selector: string): boolean {
// This method is deprecated and no longer in use so we return false.
return false;
}

containsElement(elm1: any, elm2: any): boolean {
Expand Down
2 changes: 1 addition & 1 deletion packages/animations/browser/test/dsl/animation_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ function createDiv() {

{
describe('Animation', () => {
// these tests are only mean't to be run within the DOM (for now)
// these tests are only meant to be run within the DOM (for now)
if (isNode) return;

let rootElement: any;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {makeTrigger} from '../shared';

{
describe('AnimationTrigger', () => {
// these tests are only mean't to be run within the DOM (for now)
// these tests are only meant to be run within the DOM (for now)
if (isNode) return;

let element: any;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ function makeEngine(body: any, driver?: AnimationDriver, normalizer?: AnimationS
body, driver || defaultDriver, normalizer || new NoopAnimationStyleNormalizer());
}

// these tests are only mean't to be run within the DOM
// these tests are only meant to be run within the DOM
if (isNode) return;

describe('TimelineAnimationEngine', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/
import {AnimationPlayer, AUTO_STYLE, NoopAnimationPlayer, ɵStyleData} from '@angular/animations';
import {AnimationDriver, ɵallowPreviousPlayerStylesMerge as allowPreviousPlayerStylesMerge, ɵcontainsElement as containsElement, ɵinvokeQuery as invokeQuery, ɵmatchesElement as matchesElement, ɵvalidateStyleProperty as validateStyleProperty} from '@angular/animations/browser';
import {AnimationDriver, ɵallowPreviousPlayerStylesMerge as allowPreviousPlayerStylesMerge, ɵcontainsElement as containsElement, ɵinvokeQuery as invokeQuery, ɵvalidateStyleProperty as validateStyleProperty} from '@angular/animations/browser';


/**
Expand All @@ -19,8 +19,8 @@ export class MockAnimationDriver implements AnimationDriver {
return validateStyleProperty(prop);
}

matchesElement(element: any, selector: string): boolean {
return matchesElement(element, selector);
matchesElement(_element: any, _selector: string): boolean {
return false;
}

containsElement(elm1: any, elm2: any): boolean {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
*/
import {animate, animateChild, AnimationPlayer, AUTO_STYLE, group, query, sequence, stagger, state, style, transition, trigger, ɵAnimationGroupPlayer as AnimationGroupPlayer} from '@angular/animations';
import {AnimationDriver, ɵAnimationEngine} from '@angular/animations/browser';
import {matchesElement} from '@angular/animations/browser/src/render/shared';
import {TransitionAnimationPlayer} from '@angular/animations/browser/src/render/transition_animation_engine';
import {ENTER_CLASSNAME, LEAVE_CLASSNAME} from '@angular/animations/browser/src/util';
import {MockAnimationDriver, MockAnimationPlayer} from '@angular/animations/browser/testing';
Expand All @@ -19,7 +18,7 @@ import {BrowserAnimationsModule} from '@angular/platform-browser/animations';
import {HostListener} from '../../src/metadata/directives';

(function() {
// these tests are only mean't to be run within the DOM (for now)
// these tests are only meant to be run within the DOM (for now)
if (isNode) return;

describe('animation query tests', function() {
Expand Down Expand Up @@ -3052,7 +3051,7 @@ describe('animation query tests', function() {
AnimationGroupPlayer;
const childPlayer = groupPlayer.players.find(player => {
if (player instanceof MockAnimationPlayer) {
return matchesElement(player.element, '.child');
return player.element.classList.contains('child');
}
return false;
}) as MockAnimationPlayer;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {ActivatedRoute, Router, RouterOutlet} from '@angular/router';
import {RouterTestingModule} from '@angular/router/testing';

(function() {
// these tests are only mean't to be run within the DOM (for now)
// these tests are only meant to be run within the DOM (for now)
if (isNode) return;

describe('Animation Router Tests', function() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {browserDetection} from '@angular/platform-browser/testing/src/browser_ut
import {TestBed} from '../../testing';

(function() {
// these tests are only mean't to be run within the DOM (for now)
// these tests are only meant to be run within the DOM (for now)
// Buggy in Chromium 39, see https://github.com/angular/angular/issues/15793
if (isNode) return;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {BrowserAnimationsModule} from '@angular/platform-browser/animations';
import {browserDetection} from '@angular/platform-browser/testing/src/browser_util';

(function() {
// these tests are only mean't to be run within the DOM (for now)
// these tests are only meant to be run within the DOM (for now)
// Buggy in Chromium 39, see https://github.com/angular/angular/issues/15793
if (isNode || !ɵsupportsWebAnimations()) return;

Expand Down

0 comments on commit 6df314f

Please sign in to comment.