fix(ivy): mark views dirty by default when events fire (#28474)

In Ivy, we support a new manual mode that allows for stricter control
over change detection in OnPush components. Specifically, in this mode,
events do not automatically mark OnPush views as dirty. Only changed
inputs and manual calls to `markDirty()` actually mark a view dirty.

However, this mode cannot be the default for OnPush components if we
want to be backwards compatible with View Engine. This commit re-adds
the legacy logic for OnPush components where events always mark views
dirty and makes it the default behavior.

Note: It is still TODO to add a public API for manual change detection.

PR Close #28474
This commit is contained in:
Kara Erickson 2019-01-30 21:54:44 -08:00 committed by Matias Niemelä
parent 8930f60a4b
commit 5c4d95541e
5 changed files with 332 additions and 135 deletions

View File

@ -956,13 +956,14 @@ function listenerInternal(
// The first argument of `listen` function in Procedural Renderer is:
// - either a target name (as a string) in case of global target (window, document, body)
// - or element reference (in all other cases)
listenerFn = wrapListener(tNode, lView, listenerFn, false /** preventDefault */);
const cleanupFn = renderer.listen(resolved.name || target, eventName, listenerFn);
lCleanup.push(listenerFn, cleanupFn);
useCaptureOrSubIdx = lCleanupIndex + 1;
} else {
const wrappedListener = wrapListenerWithPreventDefault(listenerFn);
target.addEventListener(eventName, wrappedListener, useCapture);
lCleanup.push(wrappedListener);
listenerFn = wrapListener(tNode, lView, listenerFn, true /** preventDefault */);
target.addEventListener(eventName, listenerFn, useCapture);
lCleanup.push(listenerFn);
}
const idxOrTargetGetter = eventTargetResolver ?
@ -2578,17 +2579,41 @@ function markDirtyIfOnPush(lView: LView, viewIndex: number): void {
}
}
/** Wraps an event listener with preventDefault behavior. */
function wrapListenerWithPreventDefault(listenerFn: (e?: any) => any): EventListener {
return function wrapListenerIn_preventDefault(e: Event) {
if (listenerFn(e) === false) {
/**
* Wraps an event listener with a function that marks ancestors dirty and prevents default behavior,
* if applicable.
*
* @param tNode The TNode associated with this listener
* @param lView The LView that contains this listener
* @param listenerFn The listener function to call
* @param wrapWithPreventDefault Whether or not to prevent default behavior
* (the procedural renderer does this already, so in those cases, we should skip)
*/
function wrapListener(
tNode: TNode, lView: LView, listenerFn: (e?: any) => any,
wrapWithPreventDefault: boolean): EventListener {
// Note: we are performing most of the work in the listener function itself
// to optimize listener registration.
return function wrapListenerIn_markDirtyAndPreventDefault(e: Event) {
// In order to be backwards compatible with View Engine, events on component host nodes
// must also mark the component view itself dirty (i.e. the view that it owns).
const startView =
tNode.flags & TNodeFlags.isComponent ? getComponentViewByIndex(tNode.index, lView) : lView;
// See interfaces/view.ts for more on LViewFlags.ManualOnPush
if ((lView[FLAGS] & LViewFlags.ManualOnPush) === 0) {
markViewDirty(startView);
}
const result = listenerFn(e);
if (wrapWithPreventDefault && result === false) {
e.preventDefault();
// Necessary for legacy browsers that don't support preventDefault (e.g. IE)
e.returnValue = false;
}
return result;
};
}
/**
* Marks current view and all ancestors dirty.
*
@ -2600,12 +2625,15 @@ function wrapListenerWithPreventDefault(listenerFn: (e?: any) => any): EventList
* @param lView The starting LView to mark dirty
* @returns the root LView
*/
export function markViewDirty(lView: LView): LView {
export function markViewDirty(lView: LView): LView|null {
while (lView && !(lView[FLAGS] & LViewFlags.IsRoot)) {
lView[FLAGS] |= LViewFlags.Dirty;
lView = lView[PARENT] !;
}
lView[FLAGS] |= LViewFlags.Dirty;
// Detached views do not have a PARENT and also aren't root views
if (lView) {
lView[FLAGS] |= LViewFlags.Dirty;
}
return lView;
}
@ -2791,7 +2819,7 @@ function executeViewQueryFn<T>(lView: LView, tView: TView, component: T): void {
*/
export function markDirty<T>(component: T) {
ngDevMode && assertDefined(component, 'component');
const rootView = markViewDirty(getComponentViewByInstance(component));
const rootView = markViewDirty(getComponentViewByInstance(component)) !;
ngDevMode && assertDefined(rootView[CONTEXT], 'rootContext should be defined');
scheduleTick(rootView[CONTEXT] as RootContext, RootContextFlags.DetectChanges);

View File

@ -241,24 +241,41 @@ export const enum LViewFlags {
/** Whether this view has default change detection strategy (checks always) or onPush */
CheckAlways = 0b00000010000,
/**
* Whether or not manual change detection is turned on for onPush components.
*
* This is a special mode that only marks components dirty in two cases:
* 1) There has been a change to an @Input property
* 2) `markDirty()` has been called manually by the user
*
* Note that in this mode, the firing of events does NOT mark components
* dirty automatically.
*
* Manual mode is turned off by default for backwards compatibility, as events
* automatically mark OnPush components dirty in View Engine.
*
* TODO: Add a public API to ChangeDetectionStrategy to turn this mode on
*/
ManualOnPush = 0b00000100000,
/** Whether or not this view is currently dirty (needing check) */
Dirty = 0b00000100000,
Dirty = 0b000001000000,
/** Whether or not this view is currently attached to change detection tree. */
Attached = 0b00001000000,
Attached = 0b000010000000,
/** Whether or not this view is destroyed. */
Destroyed = 0b00010000000,
Destroyed = 0b000100000000,
/** Whether or not this view is the root view */
IsRoot = 0b00100000000,
IsRoot = 0b001000000000,
/**
* Index of the current init phase on last 23 bits
* Index of the current init phase on last 22 bits
*/
IndexWithinInitPhaseIncrementer = 0b01000000000,
IndexWithinInitPhaseShift = 9,
IndexWithinInitPhaseReset = 0b00111111111,
IndexWithinInitPhaseIncrementer = 0b010000000000,
IndexWithinInitPhaseShift = 10,
IndexWithinInitPhaseReset = 0b001111111111,
}
/**

View File

@ -1239,6 +1239,6 @@
"name": "walkUpViews"
},
{
"name": "wrapListenerWithPreventDefault"
"name": "wrapListener"
}
]

View File

@ -637,44 +637,43 @@ function declareTests(config?: {useJit: boolean}) {
}));
}
fixmeIvy('FW-758: OnPush events not marking view dirty when using renderer2')
.it('should be checked when an event is fired', () => {
TestBed.configureTestingModule(
{declarations: [MyComp, PushCmp, EventCmp], imports: [CommonModule]});
const template = '<push-cmp [prop]="ctxProp" #cmp></push-cmp>';
TestBed.overrideComponent(MyComp, {set: {template}});
const fixture = TestBed.createComponent(MyComp);
it('should be checked when an event is fired', () => {
TestBed.configureTestingModule(
{declarations: [MyComp, PushCmp, EventCmp], imports: [CommonModule]});
const template = '<push-cmp [prop]="ctxProp" #cmp></push-cmp>';
TestBed.overrideComponent(MyComp, {set: {template}});
const fixture = TestBed.createComponent(MyComp);
const cmpEl = fixture.debugElement.children[0];
const cmp = cmpEl.componentInstance;
fixture.detectChanges();
fixture.detectChanges();
expect(cmp.numberOfChecks).toEqual(1);
const cmpEl = fixture.debugElement.children[0];
const cmp = cmpEl.componentInstance;
fixture.detectChanges();
fixture.detectChanges();
expect(cmp.numberOfChecks).toEqual(1);
// regular element
cmpEl.children[0].triggerEventHandler('click', <Event>{});
fixture.detectChanges();
fixture.detectChanges();
expect(cmp.numberOfChecks).toEqual(2);
// regular element
cmpEl.children[0].triggerEventHandler('click', <Event>{});
fixture.detectChanges();
fixture.detectChanges();
expect(cmp.numberOfChecks).toEqual(2);
// element inside of an *ngIf
cmpEl.children[1].triggerEventHandler('click', <Event>{});
fixture.detectChanges();
fixture.detectChanges();
expect(cmp.numberOfChecks).toEqual(3);
// element inside of an *ngIf
cmpEl.children[1].triggerEventHandler('click', <Event>{});
fixture.detectChanges();
fixture.detectChanges();
expect(cmp.numberOfChecks).toEqual(3);
// element inside a nested component
cmpEl.children[2].children[0].triggerEventHandler('click', <Event>{});
fixture.detectChanges();
fixture.detectChanges();
expect(cmp.numberOfChecks).toEqual(4);
// element inside a nested component
cmpEl.children[2].children[0].triggerEventHandler('click', <Event>{});
fixture.detectChanges();
fixture.detectChanges();
expect(cmp.numberOfChecks).toEqual(4);
// host element
cmpEl.triggerEventHandler('click', <Event>{});
fixture.detectChanges();
fixture.detectChanges();
expect(cmp.numberOfChecks).toEqual(5);
});
// host element
cmpEl.triggerEventHandler('click', <Event>{});
fixture.detectChanges();
fixture.detectChanges();
expect(cmp.numberOfChecks).toEqual(5);
});
it('should not affect updating properties on the component', () => {
TestBed.configureTestingModule({declarations: [MyComp, [[PushCmpWithRef]]]});

View File

@ -11,11 +11,12 @@ import {withBody} from '@angular/private/testing';
import {ChangeDetectionStrategy, ChangeDetectorRef, DoCheck, RendererType2} from '../../src/core';
import {whenRendered} from '../../src/render3/component';
import {LifecycleHooksFeature, NgOnChangesFeature, defineComponent, defineDirective, getRenderedText, templateRefExtractor} from '../../src/render3/index';
import {LifecycleHooksFeature, NgOnChangesFeature, defineComponent, defineDirective, getCurrentView, getRenderedText, templateRefExtractor} from '../../src/render3/index';
import {bind, container, containerRefreshEnd, containerRefreshStart, detectChanges, directiveInject, element, elementEnd, elementProperty, elementStart, embeddedViewEnd, embeddedViewStart, interpolation1, interpolation2, listener, markDirty, reference, text, template, textBinding, tick} from '../../src/render3/instructions';
import {RenderFlags} from '../../src/render3/interfaces/definition';
import {RElement, Renderer3, RendererFactory3} from '../../src/render3/interfaces/renderer';
import {FLAGS, LViewFlags} from '../../src/render3/interfaces/view';
import {ComponentFixture, containerEl, createComponent, renderComponent, requestAnimationFrame} from './render_util';
@ -183,41 +184,39 @@ describe('change detection', () => {
myApp.name = 'Bess';
tick(myApp);
expect(comp.doCheckCount).toEqual(2);
// View should update, as changed input marks view dirty
expect(getRenderedText(myApp)).toEqual('2 - Bess');
myApp.name = 'George';
tick(myApp);
// View should update, as changed input marks view dirty
expect(comp.doCheckCount).toEqual(3);
expect(getRenderedText(myApp)).toEqual('3 - George');
tick(myApp);
expect(comp.doCheckCount).toEqual(4);
// View should not be updated to "4", as inputs have not changed.
expect(getRenderedText(myApp)).toEqual('3 - George');
});
it('should not check OnPush components in update mode when component events occur, unless marked dirty',
() => {
const myApp = renderComponent(MyApp);
expect(comp.doCheckCount).toEqual(1);
expect(getRenderedText(myApp)).toEqual('1 - Nancy');
it('should check OnPush components in update mode when component events occur', () => {
const myApp = renderComponent(MyApp);
expect(comp.doCheckCount).toEqual(1);
expect(getRenderedText(myApp)).toEqual('1 - Nancy');
const button = containerEl.querySelector('button') !;
button.click();
requestAnimationFrame.flush();
// No ticks should have been scheduled.
expect(comp.doCheckCount).toEqual(1);
expect(getRenderedText(myApp)).toEqual('1 - Nancy');
const button = containerEl.querySelector('button') !;
button.click();
requestAnimationFrame.flush();
// No ticks should have been scheduled.
expect(comp.doCheckCount).toEqual(1);
expect(getRenderedText(myApp)).toEqual('1 - Nancy');
tick(myApp);
// The comp should still be clean. So doCheck will run, but the view should display 1.
expect(comp.doCheckCount).toEqual(2);
expect(getRenderedText(myApp)).toEqual('1 - Nancy');
markDirty(comp);
requestAnimationFrame.flush();
// Now that markDirty has been manually called, the view should be dirty and a tick
// should be scheduled to check the view.
expect(comp.doCheckCount).toEqual(3);
expect(getRenderedText(myApp)).toEqual('3 - Nancy');
});
tick(myApp);
// Because the onPush comp should be dirty, it should update once CD runs
expect(comp.doCheckCount).toEqual(2);
expect(getRenderedText(myApp)).toEqual('2 - Nancy');
});
it('should not check OnPush components in update mode when parent events occur', () => {
function noop() {}
@ -238,76 +237,230 @@ describe('change detection', () => {
(button as HTMLButtonElement).click();
tick(buttonParent);
// The comp should still be clean. So doCheck will run, but the view should display 1.
expect(comp.doCheckCount).toEqual(2);
expect(getRenderedText(buttonParent)).toEqual('1 - Nancy');
});
it('should not check parent OnPush components in update mode when child events occur, unless marked dirty',
() => {
let parent: ButtonParent;
it('should check parent OnPush components in update mode when child events occur', () => {
let parent: ButtonParent;
class ButtonParent implements DoCheck {
doCheckCount = 0;
ngDoCheck(): void { this.doCheckCount++; }
class ButtonParent implements DoCheck {
doCheckCount = 0;
ngDoCheck(): void { this.doCheckCount++; }
static ngComponentDef = defineComponent({
type: ButtonParent,
selectors: [['button-parent']],
factory: () => parent = new ButtonParent(),
consts: 2,
vars: 1,
/** {{ doCheckCount }} - <my-comp></my-comp> */
template: (rf: RenderFlags, ctx: ButtonParent) => {
if (rf & RenderFlags.Create) {
text(0);
element(1, 'my-comp');
}
if (rf & RenderFlags.Update) {
textBinding(0, interpolation1('', ctx.doCheckCount, ' - '));
}
},
directives: () => [MyComponent],
changeDetection: ChangeDetectionStrategy.OnPush
});
}
static ngComponentDef = defineComponent({
type: ButtonParent,
selectors: [['button-parent']],
factory: () => parent = new ButtonParent(),
consts: 2,
vars: 1,
/** {{ doCheckCount }} - <my-comp></my-comp> */
template: (rf: RenderFlags, ctx: ButtonParent) => {
if (rf & RenderFlags.Create) {
text(0);
element(1, 'my-comp');
}
if (rf & RenderFlags.Update) {
textBinding(0, interpolation1('', ctx.doCheckCount, ' - '));
}
},
directives: () => [MyComponent],
changeDetection: ChangeDetectionStrategy.OnPush
});
}
const MyButtonApp = createComponent('my-button-app', function(rf: RenderFlags, ctx: any) {
if (rf & RenderFlags.Create) {
element(0, 'button-parent');
const MyButtonApp = createComponent('my-button-app', function(rf: RenderFlags, ctx: any) {
if (rf & RenderFlags.Create) {
element(0, 'button-parent');
}
}, 1, 0, [ButtonParent]);
const myButtonApp = renderComponent(MyButtonApp);
expect(parent !.doCheckCount).toEqual(1);
expect(comp !.doCheckCount).toEqual(1);
expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy');
tick(myButtonApp);
expect(parent !.doCheckCount).toEqual(2);
// parent isn't checked, so child doCheck won't run
expect(comp !.doCheckCount).toEqual(1);
expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy');
const button = containerEl.querySelector('button');
button !.click();
requestAnimationFrame.flush();
// No ticks should have been scheduled.
expect(parent !.doCheckCount).toEqual(2);
expect(comp !.doCheckCount).toEqual(1);
tick(myButtonApp);
expect(parent !.doCheckCount).toEqual(3);
expect(comp !.doCheckCount).toEqual(2);
expect(getRenderedText(myButtonApp)).toEqual('3 - 2 - Nancy');
});
describe('Manual mode', () => {
class ManualComponent implements DoCheck {
/* @Input() */
name = 'Nancy';
doCheckCount = 0;
ngDoCheck(): void { this.doCheckCount++; }
onClick() {}
static ngComponentDef = defineComponent({
type: ManualComponent,
selectors: [['manual-comp']],
factory: () => comp = new ManualComponent(),
consts: 2,
vars: 2,
/**
* {{ doCheckCount }} - {{ name }}
* <button (click)="onClick()"></button>
*/
template: (rf: RenderFlags, ctx: ManualComponent) => {
if (rf & RenderFlags.Create) {
// This is temporarily the only way to turn on manual change detection
// because public API has not yet been added.
const view = getCurrentView() as any;
view[FLAGS] |= LViewFlags.ManualOnPush;
text(0);
elementStart(1, 'button');
{
listener('click', () => { ctx.onClick(); });
}
elementEnd();
}
if (rf & RenderFlags.Update) {
textBinding(0, interpolation2('', ctx.doCheckCount, ' - ', ctx.name, ''));
}
},
changeDetection: ChangeDetectionStrategy.OnPush,
inputs: {name: 'name'}
});
}
class ManualApp {
name: string = 'Nancy';
static ngComponentDef = defineComponent({
type: ManualApp,
selectors: [['manual-app']],
factory: () => new ManualApp(),
consts: 1,
vars: 1,
/** <manual-comp [name]="name"></manual-comp> */
template: (rf: RenderFlags, ctx: ManualApp) => {
if (rf & RenderFlags.Create) {
element(0, 'manual-comp');
}
if (rf & RenderFlags.Update) {
elementProperty(0, 'name', bind(ctx.name));
}
},
directives: () => [ManualComponent]
});
}
it('should not check OnPush components in update mode when component events occur, unless marked dirty',
() => {
const myApp = renderComponent(ManualApp);
expect(comp.doCheckCount).toEqual(1);
expect(getRenderedText(myApp)).toEqual('1 - Nancy');
const button = containerEl.querySelector('button') !;
button.click();
requestAnimationFrame.flush();
// No ticks should have been scheduled.
expect(comp.doCheckCount).toEqual(1);
expect(getRenderedText(myApp)).toEqual('1 - Nancy');
tick(myApp);
// The comp should still be clean. So doCheck will run, but the view should display 1.
expect(comp.doCheckCount).toEqual(2);
expect(getRenderedText(myApp)).toEqual('1 - Nancy');
markDirty(comp);
requestAnimationFrame.flush();
// Now that markDirty has been manually called, the view should be dirty and a tick
// should be scheduled to check the view.
expect(comp.doCheckCount).toEqual(3);
expect(getRenderedText(myApp)).toEqual('3 - Nancy');
});
it('should not check parent OnPush components in update mode when child events occur, unless marked dirty',
() => {
let parent: ButtonParent;
class ButtonParent implements DoCheck {
doCheckCount = 0;
ngDoCheck(): void { this.doCheckCount++; }
static ngComponentDef = defineComponent({
type: ButtonParent,
selectors: [['button-parent']],
factory: () => parent = new ButtonParent(),
consts: 2,
vars: 1,
/** {{ doCheckCount }} - <manual-comp></manual-comp> */
template: (rf: RenderFlags, ctx: ButtonParent) => {
if (rf & RenderFlags.Create) {
text(0);
element(1, 'manual-comp');
}
if (rf & RenderFlags.Update) {
textBinding(0, interpolation1('', ctx.doCheckCount, ' - '));
}
},
directives: () => [ManualComponent],
changeDetection: ChangeDetectionStrategy.OnPush
});
}
}, 1, 0, [ButtonParent]);
const myButtonApp = renderComponent(MyButtonApp);
expect(parent !.doCheckCount).toEqual(1);
expect(comp !.doCheckCount).toEqual(1);
expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy');
const MyButtonApp =
createComponent('my-button-app', function(rf: RenderFlags, ctx: any) {
if (rf & RenderFlags.Create) {
element(0, 'button-parent');
}
}, 1, 0, [ButtonParent]);
tick(myButtonApp);
expect(parent !.doCheckCount).toEqual(2);
// parent isn't checked, so child doCheck won't run
expect(comp !.doCheckCount).toEqual(1);
expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy');
const myButtonApp = renderComponent(MyButtonApp);
expect(parent !.doCheckCount).toEqual(1);
expect(comp !.doCheckCount).toEqual(1);
expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy');
const button = containerEl.querySelector('button');
button !.click();
requestAnimationFrame.flush();
// No ticks should have been scheduled.
expect(parent !.doCheckCount).toEqual(2);
expect(comp !.doCheckCount).toEqual(1);
tick(myButtonApp);
expect(parent !.doCheckCount).toEqual(2);
// parent isn't checked, so child doCheck won't run
expect(comp !.doCheckCount).toEqual(1);
expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy');
tick(myButtonApp);
expect(parent !.doCheckCount).toEqual(3);
// parent isn't checked, so child doCheck won't run
expect(comp !.doCheckCount).toEqual(1);
expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy');
const button = containerEl.querySelector('button');
button !.click();
requestAnimationFrame.flush();
// No ticks should have been scheduled.
expect(parent !.doCheckCount).toEqual(2);
expect(comp !.doCheckCount).toEqual(1);
markDirty(comp);
requestAnimationFrame.flush();
// Now that markDirty has been manually called, both views should be dirty and a tick
// should be scheduled to check the view.
expect(parent !.doCheckCount).toEqual(4);
expect(comp !.doCheckCount).toEqual(2);
expect(getRenderedText(myButtonApp)).toEqual('4 - 2 - Nancy');
});
tick(myButtonApp);
expect(parent !.doCheckCount).toEqual(3);
// parent isn't checked, so child doCheck won't run
expect(comp !.doCheckCount).toEqual(1);
expect(getRenderedText(myButtonApp)).toEqual('1 - 1 - Nancy');
markDirty(comp);
requestAnimationFrame.flush();
// Now that markDirty has been manually called, both views should be dirty and a tick
// should be scheduled to check the view.
expect(parent !.doCheckCount).toEqual(4);
expect(comp !.doCheckCount).toEqual(2);
expect(getRenderedText(myButtonApp)).toEqual('4 - 2 - Nancy');
});
});
});
describe('ChangeDetectorRef', () => {