Skip to content
This repository was archived by the owner on Jan 6, 2025. It is now read-only.

fix(api): defer getComputedStyle() calls until ngOnInit phase #374

Merged
merged 1 commit into from
Aug 14, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 34 additions & 61 deletions src/lib/flexbox/api/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -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();
}

// *********************************************
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

hasInitialized is only used in a ngDoCheck hook. According to this chart, it shouldn't be possible for ngDoCheck to fire before ngOnInit, making the flag redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean. Many of the directives using ngOnInit(), the hasInitialized() is a custom method used in the Flex directives.

Copy link
Member

Choose a reason for hiding this comment

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

I mean that the only place I see it being used is https://github.com/angular/flex-layout/pull/374/files#diff-5bc48b81f8309cb16bb22da0a329e466R140 which shouldn't be possible to fire before ngOnInit. I might be missing some context here, though.

Copy link
Contributor Author

@ThomasBurleson ThomasBurleson Aug 10, 2017

Choose a reason for hiding this comment

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

You are correct that ShowHideDirective::ngOnChanges() uses this.hasInitialized.

ngOnChanges() is called 1x before ngOnInit() and then later when the @input values are externally changed. The intention of that logic is to SKIP any responses to changes until after ngOnInit().

Copy link
Member

Choose a reason for hiding this comment

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

Alright, makes sense. I suppose it also depends on the component tree.

}

ngOnChanges(change: SimpleChanges) {
throw new Error(`BaseFxDirective::ngOnChanges should be overridden in subclass: ${change}`);
}
Expand Down Expand Up @@ -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');
}

/**
Expand All @@ -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 || []);
}

/**
Expand Down Expand Up @@ -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;

Expand All @@ -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;
}
12 changes: 7 additions & 5 deletions src/lib/flexbox/api/class.ts
Original file line number Diff line number Diff line change
Expand Up @@ -139,11 +139,13 @@ export class ClassDirective extends BaseFxDirective
* For @Input changes on the current mq activation property
*/
ngOnChanges(changes: SimpleChanges) {
if (this._classAdapter.activeKey in changes) {
this._updateKlass();
}
if (this._ngClassAdapter.activeKey in changes) {
this._updateNgClass();
if (this.hasInitialized) {
if (this._classAdapter.activeKey in changes) {
this._updateKlass();
}
if (this._ngClassAdapter.activeKey in changes) {
this._updateNgClass();
}
}
}

Expand Down
2 changes: 2 additions & 0 deletions src/lib/flexbox/api/flex-align.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Expand Down
2 changes: 2 additions & 0 deletions src/lib/flexbox/api/flex-offset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Expand Down
2 changes: 2 additions & 0 deletions src/lib/flexbox/api/flex-order.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Expand Down
2 changes: 2 additions & 0 deletions src/lib/flexbox/api/flex.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Expand Down
2 changes: 2 additions & 0 deletions src/lib/flexbox/api/layout-align.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Expand Down
2 changes: 2 additions & 0 deletions src/lib/flexbox/api/layout-wrap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Expand Down
2 changes: 2 additions & 0 deletions src/lib/flexbox/api/layout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Expand Down
6 changes: 3 additions & 3 deletions src/lib/flexbox/api/show-hide.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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();
}
}
Expand All @@ -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);
Expand Down
75 changes: 75 additions & 0 deletions src/lib/utils/style-utils.spec.ts
Original file line number Diff line number Diff line change
@@ -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 <div></div>', () => {
expectDOMFrom(`
<div></div>
`).toHaveStyle({'display': 'block'});
});

it('should find to "display" for inline style <div></div>', () => {
expectDOMFrom(`
<div style="display: flex;"></div>
`).toHaveStyle({'display': 'flex'});
});

it('should find `display` from html style element', () => {
expectDOMFrom(`
<style>
div.special { display: inline-block; }
</style>
<div class="special"></div>
`).toHaveStyle({'display': 'inline-block'});
});

it('should find `display` from component styles', () => {
let expectStyledDOM = makeExpectDOMFrom(() => TestLayoutComponent, [
'div.extra { display:table; }'
]);
expectStyledDOM(`
<div class="extra"></div>
`).toHaveStyle({'display': 'table'});
});

});


});


// *****************************************************************
// Template Component
// *****************************************************************

@Component({
selector: 'test-style-utils',
template: `<span>PlaceHolder Template HTML</span>`
})
export class TestLayoutComponent {
}
Loading