cargdev/angular
1
0
Fork 0
Code Issues 6 Pull Requests Actions Packages Projects Releases Wiki Activity

feat(ivy): Change static priority resolution to be same level as directive it belongs to (#34938)

Browse Source 
This change changes the priority order of static styling.

Current priority:
```
(least priority)
- Static
  - Component
  - Directives
  - Template
- Dynamic Binding
  - Component
    - Map/Interpolation
    - Property
  - Directives
    - Map/Interpolation
    - Property
  - Template
    - Map/Interpolation
    - Property
(highest priority)
```

The issue with the above priority is this use case:

```
<div style="color: red;" directive-which-sets-color-blue>
```
In the above case the directive will win and the resulting color will be `blue`. However a small change of adding interpolation to the example like so. (Style interpolation is coming in https://github.com/angular/angular/pull/34202)
```
<div style="color: red; width: {{exp}}px" directive-which-sets-color-blue>
```
Changes the priority from static binding to interpolated binding which means now the resulting color is `red`. It is very surprising that adding an unrelated interpolation and style can change the `color` which was not changed. To fix that we need to make sure that the static values are associated with priority of the source (directive or template) where they were declared. The new resulting priority is:

```
(least priority)
- Component
  - Static
  - Map/Interpolation
  - Property
- Directives
  - Static
  - Map/Interpolation
  - Property
- Template
  - Static
  - Map/Interpolation
  - Property
(highest priority)
```

PR Close #34938
This commit is contained in:
Misko Hevery
2020-01-26 12:29:03 -08:00
committed by Andrew Kushnir
parent dd0084609b
commit 19c489524f
16 changed files with 953 additions and 207 deletions
Download Patch File Download Diff File Expand all files Collapse all files

45
packages/core/test/render3/instructions/lview_debug_spec.ts
View File

@ -12,6 +12,7 @@ import {TNodeType} from '@angular/core/src/render3/interfaces/node';
import {LView, TView, TViewType} from '@angular/core/src/render3/interfaces/view';
import {enterView, leaveView} from '@angular/core/src/render3/state';
import {insertTStylingBinding} from '@angular/core/src/render3/styling/style_binding_list';
import {ArrayMap} from '@angular/core/src/util/array_utils';
describe('lView_debug', () => {
@ -35,19 +36,19 @@ describe('lView_debug', () => {
});
it('should decode static styling', () => {
tNode.styles = 'color: blue';
tNode.classes = 'STATIC';
expect(tNode.styleBindings_).toEqual(['color: blue']);
expect(tNode.classBindings_).toEqual(['STATIC']);
tNode.residualStyles = ['color', 'blue'] as ArrayMap<any>;
tNode.residualClasses = ['STATIC', true] as ArrayMap<any>;
expect(tNode.styleBindings_).toEqual([['color', 'blue'] as ArrayMap<any>]);
expect(tNode.classBindings_).toEqual([['STATIC', true] as ArrayMap<any>]);
});
it('should decode no-template property binding', () => {
tNode.classes = 'STATIC';
tNode.residualClasses = ['STATIC', true] as ArrayMap<any>;
insertTStylingBinding(tView.data, tNode, 'CLASS', 2, true, true);
insertTStylingBinding(tView.data, tNode, 'color', 4, true, false);
expect(tNode.styleBindings_).toEqual([
null, {
{
index: 4,
key: 'color',
isTemplate: false,
@ -55,10 +56,11 @@ describe('lView_debug', () => {
nextDuplicate: false,
prevIndex: 0,
nextIndex: 0,
}
},
null
]);
expect(tNode.classBindings_).toEqual([
'STATIC', {
{
index: 2,
key: 'CLASS',
isTemplate: false,
@ -66,17 +68,18 @@ describe('lView_debug', () => {
nextDuplicate: false,
prevIndex: 0,
nextIndex: 0,
}
},
['STATIC', true] as ArrayMap<any>
]);
});
it('should decode template and directive property binding', () => {
tNode.classes = 'STATIC';
tNode.residualClasses = ['STATIC', true] as ArrayMap<any>;
insertTStylingBinding(tView.data, tNode, 'CLASS', 2, false, true);
insertTStylingBinding(tView.data, tNode, 'color', 4, false, false);
expect(tNode.styleBindings_).toEqual([
null, {
{
index: 4,
key: 'color',
isTemplate: true,
@ -84,10 +87,11 @@ describe('lView_debug', () => {
nextDuplicate: false,
prevIndex: 0,
nextIndex: 0,
}
},
null
]);
expect(tNode.classBindings_).toEqual([
'STATIC', {
{
index: 2,
key: 'CLASS',
isTemplate: true,
@ -95,14 +99,15 @@ describe('lView_debug', () => {
nextDuplicate: false,
prevIndex: 0,
nextIndex: 0,
}
},
['STATIC', true] as ArrayMap<any>
]);
insertTStylingBinding(tView.data, tNode, null, 6, true, true);
insertTStylingBinding(tView.data, tNode, null, 8, true, false);
expect(tNode.styleBindings_).toEqual([
null, {
{
index: 8,
key: null,
isTemplate: false,
@ -119,14 +124,15 @@ describe('lView_debug', () => {
nextDuplicate: false,
prevIndex: 8,
nextIndex: 0,
}
},
null
]);
expect(tNode.classBindings_).toEqual([
'STATIC', {
{
index: 6,
key: null,
isTemplate: false,
prevDuplicate: true,
prevDuplicate: false,
nextDuplicate: true,
prevIndex: 0,
nextIndex: 2,
@ -139,7 +145,8 @@ describe('lView_debug', () => {
nextDuplicate: false,
prevIndex: 6,
nextIndex: 0,
}
},
['STATIC', true] as ArrayMap<any>
]);
});
});

9
packages/core/test/render3/instructions/shared_spec.ts
View File

@ -32,8 +32,13 @@ import {enterView, getBindingRoot, getLView, setBindingIndex} from '@angular/cor
export function enterViewWithOneDiv() {
const renderer = domRendererFactory3.createRenderer(null, null);
const div = renderer.createElement('div');
const tView =
createTView(TViewType.Component, -1, emptyTemplate, 1, 10, null, null, null, null, null);
const consts = 1;
const vars = 60; // Space for directive expando, template, component + 3 directives if we assume
// that each consume 10 slots.
const tView = createTView(
TViewType.Component, -1, emptyTemplate, consts, vars, null, null, null, null, null);
// Just assume that the expando starts after 10 initial bindings.
tView.expandoStartIndex = HEADER_OFFSET + 10;
const tNode = tView.firstChild = createTNode(tView, null !, TNodeType.Element, 0, 'div', null);
const lView = createLView(
null, tView, null, LViewFlags.CheckAlways, null, null, domRendererFactory3, renderer, null,

283
packages/core/test/render3/instructions/styling_spec.ts
View File

@ -6,10 +6,13 @@
* found in the LICENSE file at https://angular.io/license
*/
import {DirectiveDef} from '@angular/core/src/render3';
import {ɵɵdefineDirective} from '@angular/core/src/render3/definition';
import {classStringParser, initializeStylingStaticArrayMap, styleStringParser, toStylingArrayMap, ɵɵclassProp, ɵɵstyleMap, ɵɵstyleProp, ɵɵstyleSanitizer} from '@angular/core/src/render3/instructions/styling';
import {AttributeMarker} from '@angular/core/src/render3/interfaces/node';
import {TVIEW} from '@angular/core/src/render3/interfaces/view';
import {getLView, leaveView} from '@angular/core/src/render3/state';
import {AttributeMarker, TAttributes, TDirectiveDefs} from '@angular/core/src/render3/interfaces/node';
import {StylingRange, TStylingKey, TStylingRange, getTStylingRangeNext, getTStylingRangeNextDuplicate, getTStylingRangePrev, getTStylingRangePrevDuplicate, setTStylingRangeNext, setTStylingRangePrev, toTStylingRange} from '@angular/core/src/render3/interfaces/styling';
import {HEADER_OFFSET, TVIEW} from '@angular/core/src/render3/interfaces/view';
import {getLView, leaveView, setBindingRootForHostBindings} from '@angular/core/src/render3/state';
import {getNativeByIndex} from '@angular/core/src/render3/util/view_utils';
import {bypassSanitizationTrustStyle} from '@angular/core/src/sanitization/bypass';
import {ɵɵsanitizeStyle} from '@angular/core/src/sanitization/sanitization';
@ -269,25 +272,155 @@ describe('styling', () => {
describe('populateStylingStaticArrayMap', () => {
it('should initialize to null if no mergedAttrs', () => {
const tNode = getLView()[TVIEW].firstChild !;
expect(tNode.stylesMap).toEqual(undefined);
expect(tNode.classesMap).toEqual(undefined);
expect(tNode.residualStyles).toEqual(undefined);
expect(tNode.residualClasses).toEqual(undefined);
initializeStylingStaticArrayMap(tNode);
expect(tNode.stylesMap).toEqual(null);
expect(tNode.classesMap).toEqual(null);
expect(tNode.residualStyles).toEqual(null);
expect(tNode.residualClasses).toEqual(null);
});
it('should initialize from mergeAttrs', () => {
const tNode = getLView()[TVIEW].firstChild !;
expect(tNode.stylesMap).toEqual(undefined);
expect(tNode.classesMap).toEqual(undefined);
expect(tNode.residualStyles).toEqual(undefined);
expect(tNode.residualClasses).toEqual(undefined);
tNode.mergedAttrs = [
'ignore', 'value', //
AttributeMarker.Classes, 'foo', 'bar', //
AttributeMarker.Styles, 'width', '0', 'color', 'red', //
];
initializeStylingStaticArrayMap(tNode);
expect(tNode.classesMap).toEqual(['bar', true, 'foo', true] as any);
expect(tNode.stylesMap).toEqual(['color', 'red', 'width', '0'] as any);
expect(tNode.residualClasses).toEqual(['bar', true, 'foo', true] as any);
expect(tNode.residualStyles).toEqual(['color', 'red', 'width', '0'] as any);
});
});
});
describe('static', () => {
describe('template only', () => {
it('should capture static values in TStylingKey', () => {
givenTemplateAttrs([AttributeMarker.Styles, 'content', '"TEMPLATE"']);
ɵɵstyleProp('content', '"dynamic"');
expectTStylingKeys('style').toEqual([
['', 'content', 'content', '"TEMPLATE"'], 'prev', // contains statics
null // residual
]);
expectStyle(div).toEqual({content: '"dynamic"'});
ɵɵstyleProp('content', undefined);
expectTStylingKeys('style').toEqual([
['', 'content', 'content', '"TEMPLATE"'], 'both', // contains statics
'content', 'prev', // Making sure that second instruction does not have statics again.
null // residual
]);
expectStyle(div).toEqual({content: '"dynamic"'});
});
});
describe('directives only', () => {
it('should update residual on second directive', () => {
givenDirectiveAttrs([
[AttributeMarker.Styles, 'content', '"lowest"'], // 0
[AttributeMarker.Styles, 'content', '"middle"'], // 1
]);
expectStyle(div).toEqual({content: '"middle"'});
activateHostBindings(0);
ɵɵstyleProp('content', '"dynamic"');
expectTStylingKeys('style').toEqual([
['', 'content', 'content', '"lowest"'], 'both', // 1: contains statics
['content', '"middle"'], // residual
]);
expectStyle(div).toEqual({content: '"middle"'});
// second binding should not get statics
ɵɵstyleProp('content', '"dynamic2"');
expectTStylingKeys('style').toEqual([
['', 'content', 'content', '"lowest"'], 'both', // 1: contains statics
'content', 'both', // 1: Should not get statics
['content', '"middle"'] // residual
]);
expectStyle(div).toEqual({content: '"middle"'});
activateHostBindings(1);
ɵɵstyleProp('content', '"dynamic3"');
expectTStylingKeys('style').toEqual([
['', 'content', 'content', '"lowest"'], 'both', // 1: contains statics
'content', 'both', // 1: Should not get statics
['', 'content', 'content', '"middle"'], 'prev', // 0: contains statics
null // residual
]);
expectStyle(div).toEqual({content: '"dynamic3"'});
});
});
describe('template and directives', () => {
it('should combine property and map', () => {
givenDirectiveAttrs([
[AttributeMarker.Styles, 'content', '"lowest"', 'color', 'blue'], // 0
[AttributeMarker.Styles, 'content', '"middle"', 'width', '100px'], // 1
]);
givenTemplateAttrs([AttributeMarker.Styles, 'content', '"TEMPLATE"', 'color', 'red']);
// TEMPLATE
ɵɵstyleProp('content', undefined);
expectTStylingKeys('style').toEqual([
// TEMPLATE
['', 'content', 'color', 'red', 'content', '"TEMPLATE"', 'width', '100px'], 'prev',
// RESIDUAL
null
]);
expectStyle(div).toEqual({content: '"TEMPLATE"', color: 'red', width: '100px'});
// Directive 0
activateHostBindings(0);
ɵɵstyleMap('color: red; width: 0px; height: 50px');
expectTStylingKeys('style').toEqual([
// Host Binding 0
['', null, 'color', 'blue', 'content', '"lowest"'], 'both',
// TEMPLATE
['', 'content', 'color', 'red', 'content', '"TEMPLATE"', 'width', '100px'], 'prev',
// RESIDUAL
null
]);
expectStyle(div).toEqual(
{content: '"TEMPLATE"', color: 'red', width: '100px', height: '50px'});
// Directive 1
activateHostBindings(1);
ɵɵstyleMap('color: red; width: 0px; height: 50px');
expectTStylingKeys('style').toEqual([
// Host Binding 0
['', null, 'color', 'blue', 'content', '"lowest"'], 'both',
// Host Binding 1
['', null, 'content', '"middle"', 'width', '100px'], 'both',
// TEMPLATE
['', 'content', 'color', 'red', 'content', '"TEMPLATE"'], 'prev',
// RESIDUAL
null
]);
expectStyle(div).toEqual(
{content: '"TEMPLATE"', color: 'red', width: '0px', height: '50px'});
});
it('should read value from residual', () => {
givenDirectiveAttrs([
[AttributeMarker.Styles, 'content', '"lowest"', 'color', 'blue'], // 0
[AttributeMarker.Styles, 'content', '"middle"', 'width', '100px'], // 1
]);
givenTemplateAttrs([AttributeMarker.Styles, 'content', '"TEMPLATE"', 'color', 'red']);
// Directive 1
activateHostBindings(1);
ɵɵstyleProp('color', 'white');
expectTStylingKeys('style').toEqual([
// Host Binding 0 + 1
['', 'color', 'color', 'blue', 'content', '"middle"', 'width', '100px'], 'both',
// RESIDUAL
['color', 'red', 'content', '"TEMPLATE"']
]);
expectStyle(div).toEqual({content: '"TEMPLATE"', color: 'red', width: '100px'});
});
});
});
@ -346,6 +479,41 @@ describe('styling', () => {
});
});
});
describe('TStylingRange', () => {
const MAX_VALUE = StylingRange.UNSIGNED_MASK;
it('should throw on negative values', () => {
expect(() => toTStylingRange(0, -1)).toThrow();
expect(() => toTStylingRange(-1, 0)).toThrow();
});
it('should throw on overflow', () => {
expect(() => toTStylingRange(0, MAX_VALUE + 1)).toThrow();
expect(() => toTStylingRange(MAX_VALUE + 1, 0)).toThrow();
});
it('should retrieve the same value which went in just below overflow', () => {
const range = toTStylingRange(MAX_VALUE, MAX_VALUE);
expect(getTStylingRangePrev(range)).toEqual(MAX_VALUE);
expect(getTStylingRangeNext(range)).toEqual(MAX_VALUE);
});
it('should correctly increment', () => {
let range = toTStylingRange(0, 0);
for (let i = 0; i <= MAX_VALUE; i++) {
range = setTStylingRangeNext(range, i);
range = setTStylingRangePrev(range, i);
expect(getTStylingRangeNext(range)).toEqual(i);
expect(getTStylingRangePrev(range)).toEqual(i);
if (i == 10) {
// Skip the boring stuff in the middle.
i = MAX_VALUE - 10;
}
}
});
});
});
@ -355,4 +523,97 @@ function expectStyle(element: HTMLElement) {
function expectClass(element: HTMLElement) {
return expect(getElementClasses(element));
}
function givenTemplateAttrs(tAttrs: TAttributes) {
const tNode = getTNode();
tNode.attrs = tAttrs;
applyTAttributes(tAttrs);
}
function getTNode() {
return getLView()[TVIEW].firstChild !;
}
function getTData() {
return getLView()[TVIEW].data;
}
class MockDir {}
function givenDirectiveAttrs(tAttrs: TAttributes[]) {
const tNode = getTNode();
const tData = getTData();
const directives: TDirectiveDefs = tNode.directives = [0];
for (let i = 0; i < tAttrs.length; i++) {
const tAttr = tAttrs[i];
const directiveDef = ɵɵdefineDirective({type: MockDir, hostAttrs: tAttr}) as DirectiveDef<any>;
applyTAttributes(directiveDef.hostAttrs);
tData[getTDataIndexFromDirectiveIndex(i)] = directiveDef;
directives.push(directiveDef);
}
}
function applyTAttributes(attrs: TAttributes | null) {
if (attrs === null) return;
const div: HTMLElement = getLView()[HEADER_OFFSET];
let mode: AttributeMarker = AttributeMarker.ImplicitAttributes;
for (let i = 0; i < attrs.length; i++) {
const item = attrs[i];
if (typeof item === 'number') {
mode = item;
} else if (typeof item === 'string') {
if (mode == AttributeMarker.ImplicitAttributes) {
div.setAttribute(item, attrs[++i] as string);
} else if (mode == AttributeMarker.Classes) {
div.classList.add(item);
} else if (mode == AttributeMarker.Styles) {
div.style.setProperty(item, attrs[++i] as string);
}
}
}
}
function activateHostBindings(directiveIndex: number) {
const bindingRootIndex = getBindingRootIndexFromDirectiveIndex(directiveIndex);
const currentDirectiveIndex = getTDataIndexFromDirectiveIndex(directiveIndex);
setBindingRootForHostBindings(bindingRootIndex, currentDirectiveIndex);
}
function getBindingRootIndexFromDirectiveIndex(index: number) {
// For simplicity assume that each directive has 10 vars.
// We need to offset 1 for template, and 1 for expando.
return HEADER_OFFSET + (index + 2) * 10;
}
function getTDataIndexFromDirectiveIndex(index: number) {
return HEADER_OFFSET + index + 10; // offset to give template bindings space.
}
function expectTStylingKeys(styling: 'style' | 'class') {
const tNode = getTNode();
const tData = getTData();
const isClassBased = styling === 'class';
const headIndex = getTStylingRangePrev(isClassBased ? tNode.classBindings : tNode.styleBindings);
const tStylingKeys: (string | (null | string)[] | null)[] = [];
let index = headIndex;
let prevIndex = index;
// rewind to beginning of list.
while ((prevIndex = getTStylingRangePrev(tData[index + 1] as TStylingRange)) !== 0) {
index = prevIndex;
}
// insert into array.
while (index !== 0) {
const tStylingKey = tData[index] as TStylingKey;
const prevDup = getTStylingRangePrevDuplicate(tData[index + 1] as TStylingRange);
const nextDup = getTStylingRangeNextDuplicate(tData[index + 1] as TStylingRange);
tStylingKeys.push(tStylingKey as string[] | string | null);
tStylingKeys.push(prevDup ? (nextDup ? 'both' : 'prev') : (nextDup ? 'next' : ''));
index = getTStylingRangeNext(tData[index + 1] as TStylingRange);
}
tStylingKeys.push(
(isClassBased ? tNode.residualClasses : tNode.residualStyles) as null | string[]);
return expect(tStylingKeys);
}

6
packages/core/test/render3/styling_next/style_binding_list_spec.ts
View File

@ -407,7 +407,7 @@ describe('TNode styling linked list', () => {
it('should mark duplicate on static fields', () => {
const tNode = createTNode(null !, null !, TNodeType.Element, 0, '', null);
tNode.styles = 'color: blue;';
tNode.residualStyles = ['color', 'blue'] as any;
const tData: TData = [null, null];
insertTStylingBinding(tData, tNode, 'width', 2, false, false);
expectPriorityOrder(tData, tNode, false).toEqual([
@ -419,14 +419,14 @@ describe('TNode styling linked list', () => {
expectPriorityOrder(tData, tNode, false).toEqual([
// PREV, NEXT
[2, 'width', false, false],
[4, 'color', true, false],
[4, 'color', false, true],
]);
insertTStylingBinding(tData, tNode, null, 6, false, false);
expectPriorityOrder(tData, tNode, false).toEqual([
// PREV, NEXT
[2, 'width', false, true],
[4, 'color', true, true],
[4, 'color', false, true],
[6, null, true, false],
]);
});