Skip to content

Commit

Permalink
fix(ivy): ensure binding ordering doesn't mess up when a NO_CHANGE …
Browse files Browse the repository at this point in the history 
…value is encountered (angular#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 angular#32143
  • Loading branch information
@matsko @mhevery
matsko authored and mhevery committed Sep 4, 2019
1 parent f6e88cd commit 7cc4225
Show file tree
Hide file tree
Showing 5 changed files with 103 additions and 63 deletions.
4 changes: 2 additions & 2 deletions integration/_payload-limits.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"master": {
"uncompressed": {
"runtime": 1497,
"main": 157490,
"main": 157021,
"polyfills": 45399
}
}
Expand Down Expand Up @@ -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
}
}
}
Expand Down
59 changes: 32 additions & 27 deletions packages/core/src/render3/styling_next/bindings.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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) {
Expand Down
54 changes: 25 additions & 29 deletions packages/core/src/render3/styling_next/instructions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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;
Expand Down
9 changes: 6 additions & 3 deletions packages/core/src/render3/styling_next/util.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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--';
Expand Down Expand Up @@ -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;

Expand Down
40 changes: 38 additions & 2 deletions packages/core/test/acceptance/styling_next_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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: `
<div
[style.width]="w"
style.height="{{ h }}"
[style.opacity]="o"></div>
`
})
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) {
Expand Down

0 comments on commit 7cc4225

Please sign in to comment.