From 37c598db0401d5ebc556a5c7e6f668977795c31c Mon Sep 17 00:00:00 2001 From: crisbeto Date: Mon, 29 Apr 2019 20:27:32 +0200 Subject: [PATCH] fix(ivy): inherited listeners from grand super classes invoked multiple times (#30169) Fixes event listeners that come from more than one level of inheritance being invoked multiple times. This PR resolves FW-1294. PR Close #30169 --- .../features/inherit_definition_feature.ts | 51 ++++++------ .../core/test/acceptance/integration_spec.ts | 78 +++++++++++++++++-- 2 files changed, 99 insertions(+), 30 deletions(-) diff --git a/packages/core/src/render3/features/inherit_definition_feature.ts b/packages/core/src/render3/features/inherit_definition_feature.ts index f3d0508cfd..441af1e386 100644 --- a/packages/core/src/render3/features/inherit_definition_feature.ts +++ b/packages/core/src/render3/features/inherit_definition_feature.ts @@ -171,28 +171,33 @@ function inheritHostBindings( definition: DirectiveDef| ComponentDef, superHostBindings: HostBindingsFunction) { const prevHostBindings = definition.hostBindings; - if (prevHostBindings) { - // because inheritance is unknown during compile time, the runtime code - // needs to be informed of the super-class depth so that instruction code - // can distinguish one host bindings function from another. The reason why - // relying on the directive uniqueId exclusively is not enough is because the - // uniqueId value and the directive instance stay the same between hostBindings - // calls throughout the directive inheritance chain. This means that without - // a super-class depth value, there is no way to know whether a parent or - // sub-class host bindings function is currently being executed. - definition.hostBindings = (rf: RenderFlags, ctx: any, elementIndex: number) => { - // The reason why we increment first and then decrement is so that parent - // hostBindings calls have a higher id value compared to sub-class hostBindings - // calls (this way the leaf directive is always at a super-class depth of 0). - adjustActiveDirectiveSuperClassDepthPosition(1); - try { - superHostBindings(rf, ctx, elementIndex); - } finally { - adjustActiveDirectiveSuperClassDepthPosition(-1); - } - prevHostBindings(rf, ctx, elementIndex); - }; - } else { - definition.hostBindings = superHostBindings; + // If the subclass does not have a host bindings function, we set the subclass host binding + // function to be the superclass's (in this feature). We should check if they're the same here + // to ensure we don't inherit it twice. + if (superHostBindings !== prevHostBindings) { + if (prevHostBindings) { + // because inheritance is unknown during compile time, the runtime code + // needs to be informed of the super-class depth so that instruction code + // can distinguish one host bindings function from another. The reason why + // relying on the directive uniqueId exclusively is not enough is because the + // uniqueId value and the directive instance stay the same between hostBindings + // calls throughout the directive inheritance chain. This means that without + // a super-class depth value, there is no way to know whether a parent or + // sub-class host bindings function is currently being executed. + definition.hostBindings = (rf: RenderFlags, ctx: any, elementIndex: number) => { + // The reason why we increment first and then decrement is so that parent + // hostBindings calls have a higher id value compared to sub-class hostBindings + // calls (this way the leaf directive is always at a super-class depth of 0). + adjustActiveDirectiveSuperClassDepthPosition(1); + try { + superHostBindings(rf, ctx, elementIndex); + } finally { + adjustActiveDirectiveSuperClassDepthPosition(-1); + } + prevHostBindings(rf, ctx, elementIndex); + }; + } else { + definition.hostBindings = superHostBindings; + } } } diff --git a/packages/core/test/acceptance/integration_spec.ts b/packages/core/test/acceptance/integration_spec.ts index 64191a69a7..900a5a2bb2 100644 --- a/packages/core/test/acceptance/integration_spec.ts +++ b/packages/core/test/acceptance/integration_spec.ts @@ -253,27 +253,91 @@ describe('acceptance integration tests', () => { expect(clicks).toBe(1); }); - // TODO(crisbeto): this fails even with decorated classes - // in master. To be enabled as a part of FW-1294. - xit('should inherit host listeners from undecorated grand superclasses', () => { + it('should inherit host listeners from superclasses once', () => { let clicks = 0; + @Directive({selector: '[baseButton]'}) + class BaseButton { + @HostListener('click') + handleClick() { clicks++; } + } + + @Component({selector: '[subButton]', template: ''}) + class SubButton extends BaseButton { + } + + @Component({template: ''}) + class App { + } + + TestBed.configureTestingModule({declarations: [SubButton, BaseButton, App]}); + const fixture = TestBed.createComponent(App); + const button = fixture.debugElement.query(By.directive(SubButton)).nativeElement; + + button.click(); + fixture.detectChanges(); + + expect(clicks).toBe(1); + }); + + it('should inherit host listeners from grand superclasses once', () => { + let clicks = 0; + + @Directive({selector: '[superBaseButton]'}) class SuperBaseButton { @HostListener('click') handleClick() { clicks++; } } - class BaseButton extends SuperBaseButton {} + @Directive({selector: '[baseButton]'}) + class BaseButton extends SuperBaseButton { + } - @Component({selector: '[sub-button]', template: ''}) + @Component({selector: '[subButton]', template: ''}) class SubButton extends BaseButton { } - @Component({template: ''}) + @Component({template: ''}) class App { } - TestBed.configureTestingModule({declarations: [SubButton, App]}); + TestBed.configureTestingModule({declarations: [SubButton, SuperBaseButton, BaseButton, App]}); + const fixture = TestBed.createComponent(App); + const button = fixture.debugElement.query(By.directive(SubButton)).nativeElement; + + button.click(); + fixture.detectChanges(); + + expect(clicks).toBe(1); + }); + + it('should inherit host listeners from grand grand superclasses once', () => { + let clicks = 0; + + @Directive({selector: '[superSuperBaseButton]'}) + class SuperSuperBaseButton { + @HostListener('click') + handleClick() { clicks++; } + } + + @Directive({selector: '[superBaseButton]'}) + class SuperBaseButton extends SuperSuperBaseButton { + } + + @Directive({selector: '[baseButton]'}) + class BaseButton extends SuperBaseButton { + } + + @Component({selector: '[subButton]', template: ''}) + class SubButton extends BaseButton { + } + + @Component({template: ''}) + class App { + } + + TestBed.configureTestingModule( + {declarations: [SubButton, SuperBaseButton, SuperSuperBaseButton, BaseButton, App]}); const fixture = TestBed.createComponent(App); const button = fixture.debugElement.query(By.directive(SubButton)).nativeElement;