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

refactor(ivy): remove styling state storage and introduce direct style writing (#32591)

Browse Source 
This patch is a final major refactor in styling Angular.

This PR includes three main fixes:

All temporary state taht is persisted between template style/class application
and style/class application in host bindings is now removed.
Removes the styling() and stylingApply() instructions.
Introduces a "direct apply" mode that is used apply prop-based
style/class in the event that there are no map-based bindings as
well as property collisions.

PR Close #32259

PR Close #32591
This commit is contained in:
Matias Niemelä
2019-09-09 13:14:26 -07:00
committed by Andrew Kushnir
parent e6ed4a21e4
commit 4f41473048
46 changed files with 1756 additions and 1614 deletions
Download Patch File Download Diff File Expand all files Collapse all files

232
packages/core/test/acceptance/styling_next_spec.ts
View File

@ -15,6 +15,8 @@ import {expect} from '@angular/platform-browser/testing/src/matchers';
import {onlyInIvy} from '@angular/private/testing';
describe('new styling integration', () => {
beforeEach(() => resetStylingCounters());
onlyInIvy('ivy resolves styling across directives, components and templates in unison')
.it('should apply single property styles/classes to the element and default to any static styling values',
() => {
@ -124,6 +126,74 @@ describe('new styling integration', () => {
expect(element.style.width).toEqual('600px');
});
onlyInIvy('ivy resolves styling across directives, components and templates in unison')
.it('should only run stylingFlush once when there are no collisions between styling properties',
() => {
@Directive({selector: '[dir-with-styling]'})
class DirWithStyling {
@HostBinding('style.font-size') public fontSize = '100px';
}
@Component({selector: 'comp-with-styling'})
class CompWithStyling {
@HostBinding('style.width') public width = '900px';
@HostBinding('style.height') public height = '900px';
}
@Component({
template: `
<comp-with-styling
[style.opacity]="opacity"
dir-with-styling>...</comp-with-styling>
`
})
class Cmp {
opacity: string|null = '0.5';
@ViewChild(CompWithStyling, {static: true})
compWithStyling: CompWithStyling|null = null;
@ViewChild(DirWithStyling, {static: true}) dirWithStyling: DirWithStyling|null = null;
}
TestBed.configureTestingModule({declarations: [Cmp, DirWithStyling, CompWithStyling]});
const fixture = TestBed.createComponent(Cmp);
fixture.detectChanges();
const component = fixture.componentInstance;
const element = fixture.nativeElement.querySelector('comp-with-styling');
const node = getDebugNode(element) !;
const styles = node.styles !;
const config = styles.config;
expect(config.hasCollisions).toBeFalsy();
expect(config.hasMapBindings).toBeFalsy();
expect(config.hasPropBindings).toBeTruthy();
expect(config.allowDirectStyling).toBeTruthy();
expect(element.style.opacity).toEqual('0.5');
expect(element.style.width).toEqual('900px');
expect(element.style.height).toEqual('900px');
expect(element.style.fontSize).toEqual('100px');
// once for the template flush and again for the host bindings
expect(ngDevMode !.flushStyling).toEqual(2);
resetStylingCounters();
component.opacity = '0.6';
component.compWithStyling !.height = '100px';
component.compWithStyling !.width = '100px';
component.dirWithStyling !.fontSize = '50px';
fixture.detectChanges();
expect(element.style.opacity).toEqual('0.6');
expect(element.style.width).toEqual('100px');
expect(element.style.height).toEqual('100px');
expect(element.style.fontSize).toEqual('50px');
// there is no need to flush styling since the styles are applied directly
expect(ngDevMode !.flushStyling).toEqual(0);
});
onlyInIvy('ivy resolves styling across directives, components and templates in unison')
.it('should combine all styling across the template, directive and component host bindings',
() => {
@ -240,6 +310,7 @@ describe('new styling integration', () => {
fixture.componentInstance.w3 = null;
fixture.detectChanges();
expect(styles.values).toEqual({
'width': '200px',
});
@ -309,16 +380,22 @@ describe('new styling integration', () => {
.it('should apply map-based style and class entries', () => {
@Component({template: '<div [style]="s" [class]="c"></div>'})
class Cmp {
public c !: {[key: string]: any};
updateClasses(prop: string) {
this.c = {...this.c || {}};
this.c[prop] = true;
public c: {[key: string]: any}|null = null;
updateClasses(classes: string) {
const c = this.c || (this.c = {});
Object.keys(this.c).forEach(className => { c[className] = false; });
classes.split(/\s+/).forEach(className => { c[className] = true; });
}
public s !: {[key: string]: any};
public s: {[key: string]: any}|null = null;
updateStyles(prop: string, value: string|number|null) {
this.s = {...this.s || {}};
this.s[prop] = value;
const s = this.s || (this.s = {});
Object.assign(s, {[prop]: value});
}
reset() {
this.s = null;
this.c = null;
}
}
@ -332,22 +409,47 @@ describe('new styling integration', () => {
const element = fixture.nativeElement.querySelector('div');
const node = getDebugNode(element) !;
const styles = node.styles !;
const classes = node.classes !;
let styles = node.styles !;
let classes = node.classes !;
const stylesSummary = styles.summary;
const widthSummary = stylesSummary['width'];
let stylesSummary = styles.summary;
let widthSummary = stylesSummary['width'];
expect(widthSummary.prop).toEqual('width');
expect(widthSummary.value).toEqual('100px');
const heightSummary = stylesSummary['height'];
let heightSummary = stylesSummary['height'];
expect(heightSummary.prop).toEqual('height');
expect(heightSummary.value).toEqual('200px');
const classesSummary = classes.summary;
const abcSummary = classesSummary['abc'];
let classesSummary = classes.summary;
let abcSummary = classesSummary['abc'];
expect(abcSummary.prop).toEqual('abc');
expect(abcSummary.value as any).toEqual(true);
expect(abcSummary.value).toBeTruthy();
comp.reset();
comp.updateStyles('width', '500px');
comp.updateStyles('height', null);
comp.updateClasses('def');
fixture.detectChanges();
styles = node.styles !;
classes = node.classes !;
stylesSummary = styles.summary;
widthSummary = stylesSummary['width'];
expect(widthSummary.value).toEqual('500px');
heightSummary = stylesSummary['height'];
expect(heightSummary.value).toEqual(null);
classesSummary = classes.summary;
abcSummary = classesSummary['abc'];
expect(abcSummary.prop).toEqual('abc');
expect(abcSummary.value).toBeFalsy();
let defSummary = classesSummary['def'];
expect(defSummary.prop).toEqual('def');
expect(defSummary.value).toBeTruthy();
});
onlyInIvy('ivy resolves styling across directives, components and templates in unison')
@ -385,6 +487,7 @@ describe('new styling integration', () => {
const node = getDebugNode(element) !;
const styles = node.styles !;
expect(styles.values).toEqual({
'width': '555px',
'color': 'red',
@ -509,7 +612,8 @@ describe('new styling integration', () => {
resetStylingCounters();
fixture.detectChanges();
assertStyleCounters(1, 0);
// the width is applied both in TEMPLATE and in HOST_BINDINGS mode
assertStyleCounters(2, 0);
assertStyle(element, 'width', '999px');
assertStyle(element, 'height', '123px');
@ -517,8 +621,8 @@ describe('new styling integration', () => {
resetStylingCounters();
fixture.detectChanges();
// both are applied because the map was altered
assertStyleCounters(2, 0);
// the width is only applied once
assertStyleCounters(1, 0);
assertStyle(element, 'width', '0px');
assertStyle(element, 'height', '123px');
@ -526,8 +630,8 @@ describe('new styling integration', () => {
resetStylingCounters();
fixture.detectChanges();
// all three are applied because the map was altered
assertStyleCounters(3, 0);
// only the width and color have changed
assertStyleCounters(2, 0);
assertStyle(element, 'width', '1000px');
assertStyle(element, 'height', '123px');
assertStyle(element, 'color', 'red');
@ -536,7 +640,9 @@ describe('new styling integration', () => {
resetStylingCounters();
fixture.detectChanges();
assertStyleCounters(1, 0);
// height gets applied twice and all other
// values get applied
assertStyleCounters(4, 0);
assertStyle(element, 'width', '1000px');
assertStyle(element, 'height', '1000px');
assertStyle(element, 'color', 'red');
@ -545,8 +651,7 @@ describe('new styling integration', () => {
resetStylingCounters();
fixture.detectChanges();
// all four are applied because the map was altered
assertStyleCounters(4, 0);
assertStyleCounters(5, 0);
assertStyle(element, 'width', '2000px');
assertStyle(element, 'height', '1000px');
assertStyle(element, 'color', 'blue');
@ -557,62 +662,13 @@ describe('new styling integration', () => {
fixture.detectChanges();
// all four are applied because the map was altered
assertStyleCounters(3, 1);
assertStyleCounters(4, 1);
assertStyle(element, 'width', '2000px');
assertStyle(element, 'height', '1000px');
assertStyle(element, 'color', 'blue');
assertStyle(element, 'opacity', '');
});
onlyInIvy('ivy resolves styling across directives, components and templates in unison')
.it('should only persist state values in a local map if template AND host styling is used together',
() => {
@Directive({selector: '[dir-that-sets-styling]'})
class Dir {
@HostBinding('style.width') w = '100px';
}
@Component({
template: `
<div #a dir-that-sets-styling></div>
<div #b [style.width]="w"></div>
<div #c dir-that-sets-styling [style.width]="w"></div>
`
})
class Cmp {
w = '200px';
@ViewChild('a', {read: Dir, static: true}) a !: Dir;
@ViewChild('c', {read: Dir, static: true}) c !: Dir;
}
TestBed.configureTestingModule({declarations: [Cmp, Dir]});
const fixture = TestBed.createComponent(Cmp);
const comp = fixture.componentInstance;
fixture.detectChanges();
resetStylingCounters();
comp.a.w = '999px';
comp.w = '999px';
comp.c.w = '999px';
fixture.detectChanges();
expect(ngDevMode !.stylingWritePersistedState).toEqual(totalUpdates(1));
comp.a.w = '888px';
fixture.detectChanges();
expect(ngDevMode !.stylingWritePersistedState).toEqual(totalUpdates(2));
comp.c.w = '777px';
fixture.detectChanges();
expect(ngDevMode !.stylingWritePersistedState).toEqual(totalUpdates(3));
function totalUpdates(value: number) {
// this is doubled because detectChanges is run twice to
// see to check for checkNoChanges
return value * 2;
}
});
onlyInIvy('only ivy has style/class bindings debugging support')
.it('should sanitize style values before writing them', () => {
@Component({
@ -910,7 +966,6 @@ describe('new styling integration', () => {
TestBed.configureTestingModule({declarations: [Cmp, DirOne, DirTwo]});
const fixture = TestBed.createComponent(Cmp);
const comp = fixture.componentInstance;
fixture.detectChanges();
const dirOne = fixture.nativeElement.querySelector('dir-one');
@ -937,10 +992,10 @@ describe('new styling integration', () => {
@Component({
template: `
<div class="a" [style.width.px]="w" one></div>
<div class="b" [style.height.px]="h" one two></div>
<div class="c" [style.color]="c" two></div>
`
<div class="a" [style.width.px]="w" one></div>
<div class="b" [style.height.px]="h" one two></div>
<div class="c" [style.color]="c" two></div>
`
})
class Cmp {
w = 100;
@ -1092,6 +1147,27 @@ describe('new styling integration', () => {
expect(div.style.height).toEqual('200px');
expect(div.style.opacity).toEqual('1');
});
it('should allow [ngStyle] and [ngClass] to be used together', () => {
@Component({
template: `
<div [ngClass]="c" [ngStyle]="s"></div>
`
})
class Cmp {
c: any = 'foo bar';
s: any = {width: '200px'};
}
TestBed.configureTestingModule({declarations: [Cmp]});
const fixture = TestBed.createComponent(Cmp);
fixture.detectChanges();
const div = fixture.nativeElement.querySelector('div');
expect(div.style.width).toEqual('200px');
expect(div.classList.contains('foo')).toBeTruthy();
expect(div.classList.contains('bar')).toBeTruthy();
});
});
function assertStyleCounters(countForSet: number, countForRemove: number) {

1
packages/core/test/acceptance/styling_spec.ts
View File

@ -628,7 +628,6 @@ describe('styling', () => {
expect(childDir.parent).toBeAnInstanceOf(TestDir);
expect(testDirDiv.classList).not.toContain('with-button');
expect(fixture.debugElement.nativeElement.textContent).toContain('Hello');
});
});