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.

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

fix(animations): process shorthand margin and padding styles correctly #35701

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions aio/scripts/_payload-limits.json
Expand Up @@ -12,7 +12,7 @@
"master": {
"uncompressed": {
"runtime-es2015": 2987,
"main-es2015": 451469,
"main-es2015": 451552,
"polyfills-es2015": 52195
}
}
Expand All @@ -21,7 +21,7 @@
"master": {
"uncompressed": {
"runtime-es2015": 3097,
"main-es2015": 429230,
"main-es2015": 429313,
"polyfills-es2015": 52195
}
}
Expand Down
Expand Up @@ -7,9 +7,9 @@
*/
import {AnimationPlayer, ɵStyleData} from '@angular/animations';

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

import {CssKeyframesPlayer} from './css_keyframes_player';
Expand All @@ -36,7 +36,7 @@ export class CssKeyframesDriver implements AnimationDriver {
}

computeStyle(element: any, prop: string, defaultValue?: string): string {
return (window.getComputedStyle(element) as any)[prop] as string;
return computeStyle(element, prop);
}

buildKeyframeElement(element: any, name: string, keyframes: {[key: string]: any}[]): any {
Expand Down
Expand Up @@ -7,7 +7,7 @@
*/
import {AnimationPlayer} from '@angular/animations';

import {computeStyle} from '../../util';
import {computeStyle} from '../shared';
import {SpecialCasedStyles} from '../special_cased_styles';
import {ElementAnimationStyleHandler} from './element_animation_style_handler';

Expand Down
34 changes: 34 additions & 0 deletions packages/animations/browser/src/render/shared.ts
Expand Up @@ -237,3 +237,37 @@ export function hypenatePropsObject(object: {[key: string]: any}): {[key: string
});
return newObj;
}


/**
* Returns the computed style for the provided property on the provided element.
*
* This function uses `window.getComputedStyle` internally to determine the
* style value for the element. Firefox doesn't support reading the shorthand
* forms of margin/padding and for this reason this function needs to account
* for that.
Comment on lines +246 to +248
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for documenting this 👍

*/
export function computeStyle(element: HTMLElement, prop: string): string {
const gcs = window.getComputedStyle(element);

// this is casted to any because the `CSSStyleDeclaration` type is a fixed
// set of properties and `prop` is a dynamic reference to a property within
// the `CSSStyleDeclaration` list.
let value = gcs[prop as any];

// Firefox returns empty string values for `margin` and `padding` properties
// when extracted using getComputedStyle (see similar issue here:
// https://github.com/jquery/jquery/issues/3383). In this situation
// we want to emulate the value that is returned by creating the top,
// right, bottom and left properties as individual style lookups.
if (value.length === 0 && (prop === 'margin' || prop === 'padding')) {
// reconstruct the padding/margin value as `top right bottom left`
const propTop = (prop + 'Top') as 'marginTop' | 'paddingTop';
const propRight = (prop + 'Right') as 'marginRight' | 'paddingRight';
const propBottom = (prop + 'Bottom') as 'marginBottom' | 'paddingBottom';
const propLeft = (prop + 'Left') as 'marginLeft' | 'paddingLeft';
value = `${gcs[propTop]} ${gcs[propRight]} ${gcs[propBottom]} ${gcs[propLeft]}`;
}

return value;
}
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 {computeStyle, containsElement, invokeQuery, isBrowser, matchesElement, validateStyleProperty} from '../shared';
import {packageNonAnimatableStyles} from '../special_cased_styles';

import {WebAnimationsPlayer} from './web_animations_player';
Expand All @@ -32,7 +32,7 @@ export class WebAnimationsDriver implements AnimationDriver {
}

computeStyle(element: any, prop: string, defaultValue?: string): string {
return (window.getComputedStyle(element) as any)[prop] as string;
return computeStyle(element, prop);
}

overrideWebAnimationsSupport(supported: boolean) { this._isNativeImpl = supported; }
Expand Down
Expand Up @@ -7,7 +7,7 @@
*/
import {AnimationPlayer} from '@angular/animations';

import {computeStyle} from '../../util';
import {computeStyle} from '../shared';
import {SpecialCasedStyles} from '../special_cased_styles';

import {DOMAnimation} from './dom_animation';
Expand Down
7 changes: 2 additions & 5 deletions packages/animations/browser/src/util.ts
Expand Up @@ -6,9 +6,10 @@
* found in the LICENSE file at https://angular.io/license
*/
import {AnimateTimings, AnimationMetadata, AnimationMetadataType, AnimationOptions, sequence, ɵStyleData} from '@angular/animations';

import {Ast as AnimationAst, AstVisitor as AnimationAstVisitor} from './dsl/animation_ast';
import {AnimationDslVisitor} from './dsl/animation_dsl_visitor';
import {isNode} from './render/shared';
import {computeStyle, isNode} from './render/shared';

export const ONE_SECOND = 1000;

Expand Down Expand Up @@ -340,7 +341,3 @@ export function visitDslNode(visitor: any, node: any, context: any): any {
throw new Error(`Unable to resolve animation metadata node #${node.type}`);
}
}

export function computeStyle(element: any, prop: string): string {
return (<any>window.getComputedStyle(element))[prop];
}
Expand Up @@ -6,8 +6,6 @@
* found in the LICENSE file at https://angular.io/license
*/
import {ElementAnimationStyleHandler} from '../../../src/render/css_keyframes/element_animation_style_handler';
import {computeStyle} from '../../../src/util';

import {assertStyle, createElement, makeAnimationEvent, supportsAnimationEventCreation} from './shared';

const EMPTY_FN = () => {};
Expand Down
55 changes: 55 additions & 0 deletions packages/animations/browser/test/render/shared_spec.ts
@@ -0,0 +1,55 @@
/**
* @license
* Copyright Google Inc. 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
*/
import {computeStyle} from '../../src/render/shared';

describe('shared animations code', () => {
if (isNode) return;

describe('computeStyle', () => {
it('should compute the margin style into the form top,right,bottom,left', () => {
const div = buildActualElement();

div.style.setProperty('margin', '1px 2px 3px 4px');
expect(computeStyle(div, 'margin')).toEqual('1px 2px 3px 4px');

div.style.setProperty('margin', '0px');
div.style.setProperty('margin-top', '10px');
div.style.setProperty('margin-right', '20px');
div.style.setProperty('margin-bottom', '30px');
div.style.setProperty('margin-left', '40px');
expect(computeStyle(div, 'margin')).toEqual('10px 20px 30px 40px');
});

it('should compute the padding style into the form top,right,bottom,left', () => {
const div = buildActualElement();

div.style.setProperty('padding', '1px 2px 3px 4px');
expect(computeStyle(div, 'padding')).toEqual('1px 2px 3px 4px');

div.style.setProperty('padding', '0px');
div.style.setProperty('padding-top', '10px');
div.style.setProperty('padding-right', '20px');
div.style.setProperty('padding-bottom', '30px');
div.style.setProperty('padding-left', '40px');
expect(computeStyle(div, 'padding')).toEqual('10px 20px 30px 40px');
});
});
});

/**
* Returns a div element that's attached to the body.
*
* The reason why this function exists is because in order to
* compute style values on an element is must be attached within
* the body of a webpage.
*/
function buildActualElement() {
const div = document.createElement('div');
document.body.appendChild(div);
return div;
}