From 1877e6c3f888b0843c45e1340e1e4d589ee4034c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Tue, 12 Mar 2019 14:14:08 -0700 Subject: [PATCH] fix(ivy): ensure template styles/classes are applied before directives are instantiated (#29269) Angular Ivy interprets inline static style/class attribute values as instructions that are processed whilst an element gets created. Because these inline style values are referenced by style/class bindings, their inline style values are applied at a later stage. Despite them being eventually applied, their values should be applied earlier before any directives are instantiated so that directive code can rely on any inline style/class changes. This patch ensures that all static style/class attribute values are applied (rendered) on the element before directives are instantiated. Jira Issue: FW-1133 PR Close #29269 --- packages/core/src/render3/instructions.ts | 19 ++++- .../core/src/render3/interfaces/styling.ts | 6 ++ .../styling/class_and_style_bindings.ts | 84 ++++++++++--------- packages/core/test/acceptance/styling_spec.ts | 59 ++++++++++++- .../cyclic_import/bundle.golden_symbols.json | 3 - .../hello_world/bundle.golden_symbols.json | 3 - .../bundling/todo/bundle.golden_symbols.json | 3 - .../angular_material_test_blocklist.js | 10 +-- 8 files changed, 125 insertions(+), 62 deletions(-) diff --git a/packages/core/src/render3/instructions.ts b/packages/core/src/render3/instructions.ts index 5516ab0b7b..fb638ffb58 100644 --- a/packages/core/src/render3/instructions.ts +++ b/packages/core/src/render3/instructions.ts @@ -619,10 +619,13 @@ export function elementStart( ngDevMode && ngDevMode.rendererCreateElement++; const native = elementCreate(name); + const renderer = lView[RENDERER]; ngDevMode && assertDataInRange(lView, index - 1); const tNode = createNodeAtIndex(index, TNodeType.Element, native !, name, attrs || null); + let initialStylesIndex = 0; + let initialClassesIndex = 0; if (attrs) { const lastAttrIndex = setUpAttributes(native, attrs); @@ -640,6 +643,14 @@ export function elementStart( tNode.stylingTemplate = initializeStaticStylingContext(attrs, stylingAttrsStartIndex); } } + + if (tNode.stylingTemplate) { + // the initial style/class values are rendered immediately after having been + // initialized into the context so the element styling is ready when directives + // are initialized (since they may read style/class values in their constructor) + initialStylesIndex = renderInitialStyles(native, tNode.stylingTemplate, renderer); + initialClassesIndex = renderInitialClasses(native, tNode.stylingTemplate, renderer); + } } appendChild(native, tNode, lView); @@ -667,11 +678,11 @@ export function elementStart( } } - // There is no point in rendering styles when a class directive is present since - // it will take that over for us (this will be removed once #FW-882 is in). + // we render the styling again below in case any directives have set any `style` and/or + // `class` host attribute values... if (tNode.stylingTemplate) { - renderInitialClasses(native, tNode.stylingTemplate, lView[RENDERER]); - renderInitialStyles(native, tNode.stylingTemplate, lView[RENDERER]); + renderInitialClasses(native, tNode.stylingTemplate, renderer, initialClassesIndex); + renderInitialStyles(native, tNode.stylingTemplate, renderer, initialStylesIndex); } const currentQueries = lView[QUERIES]; diff --git a/packages/core/src/render3/interfaces/styling.ts b/packages/core/src/render3/interfaces/styling.ts index 90ca68100f..93589c23da 100644 --- a/packages/core/src/render3/interfaces/styling.ts +++ b/packages/core/src/render3/interfaces/styling.ts @@ -470,6 +470,12 @@ export const enum InitialStylingValuesIndex { */ DirectiveOwnerOffset = 2, + /** + * The first bit set aside to mark if the initial style was already rendere + */ + AppliedFlagBitPosition = 0b0, + AppliedFlagBitLength = 1, + /** * The total size for each style/class entry (prop + value + directiveOwner) */ diff --git a/packages/core/src/render3/styling/class_and_style_bindings.ts b/packages/core/src/render3/styling/class_and_style_bindings.ts index 1fec18b5d5..f4b933f583 100644 --- a/packages/core/src/render3/styling/class_and_style_bindings.ts +++ b/packages/core/src/render3/styling/class_and_style_bindings.ts @@ -139,47 +139,53 @@ function patchInitialStylingValue( } /** - * Runs through the initial style data present in the context and renders - * them via the renderer on the element. - */ -export function renderInitialStyles( - element: RElement, context: StylingContext, renderer: Renderer3) { - const initialStyles = context[StylingIndex.InitialStyleValuesPosition]; - renderInitialStylingValues(element, renderer, initialStyles, false); -} - -/** - * Runs through the initial class data present in the context and renders - * them via the renderer on the element. + * Runs through the initial class values present in the provided + * context and renders them via the provided renderer on the element. + * + * @param element the element the styling will be applied to + * @param context the source styling context which contains the initial class values + * @param renderer the renderer instance that will be used to apply the class + * @returns the index that the classes were applied up until */ export function renderInitialClasses( - element: RElement, context: StylingContext, renderer: Renderer3) { + element: RElement, context: StylingContext, renderer: Renderer3, startIndex?: number): number { const initialClasses = context[StylingIndex.InitialClassValuesPosition]; - renderInitialStylingValues(element, renderer, initialClasses, true); + let i = startIndex || InitialStylingValuesIndex.KeyValueStartPosition; + while (i < initialClasses.length) { + const value = initialClasses[i + InitialStylingValuesIndex.ValueOffset]; + if (value) { + setClass( + element, initialClasses[i + InitialStylingValuesIndex.PropOffset] as string, true, + renderer, null); + } + i += InitialStylingValuesIndex.Size; + } + return i; } /** - * This is a helper function designed to render each entry present within the - * provided list of initialStylingValues. + * Runs through the initial styles values present in the provided + * context and renders them via the provided renderer on the element. + * + * @param element the element the styling will be applied to + * @param context the source styling context which contains the initial class values + * @param renderer the renderer instance that will be used to apply the class + * @returns the index that the styles were applied up until */ -function renderInitialStylingValues( - element: RElement, renderer: Renderer3, initialStylingValues: InitialStylingValues, - isEntryClassBased: boolean) { - for (let i = InitialStylingValuesIndex.KeyValueStartPosition; i < initialStylingValues.length; - i += InitialStylingValuesIndex.Size) { - const value = initialStylingValues[i + InitialStylingValuesIndex.ValueOffset]; +export function renderInitialStyles( + element: RElement, context: StylingContext, renderer: Renderer3, startIndex?: number) { + const initialStyles = context[StylingIndex.InitialStyleValuesPosition]; + let i = startIndex || InitialStylingValuesIndex.KeyValueStartPosition; + while (i < initialStyles.length) { + const value = initialStyles[i + InitialStylingValuesIndex.ValueOffset]; if (value) { - if (isEntryClassBased) { - setClass( - element, initialStylingValues[i + InitialStylingValuesIndex.PropOffset] as string, true, - renderer, null); - } else { - setStyle( - element, initialStylingValues[i + InitialStylingValuesIndex.PropOffset] as string, - value as string, renderer, null); - } + setStyle( + element, initialStyles[i + InitialStylingValuesIndex.PropOffset] as string, + value as string, renderer, null); } + i += InitialStylingValuesIndex.Size; } + return i; } export function allowNewBindingsForStylingContext(context: StylingContext): boolean { @@ -375,7 +381,7 @@ export function updateContextWithBindings( } // if a property is not found in the initial style values list then it - // is ALWAYS added incase a follow-up directive introduces the same initial + // is ALWAYS added in case a follow-up directive introduces the same initial // style/class value later on. let initialValuesToLookup = entryIsClassBased ? initialClasses : initialStyles; let indexForInitial = getInitialStylingValuesIndexOf(initialValuesToLookup, propName); @@ -765,7 +771,7 @@ function patchStylingMapIntoContext( const mapProp = props[i]; if (!mapProp) { - // this is an early exit incase a value was already encountered above in the + // this is an early exit in case a value was already encountered above in the // previous loop (which means that the property was applied or rejected) continue; } @@ -801,7 +807,7 @@ function patchStylingMapIntoContext( // SKIP IF INITIAL CHECK // If the former `value` is `null` then it means that an initial value // could be being rendered on screen. If that is the case then there is - // no point in updating the value incase it matches. In other words if the + // no point in updating the value in case it matches. In other words if the // new value is the exact same as the previously rendered value (which // happens to be the initial value) then do nothing. if (distantCtxValue !== null || @@ -872,7 +878,7 @@ function patchStylingMapIntoContext( // reapply their values into the object. For this to happen, the cached array needs to be updated // with dirty flags so that follow-up calls to `updateStylingMap` will reapply their styling code. // the reapplication of styling code within the context will reshape it and update the offset - // values (also follow-up directives can write new values incase earlier directives set anything + // values (also follow-up directives can write new values in case earlier directives set anything // to null due to removals or falsy values). valuesEntryShapeChange = valuesEntryShapeChange || existingCachedValueCount !== totalUniqueValues; updateCachedMapValue( @@ -1074,7 +1080,7 @@ export function renderStyling( // VALUE DEFER CASE 2: Use the initial value if all else fails (is falsy) // the initial value will always be a string or null, - // therefore we can safely adopt it incase there's nothing else + // therefore we can safely adopt it in case there's nothing else // note that this should always be a falsy check since `false` is used // for both class and style comparisons (styles can't be false and false // classes are turned off and should therefore defer to their initial values) @@ -1742,7 +1748,7 @@ function allowValueChange( // directive is the parent component directive (the template) and each directive // that is added after is considered less important than the previous entry. This // prioritization of directives enables the styling algorithm to decide if a style - // or class should be allowed to be updated/replaced incase an earlier directive + // or class should be allowed to be updated/replaced in case an earlier directive // already wrote to the exact same style-property or className value. In other words // this decides what to do if and when there is a collision. if (currentValue != null) { @@ -1751,7 +1757,7 @@ function allowValueChange( // previous directive's value... return newDirectiveOwner <= currentDirectiveOwner; } else { - // only write a null value incase it's the same owner writing it. + // only write a null value in case it's the same owner writing it. // this avoids having a higher-priority directive write to null // only to have a lesser-priority directive change right to a // non-null value immediately afterwards. @@ -1955,7 +1961,7 @@ function registerMultiMapEntry( while (cachedValues.length < limit) { // this means that ONLY directive class styling (like ngClass) was used // therefore the root directive will still need to be filled in as well - // as any other directive spaces incase they only used static values + // as any other directive spaces in case they only used static values cachedValues.push(0, startPosition, null, 0); } } diff --git a/packages/core/test/acceptance/styling_spec.ts b/packages/core/test/acceptance/styling_spec.ts index 4eb477fef4..692164dc5b 100644 --- a/packages/core/test/acceptance/styling_spec.ts +++ b/packages/core/test/acceptance/styling_spec.ts @@ -5,7 +5,7 @@ * 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, HostBinding} from '@angular/core'; +import {Component, Directive, ElementRef, HostBinding} from '@angular/core'; import {TestBed} from '@angular/core/testing'; import {expect} from '@angular/platform-browser/testing/src/matchers'; import {ivyEnabled, onlyInIvy} from '@angular/private/testing'; @@ -96,4 +96,61 @@ describe('acceptance integration tests', () => { expect(element.classList.contains('foo')).toBeTruthy(); expect(element.classList.contains('baz')).toBeTruthy(); }); + + it('should render inline style and class attribute values on the element before a directive is instantiated', + () => { + @Component({ + template: ` +
+ ` + }) + class Cmp { + } + + @Directive({selector: '[directive-expecting-styling]'}) + class DirectiveExpectingStyling { + constructor(elm: ElementRef) { + const native = elm.nativeElement; + native.setAttribute('data-captured-width', native.style.width); + native.setAttribute('data-captured-classes', native.className); + } + } + + TestBed.configureTestingModule({declarations: [Cmp, DirectiveExpectingStyling]}); + const fixture = TestBed.createComponent(Cmp); + fixture.detectChanges(); + + const element = fixture.nativeElement.querySelector('div'); + expect(element.style.width).toEqual('200px'); + expect(element.getAttribute('data-captured-width')).toEqual('200px'); + expect(element.className.trim()).toEqual('abc xyz'); + expect(element.getAttribute('data-captured-classes')).toEqual('abc xyz'); + }); + + it('should only render the same initial styling values once before a directive runs', () => { + @Component({ + template: ` +
+ ` + }) + class Cmp { + } + + @Directive({selector: '[directive-expecting-styling]'}) + class DirectiveExpectingStyling { + constructor(elm: ElementRef) { + const native = elm.nativeElement; + native.style.width = '300px'; + native.classList.remove('abc'); + } + } + + TestBed.configureTestingModule({declarations: [Cmp, DirectiveExpectingStyling]}); + const fixture = TestBed.createComponent(Cmp); + fixture.detectChanges(); + + const element = fixture.nativeElement.querySelector('div'); + expect(element.style.width).toEqual('300px'); + expect(element.classList.contains('abc')).toBeFalsy(); + }); }); diff --git a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json index 40a176494e..4bba4c6506 100644 --- a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json +++ b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json @@ -605,9 +605,6 @@ { "name": "renderInitialStyles" }, - { - "name": "renderInitialStylingValues" - }, { "name": "renderStringify" }, diff --git a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json index 3a8ba6079a..b660974683 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -425,9 +425,6 @@ { "name": "renderInitialStyles" }, - { - "name": "renderInitialStylingValues" - }, { "name": "renderStringify" }, diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index 53a801a8d9..3923b09ef4 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -1178,9 +1178,6 @@ { "name": "renderInitialStyles" }, - { - "name": "renderInitialStylingValues" - }, { "name": "renderStringify" }, diff --git a/tools/material-ci/angular_material_test_blocklist.js b/tools/material-ci/angular_material_test_blocklist.js index 5527a738ac..0d720a6173 100644 --- a/tools/material-ci/angular_material_test_blocklist.js +++ b/tools/material-ci/angular_material_test_blocklist.js @@ -104,14 +104,6 @@ window.testBlocklist = { "MatSnackBar with TemplateRef should be able to open a snack bar using a TemplateRef": { "error": "Error: Expected ' Fries Pizza ' to contain 'Pasta'.", "notes": "Breaking change: Change detection follows insertion tree only, not declaration tree (MatSnackBarContainer is OnPush)" - }, - "MatTooltip special cases should clear the `user-select` when a tooltip is set on a text field": { - "error": "Error: Expected 'none' to be falsy.", - "notes": "FW-1133: Inline styles are not applied before constructor is run" - }, - "MatTooltip special cases should clear the `-webkit-user-drag` on draggable elements": { - "error": "Error: Expected 'none' to be falsy.", - "notes": "FW-1133: Inline styles are not applied before constructor is run" } }; -// clang-format on +// clang-format on \ No newline at end of file