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

ThomasBurleson · 2017-08-10T19:25:31Z

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.

ThomasBurleson · 2017-08-10T19:33:42Z

@crisbeto - would you mind reviewing this ?

crisbeto

Is it feasible to have a unit test for this?

crisbeto · 2017-08-10T19:37:17Z

src/lib/flexbox/api/base.ts

+   */
+  ngOnInit() {
+    this._display = this._getDisplayStyle();
+    this._hasInitialized = true;


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.

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

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.

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().

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

crisbeto · 2017-08-10T19:38:20Z

src/lib/flexbox/api/base.ts

@@ -127,7 +135,7 @@ export abstract class BaseFxDirective implements OnDestroy, OnChanges {
    let value = this._lookupStyle(element, 'display');

    // Note: 'inline' is the default of all elements, unless UA stylesheet overrides
-    return value ? value.trim() : 'inline';
+    return value ? value.trim() : ((element.nodeType === 1) ? 'block' : 'inline');


Not a huge fan of nested ternaries. Consider breaking it out into an if and a ternary? E.g.

if (value) { return value; } return element.nodeType === 1 ? 'block' : 'inline';

crisbeto · 2017-08-10T19:40:40Z

src/lib/flexbox/api/base.ts

@@ -127,7 +135,7 @@ export abstract class BaseFxDirective implements OnDestroy, OnChanges {
    let value = this._lookupStyle(element, 'display');

    // Note: 'inline' is the default of all elements, unless UA stylesheet overrides
-    return value ? value.trim() : 'inline';
+    return value ? value.trim() : ((element.nodeType === 1) ? 'block' : 'inline');


I'm not sure that checking that the nodeType === 1 is a valid way to verify that the element is block-level. 1 only means that it's an Element node which is true for almost everything except cases like comments or raw text nodes.

Hmmm. That is true. Any suggestions?

Perhaps, we should KISS with return value ? value.trim() : 'block' ?

That won't be true necessarily for all cases. One potential solution can be to have a list of the default block-level elements and verify it against the element.nodeName, assuming that everything else is inline.

That does not seem reasonable for this library.

_lookupStyle() checks in-line styles or calls getComputedStyle(). So the expected response should usually be defined.

'inline' is the default of all elements, unless UA stylesheet overrides; which case getComputedStyle() should determine a valid value.

I think return value ? value.trim() : 'inline' is a reasonable fallback when _lookupStyle() fails.

It will catch most cases, but not the ones that are set to block by the UA, which are:

p
h1, h2, h3, h4, h5, h6
ol, ul
pre
address
blockquote
dl
div
fieldset
form
hr
noscript
table

ThomasBurleson · 2017-08-10T21:28:21Z

@crisbeto - working on 1-2 unit tests.

ThomasBurleson · 2017-08-11T12:42:10Z

@crisbeto - unit-tests provided. Can you review last bits?

crisbeto

Mostly LGTM, a few more nits.

crisbeto · 2017-08-11T12:56:46Z

src/lib/utils/style-utils.ts

+ * found in the LICENSE file at https://angular.io/license
+ */
+import {Renderer2} from '@angular/core';
+import {ɵgetDOM as getDom} from '@angular/platform-browser';


Is this intended (ɵgetDOM)? It looks like something generated by the AoT compiler that shouldn't be consumed directly.

Yes, that is intended. Same as in Angular.

crisbeto · 2017-08-11T14:32:38Z

src/lib/utils/style-utils.ts

+export function lookupStyle(element: HTMLElement, styleName: string, inlineOnly = false): any {
+  let value = '';
+  if (element) {
+    try {


AFAIK try/catch has a slight performance overhead. This one could be worked around by checking whether typeof getComputedStyle !== 'undefined'.

crisbeto · 2017-08-11T14:32:52Z

src/lib/utils/style-utils.ts

+/**
+ * Determine the inline or inherited CSS style
+ */
+export function lookupStyle(element: HTMLElement, styleName: string, inlineOnly = false): any {


The return value for this one is a string.

crisbeto · 2017-08-11T14:34:06Z

src/lib/utils/testing/dom-tools.ts

+                  styleName: string,
+                  styleValue: string = null,
+                  inlineOnly = true): boolean {
+  let searchComputed = () => {


Since this is only used once, it seems redundant to define a local function.

crisbeto

LGTM

ThomasBurleson · 2017-08-11T15:56:49Z

@crisbeto - Thank you sir for your help and reviews.

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.

angular-automatic-lock-bot · 2019-09-05T23:02:02Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Aug 10, 2017

ThomasBurleson requested a review from crisbeto August 10, 2017 19:33

crisbeto reviewed Aug 10, 2017

View reviewed changes

ThomasBurleson force-pushed the thomas/issue-310 branch from 198f458 to a57fa3f Compare August 10, 2017 20:13

ThomasBurleson force-pushed the thomas/issue-310 branch 2 times, most recently from 2c549a3 to 430069e Compare August 11, 2017 12:41

crisbeto reviewed Aug 11, 2017

View reviewed changes

ThomasBurleson force-pushed the thomas/issue-310 branch from 430069e to fbafab5 Compare August 11, 2017 15:31

ThomasBurleson added pr: lgtm This PR has been approved by the reviewer pr: needs presubmit labels Aug 11, 2017

ThomasBurleson force-pushed the thomas/issue-310 branch from fbafab5 to 75a4112 Compare August 11, 2017 15:50

crisbeto approved these changes Aug 11, 2017

View reviewed changes

ThomasBurleson force-pushed the thomas/issue-310 branch from 75a4112 to da83bde Compare August 11, 2017 22:39

mmalerba merged commit 3611003 into master Aug 14, 2017

benbraou mentioned this pull request Aug 15, 2017

feat(srcset): add srcset directive to inject <source> elements to sup… #366

Closed

ThomasBurleson deleted the thomas/issue-310 branch September 13, 2017 22:13

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ThomasBurleson commented Aug 10, 2017 •

edited

Loading

ThomasBurleson commented Aug 10, 2017

crisbeto left a comment

crisbeto Aug 10, 2017

ThomasBurleson Aug 10, 2017

crisbeto Aug 10, 2017

ThomasBurleson Aug 10, 2017 •

edited

Loading

crisbeto Aug 10, 2017

crisbeto Aug 10, 2017 •

edited

Loading

crisbeto Aug 10, 2017

ThomasBurleson Aug 10, 2017

crisbeto Aug 10, 2017

ThomasBurleson Aug 10, 2017

ThomasBurleson Aug 10, 2017 •

edited

Loading

crisbeto Aug 10, 2017

ThomasBurleson commented Aug 10, 2017

ThomasBurleson commented Aug 11, 2017

crisbeto left a comment

crisbeto Aug 11, 2017 •

edited

Loading

ThomasBurleson Aug 11, 2017

crisbeto Aug 11, 2017

crisbeto Aug 11, 2017

ThomasBurleson Aug 11, 2017

crisbeto Aug 11, 2017

ThomasBurleson Aug 11, 2017

crisbeto left a comment

ThomasBurleson commented Aug 11, 2017

angular-automatic-lock-bot bot commented Sep 5, 2019

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

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

Conversation

ThomasBurleson commented Aug 10, 2017 • edited Loading

ThomasBurleson commented Aug 10, 2017

crisbeto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasBurleson Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasBurleson Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasBurleson commented Aug 10, 2017

ThomasBurleson commented Aug 11, 2017

crisbeto left a comment

Choose a reason for hiding this comment

crisbeto Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto left a comment

Choose a reason for hiding this comment

ThomasBurleson commented Aug 11, 2017

angular-automatic-lock-bot bot commented Sep 5, 2019

ThomasBurleson commented Aug 10, 2017 •

edited

Loading

ThomasBurleson Aug 10, 2017 •

edited

Loading

crisbeto Aug 10, 2017 •

edited

Loading

ThomasBurleson Aug 10, 2017 •

edited

Loading

crisbeto Aug 11, 2017 •

edited

Loading