From 7cc4225eb93398c83aac60914c96c60341cc90f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Wed, 14 Aug 2019 14:16:09 -0700 Subject: [PATCH] fix(ivy): ensure binding ordering doesn't mess up when a `NO_CHANGE` value is encountered (#32143) Prior to this fix if a `NO_CHANGE` value was assigned to a binding, or an interpolation value rendererd a `NO_CHANGE` value, then the presence of that value would cause the internal counter index values to not increment properly. This patch ensures that this doesn't happen and that the counter/bitmask values update accordingly. PR Close #32143 --- integration/_payload-limits.json | 4 +- .../core/src/render3/styling_next/bindings.ts | 59 ++++++++++--------- .../src/render3/styling_next/instructions.ts | 54 ++++++++--------- .../core/src/render3/styling_next/util.ts | 9 ++- .../core/test/acceptance/styling_next_spec.ts | 40 ++++++++++++- 5 files changed, 103 insertions(+), 63 deletions(-) diff --git a/integration/_payload-limits.json b/integration/_payload-limits.json index c0e4539526..5d0ab0db8c 100644 --- a/integration/_payload-limits.json +++ b/integration/_payload-limits.json @@ -3,7 +3,7 @@ "master": { "uncompressed": { "runtime": 1497, - "main": 157490, + "main": 157021, "polyfills": 45399 } } @@ -39,7 +39,7 @@ "master": { "uncompressed": { "bundle": "TODO(i): temporarily increase the payload size limit from 105779 - this is due to a closure issue related to ESM reexports that still needs to be investigated", - "bundle": 177447 + "bundle": 177137 } } } diff --git a/packages/core/src/render3/styling_next/bindings.ts b/packages/core/src/render3/styling_next/bindings.ts index b92a842f57..1a2d78fc14 100644 --- a/packages/core/src/render3/styling_next/bindings.ts +++ b/packages/core/src/render3/styling_next/bindings.ts @@ -8,6 +8,7 @@ import {SafeValue} from '../../sanitization/bypass'; import {StyleSanitizeFn, StyleSanitizeMode} from '../../sanitization/style_sanitizer'; import {ProceduralRenderer3, RElement, Renderer3, RendererStyleFlags3, isProceduralRenderer} from '../interfaces/renderer'; +import {NO_CHANGE} from '../tokens'; import {ApplyStylingFn, LStylingData, StylingMapArray, StylingMapArrayIndex, StylingMapsSyncMode, SyncStylingMapsFn, TStylingContext, TStylingContextIndex, TStylingContextPropConfigFlags} from './interfaces'; import {BIT_MASK_START_VALUE, deleteStylingStateFromStorage, getStylingState, resetStylingState, storeStylingState} from './state'; @@ -80,21 +81,23 @@ let deferredBindingQueue: (TStylingContext | number | string | null | boolean)[] */ export function updateClassBinding( context: TStylingContext, data: LStylingData, element: RElement, prop: string | null, - bindingIndex: number, value: boolean | string | null | undefined | StylingMapArray, + bindingIndex: number, value: boolean | string | null | undefined | StylingMapArray | NO_CHANGE, deferRegistration: boolean, forceUpdate: boolean): boolean { const isMapBased = !prop; const state = getStylingState(element, stateIsPersisted(context)); const index = isMapBased ? STYLING_INDEX_FOR_MAP_BINDING : state.classesIndex++; - const updated = updateBindingData( - context, data, index, prop, bindingIndex, value, deferRegistration, forceUpdate, false); - if (updated || forceUpdate) { - // We flip the bit in the bitMask to reflect that the binding - // at the `index` slot has changed. This identifies to the flushing - // phase that the bindings for this particular CSS class need to be - // applied again because on or more of the bindings for the CSS - // class have changed. - state.classesBitMask |= 1 << index; - return true; + if (value !== NO_CHANGE) { + const updated = updateBindingData( + context, data, index, prop, bindingIndex, value, deferRegistration, forceUpdate, false); + if (updated || forceUpdate) { + // We flip the bit in the bitMask to reflect that the binding + // at the `index` slot has changed. This identifies to the flushing + // phase that the bindings for this particular CSS class need to be + // applied again because on or more of the bindings for the CSS + // class have changed. + state.classesBitMask |= 1 << index; + return true; + } } return false; } @@ -111,25 +114,27 @@ export function updateClassBinding( */ export function updateStyleBinding( context: TStylingContext, data: LStylingData, element: RElement, prop: string | null, - bindingIndex: number, value: string | number | SafeValue | null | undefined | StylingMapArray, + bindingIndex: number, + value: string | number | SafeValue | null | undefined | StylingMapArray | NO_CHANGE, sanitizer: StyleSanitizeFn | null, deferRegistration: boolean, forceUpdate: boolean): boolean { const isMapBased = !prop; const state = getStylingState(element, stateIsPersisted(context)); const index = isMapBased ? STYLING_INDEX_FOR_MAP_BINDING : state.stylesIndex++; - const sanitizationRequired = isMapBased ? - true : - (sanitizer ? sanitizer(prop !, null, StyleSanitizeMode.ValidateProperty) : false); - const updated = updateBindingData( - context, data, index, prop, bindingIndex, value, deferRegistration, forceUpdate, - sanitizationRequired); - if (updated || forceUpdate) { - // We flip the bit in the bitMask to reflect that the binding - // at the `index` slot has changed. This identifies to the flushing - // phase that the bindings for this particular property need to be - // applied again because on or more of the bindings for the CSS - // property have changed. - state.stylesBitMask |= 1 << index; - return true; + if (value !== NO_CHANGE) { + const sanitizationRequired = isMapBased || + (sanitizer ? sanitizer(prop !, null, StyleSanitizeMode.ValidateProperty) : false); + const updated = updateBindingData( + context, data, index, prop, bindingIndex, value, deferRegistration, forceUpdate, + sanitizationRequired); + if (updated || forceUpdate) { + // We flip the bit in the bitMask to reflect that the binding + // at the `index` slot has changed. This identifies to the flushing + // phase that the bindings for this particular property need to be + // applied again because on or more of the bindings for the CSS + // property have changed. + state.stylesBitMask |= 1 << index; + return true; + } } return false; } @@ -150,7 +155,7 @@ export function updateStyleBinding( function updateBindingData( context: TStylingContext, data: LStylingData, counterIndex: number, prop: string | null, bindingIndex: number, - value: string | SafeValue | number | boolean | null | undefined | StylingMapArray, + value: string | SafeValue | number | boolean | null | undefined | StylingMapArray | NO_CHANGE, deferRegistration: boolean, forceUpdate: boolean, sanitizationRequired: boolean): boolean { if (!isContextLocked(context)) { if (deferRegistration) { diff --git a/packages/core/src/render3/styling_next/instructions.ts b/packages/core/src/render3/styling_next/instructions.ts index 955ccd7d56..0ba7585067 100644 --- a/packages/core/src/render3/styling_next/instructions.ts +++ b/packages/core/src/render3/styling_next/instructions.ts @@ -168,21 +168,19 @@ function _stylingProp( const tNode = getTNode(elementIndex, lView); const native = getNativeByTNode(tNode, lView) as RElement; - let updated = false; - if (value !== NO_CHANGE) { - if (isClassBased) { - updated = updateClassBinding( - getClassesContext(tNode), lView, native, prop, bindingIndex, - value as string | boolean | null, defer, false); - } else { - const sanitizer = getCurrentStyleSanitizer(); - updated = updateStyleBinding( - getStylesContext(tNode), lView, native, prop, bindingIndex, - value as string | SafeValue | null, sanitizer, defer, false); - } + let valueHasChanged = false; + if (isClassBased) { + valueHasChanged = updateClassBinding( + getClassesContext(tNode), lView, native, prop, bindingIndex, + value as string | boolean | null, defer, false); + } else { + const sanitizer = getCurrentStyleSanitizer(); + valueHasChanged = updateStyleBinding( + getStylesContext(tNode), lView, native, prop, bindingIndex, + value as string | SafeValue | null, sanitizer, defer, false); } - return updated; + return valueHasChanged; } /** @@ -299,22 +297,20 @@ function _stylingMap( activateStylingMapFeature(); const lView = getLView(); - let valueHasChanged = false; - if (value !== NO_CHANGE) { - const tNode = getTNode(elementIndex, lView); - const native = getNativeByTNode(tNode, lView) as RElement; - const oldValue = lView[bindingIndex]; - valueHasChanged = hasValueChanged(oldValue, value); - const stylingMapArr = normalizeIntoStylingMap(oldValue, value, !isClassBased); - if (isClassBased) { - updateClassBinding( - context, lView, native, null, bindingIndex, stylingMapArr, defer, valueHasChanged); - } else { - const sanitizer = getCurrentStyleSanitizer(); - updateStyleBinding( - context, lView, native, null, bindingIndex, stylingMapArr, sanitizer, defer, - valueHasChanged); - } + const tNode = getTNode(elementIndex, lView); + const native = getNativeByTNode(tNode, lView) as RElement; + const oldValue = lView[bindingIndex]; + const valueHasChanged = hasValueChanged(oldValue, value); + const stylingMapArr = + value === NO_CHANGE ? NO_CHANGE : normalizeIntoStylingMap(oldValue, value, !isClassBased); + if (isClassBased) { + updateClassBinding( + context, lView, native, null, bindingIndex, stylingMapArr, defer, valueHasChanged); + } else { + const sanitizer = getCurrentStyleSanitizer(); + updateStyleBinding( + context, lView, native, null, bindingIndex, stylingMapArr, sanitizer, defer, + valueHasChanged); } return valueHasChanged; diff --git a/packages/core/src/render3/styling_next/util.ts b/packages/core/src/render3/styling_next/util.ts index 2bc267661f..5410c8840d 100644 --- a/packages/core/src/render3/styling_next/util.ts +++ b/packages/core/src/render3/styling_next/util.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ import {TNode, TNodeFlags} from '../interfaces/node'; - +import {NO_CHANGE} from '../tokens'; import {StylingMapArray, StylingMapArrayIndex, TStylingConfigFlags, TStylingContext, TStylingContextIndex, TStylingContextPropConfigFlags} from './interfaces'; const MAP_BASED_ENTRY_PROP_NAME = '--MAP--'; @@ -146,8 +146,11 @@ export function isMapBased(prop: string) { } export function hasValueChanged( - a: StylingMapArray | number | String | string | null | boolean | undefined | {}, - b: StylingMapArray | number | String | string | null | boolean | undefined | {}): boolean { + a: NO_CHANGE | StylingMapArray | number | String | string | null | boolean | undefined | {}, + b: NO_CHANGE | StylingMapArray | number | String | string | null | boolean | undefined | + {}): boolean { + if (b === NO_CHANGE) return false; + let compareValueA = Array.isArray(a) ? a[StylingMapArrayIndex.RawValuePosition] : a; let compareValueB = Array.isArray(b) ? b[StylingMapArrayIndex.RawValuePosition] : b; diff --git a/packages/core/test/acceptance/styling_next_spec.ts b/packages/core/test/acceptance/styling_next_spec.ts index 5125a8e984..f9b3e6276a 100644 --- a/packages/core/test/acceptance/styling_next_spec.ts +++ b/packages/core/test/acceptance/styling_next_spec.ts @@ -6,9 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ import {Component, Directive, HostBinding, Input, ViewChild} from '@angular/core'; -import {SecurityContext} from '@angular/core/src/core'; import {DebugNode, LViewDebug, toDebug} from '@angular/core/src/render3/instructions/lview_debug'; -import {getCheckNoChangesMode} from '@angular/core/src/render3/state'; import {loadLContextFromNode} from '@angular/core/src/render3/util/discovery_utils'; import {ngDevModeResetPerfCounters as resetStylingCounters} from '@angular/core/src/util/ng_dev_mode'; import {TestBed} from '@angular/core/testing'; @@ -1056,6 +1054,44 @@ describe('new styling integration', () => { expect(div.style.width).toEqual('100px'); expect(div.style.height).toEqual('300px'); }); + + it('should work with NO_CHANGE values if they are applied to bindings ', () => { + @Component({ + template: ` +
+ ` + }) + class Cmp { + w: any = null; + h: any = null; + o: any = null; + } + + TestBed.configureTestingModule({declarations: [Cmp]}); + const fixture = TestBed.createComponent(Cmp); + const comp = fixture.componentInstance; + + comp.w = '100px'; + comp.h = '200px'; + comp.o = '0.5'; + fixture.detectChanges(); + + const div = fixture.nativeElement.querySelector('div'); + expect(div.style.width).toEqual('100px'); + expect(div.style.height).toEqual('200px'); + expect(div.style.opacity).toEqual('0.5'); + + comp.w = '500px'; + comp.o = '1'; + fixture.detectChanges(); + + expect(div.style.width).toEqual('500px'); + expect(div.style.height).toEqual('200px'); + expect(div.style.opacity).toEqual('1'); + }); }); function assertStyleCounters(countForSet: number, countForRemove: number) {