From 86fd5719b5ada3f2e478b814a1aef7fadf303064 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Thu, 19 Sep 2019 11:23:19 -0700 Subject: [PATCH] fix(ivy): ensure multiple map-based bindings do not skip intermediate values (#32774) This patch fixes a bug where the map-based cursor moves too far and skips intermediate values when the correct combination of single-prop bindings and map-based bindings are used together. PR Close #32774 --- packages/core/src/render3/styling/bindings.ts | 33 ++++++++++----- .../src/render3/styling/map_based_bindings.ts | 21 +++++++--- packages/core/test/acceptance/styling_spec.ts | 40 +++++++++++++++++++ 3 files changed, 79 insertions(+), 15 deletions(-) diff --git a/packages/core/src/render3/styling/bindings.ts b/packages/core/src/render3/styling/bindings.ts index a81f1e4165..aba76032ec 100644 --- a/packages/core/src/render3/styling/bindings.ts +++ b/packages/core/src/render3/styling/bindings.ts @@ -152,7 +152,7 @@ function updateBindingData( const doSetValuesAsStale = (getConfig(context) & TStylingConfig.HasHostBindings) && !hostBindingsMode && (prop ? !value : true); if (doSetValuesAsStale) { - renderHostBindingsAsStale(context, data, prop, !prop); + renderHostBindingsAsStale(context, data, prop); } } return changed; @@ -170,28 +170,32 @@ function updateBindingData( * binding changes. */ function renderHostBindingsAsStale( - context: TStylingContext, data: LStylingData, prop: string | null, isMapBased: boolean): void { + context: TStylingContext, data: LStylingData, prop: string | null): void { const valuesCount = getValuesCount(context); - if (hasConfig(context, TStylingConfig.HasPropBindings)) { + if (prop !== null && hasConfig(context, TStylingConfig.HasPropBindings)) { const itemsPerRow = TStylingContextIndex.BindingsStartOffset + valuesCount; let i = TStylingContextIndex.ValuesStartPosition; + let found = false; while (i < context.length) { if (getProp(context, i) === prop) { + found = true; break; } i += itemsPerRow; } - const bindingsStart = i + TStylingContextIndex.BindingsStartOffset; - const valuesStart = bindingsStart + 1; // the first column is template bindings - const valuesEnd = bindingsStart + valuesCount - 1; + if (found) { + const bindingsStart = i + TStylingContextIndex.BindingsStartOffset; + const valuesStart = bindingsStart + 1; // the first column is template bindings + const valuesEnd = bindingsStart + valuesCount - 1; - for (let i = valuesStart; i < valuesEnd; i++) { - const bindingIndex = context[i] as number; - if (bindingIndex !== 0) { - setValue(data, bindingIndex, null); + for (let i = valuesStart; i < valuesEnd; i++) { + const bindingIndex = context[i] as number; + if (bindingIndex !== 0) { + setValue(data, bindingIndex, null); + } } } } @@ -575,9 +579,18 @@ export function applyStylingViaContext( // determine whether or not to apply the target property or to skip it let mode = mapsMode | (valueApplied ? StylingMapsSyncMode.SkipTargetProp : StylingMapsSyncMode.ApplyTargetProp); + + // the first column in the context (when `j == 0`) is special-cased for + // template bindings. If and when host bindings are being processed then + // the first column will still be iterated over, but the values will only + // be checked against (not applied). If and when this happens we need to + // notify the map-based syncing code to know not to apply the values it + // comes across in the very first map-based binding (which is also located + // in column zero). if (hostBindingsMode && j === 0) { mode |= StylingMapsSyncMode.CheckValuesOnly; } + const valueAppliedWithinMap = stylingMapsSyncFn( context, renderer, element, bindingData, j, applyStylingFn, sanitizer, mode, prop, defaultValue); diff --git a/packages/core/src/render3/styling/map_based_bindings.ts b/packages/core/src/render3/styling/map_based_bindings.ts index 709cc70f81..2b6974f833 100644 --- a/packages/core/src/render3/styling/map_based_bindings.ts +++ b/packages/core/src/render3/styling/map_based_bindings.ts @@ -14,7 +14,6 @@ import {getBindingValue, getMapProp, getMapValue, getValue, getValuesCount, isSt import {setStylingMapsSyncFn} from './bindings'; - /** * -------- * @@ -267,19 +266,31 @@ function innerSyncStylingMap( * * - value is being applied: * if the value is being applied from this current styling - * map then there is no need to apply it in a deeper map. + * map then there is no need to apply it in a deeper map + * (i.e. the `SkipTargetProp` flag is set) * * - value is being not applied: * apply the value if it is found in a deeper map. + * (i.e. the `SkipTargetProp` flag is unset) * * When these reasons are encountered the flags will for the * inner map mode will be configured. */ function resolveInnerMapMode( - currentMode: number, valueIsDefined: boolean, isExactMatch: boolean): number { + currentMode: number, valueIsDefined: boolean, isTargetPropMatched: boolean): number { let innerMode = currentMode; - if (!valueIsDefined && !(currentMode & StylingMapsSyncMode.SkipTargetProp) && - (isExactMatch || (currentMode & StylingMapsSyncMode.ApplyAllValues))) { + + // the statements below figures out whether or not an inner styling map + // is allowed to apply its value or not. The main thing to keep note + // of is that if the target prop isn't matched then its expected that + // all values before it are allowed to be applied so long as "apply all values" + // is set to true. + const applyAllValues = currentMode & StylingMapsSyncMode.ApplyAllValues; + const applyTargetProp = currentMode & StylingMapsSyncMode.ApplyTargetProp; + const allowInnerApply = + !valueIsDefined && (isTargetPropMatched ? applyTargetProp : applyAllValues); + + if (allowInnerApply) { // case 1: set the mode to apply the targeted prop value if it // ends up being encountered in another map value innerMode |= StylingMapsSyncMode.ApplyTargetProp; diff --git a/packages/core/test/acceptance/styling_spec.ts b/packages/core/test/acceptance/styling_spec.ts index 123e9992b6..72c5bb2731 100644 --- a/packages/core/test/acceptance/styling_spec.ts +++ b/packages/core/test/acceptance/styling_spec.ts @@ -1995,6 +1995,46 @@ describe('styling', () => { expect(readyHost).toBeTruthy(); expect(readyChild).toBeTruthy(); }); + + onlyInIvy('only ivy allows for multiple styles/classes to be balanaced across directives') + .it('should allow various duplicate properties to be defined in various styling maps within the template and directive styling bindings', + () => { + @Component({ + template: ` +
+ ` + }) + class Cmp { + h = '100px'; + w = '100px'; + s1: any = {border: '10px solid black', width: '200px'}; + s2: any = {border: '10px solid red', width: '300px'}; + } + + @Directive({selector: '[dir-with-styling]'}) + class DirectiveExpectingStyling { + @Input('dir-with-styling') @HostBinding('style') public styles: any = null; + } + + TestBed.configureTestingModule({declarations: [Cmp, DirectiveExpectingStyling]}); + const fixture = TestBed.createComponent(Cmp); + fixture.detectChanges(); + + const element = fixture.nativeElement.querySelector('div'); + expect(element.style.border).toEqual('10px solid black'); + expect(element.style.width).toEqual('100px'); + expect(element.style.height).toEqual('100px'); + + fixture.componentInstance.s1 = null; + fixture.detectChanges(); + + expect(element.style.border).toEqual('10px solid red'); + expect(element.style.width).toEqual('100px'); + expect(element.style.height).toEqual('100px'); + }); }); function getDebugNode(element: Node): DebugNode|null {