From 430069e08df186becca191d992698e7a1789fffb Mon Sep 17 00:00:00 2001 From: Thomas Burleson Date: Thu, 10 Aug 2017 14:23:07 -0500 Subject: [PATCH] fix(api): defer getComputedStyle() calls until ngOnInit phase MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, the Show/Hide directives would query the `display` style during directive instantiation. For elements instantiated during `*ngFor`, constructor calls to getComputedStyle() will incorrectly return “” due to failures in queries for styles in CSS stylesheets or inline styles. Deferring these types of queries to the ngOnInit() lifecycle event, enables valid response to queries for both inline styles and stylesheets. Fixes #310. --- src/lib/flexbox/api/base.ts | 95 +++++++++--------------- src/lib/flexbox/api/flex-align.ts | 2 + src/lib/flexbox/api/flex-offset.ts | 2 + src/lib/flexbox/api/flex-order.ts | 2 + src/lib/flexbox/api/flex.ts | 2 + src/lib/flexbox/api/layout-align.ts | 2 + src/lib/flexbox/api/layout-wrap.ts | 2 + src/lib/flexbox/api/layout.ts | 2 + src/lib/flexbox/api/show-hide.ts | 6 +- src/lib/utils/style-utils.spec.ts | 75 +++++++++++++++++++ src/lib/utils/style-utils.ts | 91 +++++++++++++++++++++++ src/lib/utils/testing/custom-matchers.ts | 49 ++++++++++-- src/lib/utils/testing/dom-tools.ts | 11 ++- src/lib/utils/testing/helpers.ts | 4 +- test/karma.conf.js | 1 + 15 files changed, 271 insertions(+), 75 deletions(-) create mode 100644 src/lib/utils/style-utils.spec.ts create mode 100644 src/lib/utils/style-utils.ts diff --git a/src/lib/flexbox/api/base.ts b/src/lib/flexbox/api/base.ts index 520d6bdc9..bc2916365 100644 --- a/src/lib/flexbox/api/base.ts +++ b/src/lib/flexbox/api/base.ts @@ -9,21 +9,20 @@ import { ElementRef, OnDestroy, SimpleChanges, OnChanges, SimpleChange, Renderer2 } from '@angular/core'; -import {ɵgetDOM as getDom} from '@angular/platform-browser'; -import {applyCssPrefixes} from '../../utils/auto-prefixer'; import {buildLayoutCSS} from '../../utils/layout-validator'; +import { + StyleDefinition, + lookupStyle, + lookupInlineStyle, + applyStyleToElement, + applyStyleToElements +} from '../../utils/style-utils'; import {ResponsiveActivation, KeyOptions} from '../responsive/responsive-activation'; import {MediaMonitor} from '../../media-query/media-monitor'; import {MediaQuerySubscriber} from '../../media-query/media-change'; -/** - * Definition of a css style. Either a property name (e.g. "flex-basis") or an object - * map of property name and value (e.g. {display: 'none', flex-order: 5}). - */ -export type StyleDefinition = string | { [property: string]: string | number }; - /** Abstract base class for the Layout API styling directives. */ export abstract class BaseFxDirective implements OnDestroy, OnChanges { @@ -66,7 +65,6 @@ export abstract class BaseFxDirective implements OnDestroy, OnChanges { constructor(protected _mediaMonitor: MediaMonitor, protected _elementRef: ElementRef, protected _renderer: Renderer2) { - this._display = this._getDisplayStyle(); } // ********************************************* @@ -92,6 +90,15 @@ export abstract class BaseFxDirective implements OnDestroy, OnChanges { // Lifecycle Methods // ********************************************* + /** + * Use post-component-initialization event to perform extra + * querying such as computed Display style + */ + ngOnInit() { + this._display = this._getDisplayStyle(); + this._hasInitialized = true; + } + ngOnChanges(change: SimpleChanges) { throw new Error(`BaseFxDirective::ngOnChanges should be overridden in subclass: ${change}`); } @@ -124,10 +131,7 @@ export abstract class BaseFxDirective implements OnDestroy, OnChanges { */ protected _getDisplayStyle(source?: HTMLElement): string { let element: HTMLElement = source || this._elementRef.nativeElement; - let value = this._lookupStyle(element, 'display'); - - // Note: 'inline' is the default of all elements, unless UA stylesheet overrides - return value ? value.trim() : 'inline'; + return lookupStyle(element, 'display'); } /** @@ -140,74 +144,32 @@ export abstract class BaseFxDirective implements OnDestroy, OnChanges { let value = 'row'; if (target) { - value = this._lookupStyle(target, 'flex-direction') || 'row'; + value = lookupStyle(target, 'flex-direction') || 'row'; + let hasInlineValue = lookupInlineStyle(target, 'flex-direction'); - let hasInlineValue = getDom().getStyle(target, 'flex-direction'); if (!hasInlineValue && addIfMissing) { - this._applyStyleToElements(buildLayoutCSS(value), [target]); + applyStyleToElements(this._renderer, buildLayoutCSS(value), [target]); } } return value.trim(); } - /** - * Determine the inline or inherited CSS style - */ - protected _lookupStyle(element: HTMLElement, styleName: string): any { - let value = ''; - try { - if (element) { - let immediateValue = getDom().getStyle(element, styleName); - value = immediateValue || getDom().getComputedStyle(element).getPropertyValue(styleName); - } - } catch (e) { - // TODO: platform-server throws an exception for getComputedStyle - } - return value; - } - - /** - * Applies the styles to the element. The styles object map may contain an array of values. Each - * value will be added as element style. - */ - protected _applyMultiValueStyleToElement(styles: {}, element: any) { - Object.keys(styles).forEach(key => { - const values = Array.isArray(styles[key]) ? styles[key] : [styles[key]]; - for (let value of values) { - this._renderer.setStyle(element, key, value); - } - }); - } - /** * Applies styles given via string pair or object map to the directive element. */ protected _applyStyleToElement(style: StyleDefinition, value?: string | number, nativeElement?: any) { - let styles = {}; let element = nativeElement || this._elementRef.nativeElement; - - if (typeof style === 'string') { - styles[style] = value; - style = styles; - } - - styles = applyCssPrefixes(style); - - this._applyMultiValueStyleToElement(styles, element); + applyStyleToElement(this._renderer, element, style, value); } /** - * Applies styles given via string pair or object map to the directive element. + * Applies styles given via string pair or object map to the directive's element. */ protected _applyStyleToElements(style: StyleDefinition, elements: HTMLElement[ ]) { - let styles = applyCssPrefixes(style); - - elements.forEach(el => { - this._applyMultiValueStyleToElement(styles, el); - }); + applyStyleToElements(this._renderer, style, elements || []); } /** @@ -264,6 +226,10 @@ export abstract class BaseFxDirective implements OnDestroy, OnChanges { return this._mqActivation.hasKeyValue(key); } + protected get hasInitialized() { + return this._hasInitialized; + } + /** Original dom Elements CSS display style */ protected _display; @@ -277,4 +243,11 @@ export abstract class BaseFxDirective implements OnDestroy, OnChanges { */ protected _inputMap = {}; + /** + * Has the `ngOnInit()` method fired + * + * Used to allow *ngFor tasks to finish and support queries like + * getComputedStyle() during ngOnInit(). + */ + protected _hasInitialized = false; } diff --git a/src/lib/flexbox/api/flex-align.ts b/src/lib/flexbox/api/flex-align.ts index ee69347df..181192642 100644 --- a/src/lib/flexbox/api/flex-align.ts +++ b/src/lib/flexbox/api/flex-align.ts @@ -77,6 +77,8 @@ export class FlexAlignDirective extends BaseFxDirective implements OnInit, OnCha * mql change events to onMediaQueryChange handlers */ ngOnInit() { + super.ngOnInit(); + this._listenForMediaQueryChanges('align', 'stretch', (changes: MediaChange) => { this._updateWithValue(changes.value); }); diff --git a/src/lib/flexbox/api/flex-offset.ts b/src/lib/flexbox/api/flex-offset.ts index 2a936b621..6b2b54076 100644 --- a/src/lib/flexbox/api/flex-offset.ts +++ b/src/lib/flexbox/api/flex-offset.ts @@ -95,6 +95,8 @@ export class FlexOffsetDirective extends BaseFxDirective implements OnInit, OnCh * mql change events to onMediaQueryChange handlers */ ngOnInit() { + super.ngOnInit(); + this._listenForMediaQueryChanges('offset', 0 , (changes: MediaChange) => { this._updateWithValue(changes.value); }); diff --git a/src/lib/flexbox/api/flex-order.ts b/src/lib/flexbox/api/flex-order.ts index 531c959c7..d2c567803 100644 --- a/src/lib/flexbox/api/flex-order.ts +++ b/src/lib/flexbox/api/flex-order.ts @@ -74,6 +74,8 @@ export class FlexOrderDirective extends BaseFxDirective implements OnInit, OnCha * mql change events to onMediaQueryChange handlers */ ngOnInit() { + super.ngOnInit(); + this._listenForMediaQueryChanges('order', '0', (changes: MediaChange) => { this._updateWithValue(changes.value); }); diff --git a/src/lib/flexbox/api/flex.ts b/src/lib/flexbox/api/flex.ts index 250636fbb..983bd54ec 100644 --- a/src/lib/flexbox/api/flex.ts +++ b/src/lib/flexbox/api/flex.ts @@ -118,6 +118,8 @@ export class FlexDirective extends BaseFxDirective implements OnInit, OnChanges, * mql change events to onMediaQueryChange handlers */ ngOnInit() { + super.ngOnInit(); + this._listenForMediaQueryChanges('flex', '', (changes: MediaChange) => { this._updateStyle(changes.value); }); diff --git a/src/lib/flexbox/api/layout-align.ts b/src/lib/flexbox/api/layout-align.ts index f58c133fc..665ed34b2 100644 --- a/src/lib/flexbox/api/layout-align.ts +++ b/src/lib/flexbox/api/layout-align.ts @@ -91,6 +91,8 @@ export class LayoutAlignDirective extends BaseFxDirective implements OnInit, OnC * mql change events to onMediaQueryChange handlers */ ngOnInit() { + super.ngOnInit(); + this._listenForMediaQueryChanges('align', 'start stretch', (changes: MediaChange) => { this._updateWithValue(changes.value); }); diff --git a/src/lib/flexbox/api/layout-wrap.ts b/src/lib/flexbox/api/layout-wrap.ts index bcb75073e..bcbc376ff 100644 --- a/src/lib/flexbox/api/layout-wrap.ts +++ b/src/lib/flexbox/api/layout-wrap.ts @@ -89,6 +89,8 @@ export class LayoutWrapDirective extends BaseFxDirective implements OnInit, OnCh * mql change events to onMediaQueryChange handlers */ ngOnInit() { + super.ngOnInit(); + this._listenForMediaQueryChanges('wrap', 'wrap', (changes: MediaChange) => { this._updateWithValue(changes.value); }); diff --git a/src/lib/flexbox/api/layout.ts b/src/lib/flexbox/api/layout.ts index 70117046c..0b4fbcc89 100644 --- a/src/lib/flexbox/api/layout.ts +++ b/src/lib/flexbox/api/layout.ts @@ -97,6 +97,8 @@ export class LayoutDirective extends BaseFxDirective implements OnInit, OnChange * mql change events to onMediaQueryChange handlers */ ngOnInit() { + super.ngOnInit(); + this._listenForMediaQueryChanges('layout', 'row', (changes: MediaChange) => { this._updateWithDirection(changes.value); }); diff --git a/src/lib/flexbox/api/show-hide.ts b/src/lib/flexbox/api/show-hide.ts index 1dbe95fe2..ec4d8372e 100644 --- a/src/lib/flexbox/api/show-hide.ts +++ b/src/lib/flexbox/api/show-hide.ts @@ -108,7 +108,6 @@ export class ShowHideDirective extends BaseFxDirective implements OnInit, OnChan super(monitor, elRef, renderer); - this._display = this._getDisplayStyle(); // re-invoke override to use `this._layout` if (_layout) { /** * The Layout can set the display:flex (and incorrectly affect the Hide/Show directives. @@ -138,7 +137,7 @@ export class ShowHideDirective extends BaseFxDirective implements OnInit, OnChan * Then conditionally override with the mq-activated Input's current value */ ngOnChanges(changes: SimpleChanges) { - if (changes['show'] != null || this._mqActivation) { + if (this.hasInitialized && (changes['show'] != null || this._mqActivation)) { this._updateWithValue(); } } @@ -148,8 +147,9 @@ export class ShowHideDirective extends BaseFxDirective implements OnInit, OnChan * mql change events to onMediaQueryChange handlers */ ngOnInit() { - let value = this._getDefaultVal('show', true); + super.ngOnInit(); + let value = this._getDefaultVal('show', true); // Build _mqActivation controller this._listenForMediaQueryChanges('show', value, (changes: MediaChange) => { this._updateWithValue(changes.value); diff --git a/src/lib/utils/style-utils.spec.ts b/src/lib/utils/style-utils.spec.ts new file mode 100644 index 000000000..0e29d632f --- /dev/null +++ b/src/lib/utils/style-utils.spec.ts @@ -0,0 +1,75 @@ +/** + * @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 {Component} from '@angular/core'; +import {CommonModule} from '@angular/common'; +import {TestBed} from '@angular/core/testing'; + +import {customMatchers} from './testing/custom-matchers'; +import {makeExpectDOMFrom} from './testing/helpers'; + +describe('style-utils directive', () => { + let expectDOMFrom = makeExpectDOMFrom(() => TestLayoutComponent); + + beforeEach(() => { + jasmine.addMatchers(customMatchers); + + // Configure testbed to prepare services + TestBed.configureTestingModule({ + imports: [CommonModule], + declarations: [TestLayoutComponent] + }); + }); + + describe('testing display styles', () => { + + it('should default to "display:block" for
', () => { + expectDOMFrom(` +
+ `).toHaveStyle({'display': 'block'}); + }); + + it('should find to "display" for inline style
', () => { + expectDOMFrom(` +
+ `).toHaveStyle({'display': 'flex'}); + }); + + it('should find `display` from html style element', () => { + expectDOMFrom(` + +
+ `).toHaveStyle({'display': 'inline-block'}); + }); + + it('should find `display` from component styles', () => { + let expectStyledDOM = makeExpectDOMFrom(() => TestLayoutComponent, [ + 'div.extra { display:table; }' + ]); + expectStyledDOM(` +
+ `).toHaveStyle({'display': 'table'}); + }); + + }); + + +}); + + +// ***************************************************************** +// Template Component +// ***************************************************************** + +@Component({ + selector: 'test-style-utils', + template: `PlaceHolder Template HTML` +}) +export class TestLayoutComponent { +} diff --git a/src/lib/utils/style-utils.ts b/src/lib/utils/style-utils.ts new file mode 100644 index 000000000..b5747eac2 --- /dev/null +++ b/src/lib/utils/style-utils.ts @@ -0,0 +1,91 @@ +/** + * @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 {Renderer2} from '@angular/core'; +import {ɵgetDOM as getDom} from '@angular/platform-browser'; +import {applyCssPrefixes} from './auto-prefixer'; + +/** + * Definition of a css style. Either a property name (e.g. "flex-basis") or an object + * map of property name and value (e.g. {display: 'none', flex-order: 5}). + */ +export type StyleDefinition = string | { [property: string]: string | number }; + + +/** + * Applies styles given via string pair or object map to the directive element. + */ +export function applyStyleToElement(renderer: Renderer2, + nativeElement: any, + style: StyleDefinition, + value?: string | number) { + let element = nativeElement || this._elementRef.nativeElement; + let styles = {}; + + if (typeof style === 'string') { + styles[style] = value; + style = styles; + } + + styles = applyCssPrefixes(style); + applyMultiValueStyleToElement(styles, element, renderer); +} + + +/** + * Applies styles given via string pair or object map to the directive's element. + */ +export function applyStyleToElements(renderer: Renderer2, + style: StyleDefinition, + elements: HTMLElement[ ]) { + let styles = applyCssPrefixes(style); + + elements.forEach(el => { + applyMultiValueStyleToElement(styles, el, renderer); + }); +} + +/** + * Applies the styles to the element. The styles object map may contain an array of values. + * Each value will be added as element style. + */ +export function applyMultiValueStyleToElement(styles: {}, element: any, renderer: Renderer2) { + Object.keys(styles).forEach(key => { + const values = Array.isArray(styles[key]) ? styles[key] : [styles[key]]; + for (let value of values) { + renderer.setStyle(element, key, value); + } + }); +} + +/** + * Find the DOM element's inline style value (if any) + */ +export function lookupInlineStyle(element: HTMLElement, styleName: string): string { + return getDom().getStyle(element, styleName); +} + +/** + * Determine the inline or inherited CSS style + */ +export function lookupStyle(element: HTMLElement, styleName: string, inlineOnly = false): any { + let value = ''; + if (element) { + try { + let immediateValue = value = lookupInlineStyle(element, styleName); + if ( !inlineOnly ) { + value = immediateValue || getDom().getComputedStyle(element).getPropertyValue(styleName); + } + } catch (e) { + // TODO: platform-server throws an exception for getComputedStyle + } + } + + // Note: 'inline' is the default of all elements, unless UA stylesheet overrides; + // in which case getComputedStyle() should determine a valid value. + return value ? value.trim() : 'block'; +} diff --git a/src/lib/utils/testing/custom-matchers.ts b/src/lib/utils/testing/custom-matchers.ts index 37d7685f2..b1501ade4 100644 --- a/src/lib/utils/testing/custom-matchers.ts +++ b/src/lib/utils/testing/custom-matchers.ts @@ -30,7 +30,7 @@ export interface NgMatchers extends jasmine.Matchers { /** * Compare key:value pairs as matching EXACTLY */ - toHaveMap(expected: {[k: string]: string}): boolean; + toHaveMap(expected: { [k: string]: string }): boolean; /** * Expect the element to have the given CSS class. @@ -42,19 +42,29 @@ export interface NgMatchers extends jasmine.Matchers { toHaveCssClass(expected: string): boolean; /** - * Expect the element to have the given CSS styles. + * Expect the element to have the given CSS styles injected INLINE * * ## Example * * {@example testing/ts/matchers.ts region='toHaveCssStyle'} */ - toHaveCssStyle(expected: {[k: string]: string}|string): boolean; + toHaveCssStyle(expected: { [k: string]: string } | string): boolean; + + /** + * Expect the element to have the given CSS inline OR computed styles. + * + * ## Example + * + * {@example testing/ts/matchers.ts region='toHaveCssStyle'} + */ + toHaveStyle(expected: { [k: string]: string } | string): boolean; /** * Invert the matchers. */ not: NgMatchers; } + /** * NOTE: These custom JASMINE Matchers are used only * in the Karma/Jasmine testing for the Layout Directives @@ -116,9 +126,9 @@ export const customMatchers: jasmine.CustomMatcherFactories = { } }, - toHaveMap : function() { + toHaveMap: function () { return { - compare: function (actual: {[k: string]: string}, map: {[k: string]: string}) { + compare: function (actual: { [k: string]: string }, map: { [k: string]: string }) { let allPassed: boolean; allPassed = Object.keys(map).length !== 0; Object.keys(map).forEach(key => { @@ -138,9 +148,35 @@ export const customMatchers: jasmine.CustomMatcherFactories = { }; }, + toHaveStyle: function () { + return { + compare: function (actual: any, styles: { [k: string]: string } | string) { + let found = { }, computed = getComputedStyle(actual); + let allPassed: boolean = Object.keys(styles).length !== 0; + Object.keys(styles).forEach(prop => { + allPassed = allPassed && _.hasStyle(actual, prop, styles[prop], false); + if ( !allPassed ) { + found[prop] = computed.getPropertyValue(prop); + } + }); + + return { + pass: allPassed, + get message() { + const expectedValueStr = typeof styles === 'string' ? styles : JSON.stringify(styles); + return ` + Expected ${JSON.stringify(found)} ${!allPassed ? ' ' : 'not '} to contain the + CSS ${typeof styles === 'string' ? 'property' : 'styles'} '${expectedValueStr}' + `; + } + }; + } + }; + }, + toHaveCssStyle: function () { return { - compare: function (actual: any, styles: {[k: string]: string}|string) { + compare: function (actual: any, styles: { [k: string]: string } | string) { let allPassed: boolean; if (typeof styles === 'string') { allPassed = _.hasStyle(actual, styles); @@ -164,7 +200,6 @@ export const customMatchers: jasmine.CustomMatcherFactories = { } }; } - }; /** diff --git a/src/lib/utils/testing/dom-tools.ts b/src/lib/utils/testing/dom-tools.ts index 28268fc7e..2f757e296 100644 --- a/src/lib/utils/testing/dom-tools.ts +++ b/src/lib/utils/testing/dom-tools.ts @@ -36,8 +36,14 @@ function getStyle(element: any, stylename: string): string { return element.style[stylename]; } -function hasStyle(element: any, styleName: string, styleValue: string = null): boolean { - const value = this.getStyle(element, styleName) || ''; +function hasStyle(element: any, + styleName: string, + styleValue: string = null, + inlineOnly = true): boolean { + let searchComputed = () => { + return !inlineOnly ? getComputedStyle(element).getPropertyValue(styleName) : ''; + }; + const value = getStyle(element, styleName) || searchComputed() || ''; return styleValue ? value == styleValue : value.length > 0; } @@ -89,6 +95,7 @@ function isShadowRoot(node: any): boolean { function isPresent(obj: any): boolean { return obj != null; } + function tagName(element: any): string { return element.tagName; } diff --git a/src/lib/utils/testing/helpers.ts b/src/lib/utils/testing/helpers.ts index 24bcaa288..09fa5d39b 100644 --- a/src/lib/utils/testing/helpers.ts +++ b/src/lib/utils/testing/helpers.ts @@ -19,7 +19,7 @@ export type ComponentClazzFn = () => Type; * NOTE: These Generators (aka Partial Functions) are used only in * the Karma/Jasmine testing. */ -export function makeExpectDOMFrom(getClass: ComponentClazzFn) { +export function makeExpectDOMFrom(getClass: ComponentClazzFn, styles?: any) { let createTestComponent; // Return actual `expectTemplate()` function @@ -28,7 +28,7 @@ export function makeExpectDOMFrom(getClass: ComponentClazzFn) { createTestComponent = makeCreateTestComponent(getClass); } - let fixture = createTestComponent(template); + let fixture = createTestComponent(template, styles); if (key) { let instance = fixture.componentInstance; instance[key] = value; diff --git a/test/karma.conf.js b/test/karma.conf.js index 0b50f10f7..b569902ab 100644 --- a/test/karma.conf.js +++ b/test/karma.conf.js @@ -36,6 +36,7 @@ module.exports = (config) => { // Includes all package tests and source files into karma. Those files will be watched. // This pattern also matches all all sourcemap files and TypeScript files for debugging. {pattern: 'dist/packages/**/*', included: false, watched: true}, + ], customLaunchers: customLaunchers,