From 452668b581d3bf9e0b72f4a16da12ec9dabe4b67 Mon Sep 17 00:00:00 2001 From: Kara Erickson Date: Mon, 10 Dec 2018 14:51:28 -0800 Subject: [PATCH] fix(ivy): host bindings should work if input has same name (#27589) Previously in Ivy, host bindings did not work if they shared a public name with an Input because they used the `elementProperty` instruction as is. This instruction was originally built for inside component templates, so it would either set a directive input OR a native property. This is the correct behavior for inside a template, but for host bindings, we always want the native properties to be set regardless of the presence of an Input. This change adds an extra argument to `elementProperty` so we can tell it to ignore directive inputs and only set native properties (if it is in the context of a host binding). PR Close #27589 --- .../r3_view_compiler_binding_spec.ts | 4 +- .../r3_view_compiler_styling_spec.ts | 8 +-- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 2 +- .../compiler/src/render3/view/compiler.ts | 24 ++++--- packages/core/src/render3/instructions.ts | 12 ++-- packages/core/test/linker/integration_spec.ts | 22 +++---- .../core/test/render3/host_binding_spec.ts | 64 +++++++++++++++++++ 7 files changed, 103 insertions(+), 33 deletions(-) diff --git a/packages/compiler-cli/test/compliance/r3_view_compiler_binding_spec.ts b/packages/compiler-cli/test/compliance/r3_view_compiler_binding_spec.ts index 450a95e8c1..e4d5da4bbb 100644 --- a/packages/compiler-cli/test/compliance/r3_view_compiler_binding_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_view_compiler_binding_spec.ts @@ -150,7 +150,7 @@ describe('compiler compliance: bindings', () => { $r3$.ɵallocHostVars(1); } if (rf & 2) { - $r3$.ɵelementProperty(elIndex, "id", $r3$.ɵbind(ctx.dirId)); + $r3$.ɵelementProperty(elIndex, "id", $r3$.ɵbind(ctx.dirId), null, true); } } }); @@ -197,7 +197,7 @@ describe('compiler compliance: bindings', () => { $r3$.ɵallocHostVars(3); } if (rf & 2) { - $r3$.ɵelementProperty(elIndex, "id", $r3$.ɵbind($r3$.ɵpureFunction1(1, $ff$, ctx.id))); + $r3$.ɵelementProperty(elIndex, "id", $r3$.ɵbind($r3$.ɵpureFunction1(1, $ff$, ctx.id)), null, true); } }, consts: 0, diff --git a/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts b/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts index 71d418141d..fd59463428 100644 --- a/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts @@ -1047,8 +1047,8 @@ describe('compiler compliance: styling', () => { $r3$.ɵelementStyling($_c0$, $_c1$, $r3$.ɵdefaultStyleSanitizer, ctx); } if (rf & 2) { - $r3$.ɵelementProperty(elIndex, "id", $r3$.ɵbind(ctx.id)); - $r3$.ɵelementProperty(elIndex, "title", $r3$.ɵbind(ctx.title)); + $r3$.ɵelementProperty(elIndex, "id", $r3$.ɵbind(ctx.id), null, true); + $r3$.ɵelementProperty(elIndex, "title", $r3$.ɵbind(ctx.title), null, true); $r3$.ɵelementStylingMap(elIndex, ctx.myClass, ctx.myStyle, ctx); $r3$.ɵelementStylingApply(elIndex, ctx); } @@ -1095,8 +1095,8 @@ describe('compiler compliance: styling', () => { $r3$.ɵelementStyling($_c0$, $_c1$, null, ctx); } if (rf & 2) { - $r3$.ɵelementProperty(elIndex, "id", $r3$.ɵbind(ctx.id)); - $r3$.ɵelementProperty(elIndex, "title", $r3$.ɵbind(ctx.title)); + $r3$.ɵelementProperty(elIndex, "id", $r3$.ɵbind(ctx.id), null, true); + $r3$.ɵelementProperty(elIndex, "title", $r3$.ɵbind(ctx.title), null, true); $r3$.ɵelementStyleProp(elIndex, 0, ctx.myWidth, null, ctx); $r3$.ɵelementClassProp(elIndex, 0, ctx.myFooClass, ctx); $r3$.ɵelementStylingApply(elIndex, ctx); diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 94b1d6df9b..b7e918f58f 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -576,7 +576,7 @@ describe('ngtsc behavioral tests', () => { } if (rf & 2) { i0.ɵelementAttribute(elIndex, "hello", i0.ɵbind(ctx.foo)); - i0.ɵelementProperty(elIndex, "prop", i0.ɵbind(ctx.bar)); + i0.ɵelementProperty(elIndex, "prop", i0.ɵbind(ctx.bar), null, true); i0.ɵelementClassProp(elIndex, 0, ctx.someClass, ctx); i0.ɵelementStylingApply(elIndex, ctx); } diff --git a/packages/compiler/src/render3/view/compiler.ts b/packages/compiler/src/render3/view/compiler.ts index a7f8ef60ae..95e3065ac2 100644 --- a/packages/compiler/src/render3/view/compiler.ts +++ b/packages/compiler/src/render3/view/compiler.ts @@ -691,16 +691,15 @@ function createHostBindingsFunction( const value = binding.expression.visit(valueConverter); const bindingExpr = bindingFn(bindingContext, value); - const {bindingName, instruction} = getBindingNameAndInstruction(name); + const {bindingName, instruction, extraParams} = getBindingNameAndInstruction(name); + + const instructionParams: o.Expression[] = [ + elVarExp, o.literal(bindingName), o.importExpr(R3.bind).callFn([bindingExpr.currValExpr]) + ]; updateStatements.push(...bindingExpr.stmts); - updateStatements.push(o.importExpr(instruction) - .callFn([ - elVarExp, - o.literal(bindingName), - o.importExpr(R3.bind).callFn([bindingExpr.currValExpr]), - ]) - .toStmt()); + updateStatements.push( + o.importExpr(instruction).callFn(instructionParams.concat(extraParams)).toStmt()); } } @@ -752,8 +751,9 @@ function createStylingStmt( } function getBindingNameAndInstruction(bindingName: string): - {bindingName: string, instruction: o.ExternalReference} { + {bindingName: string, instruction: o.ExternalReference, extraParams: o.Expression[]} { let instruction !: o.ExternalReference; + const extraParams: o.Expression[] = []; // Check to see if this is an attr binding or a property binding const attrMatches = bindingName.match(ATTR_REGEX); @@ -762,9 +762,13 @@ function getBindingNameAndInstruction(bindingName: string): instruction = R3.elementAttribute; } else { instruction = R3.elementProperty; + extraParams.push( + o.literal(null), // TODO: This should be a sanitizer fn (FW-785) + o.literal(true) // host bindings must have nativeOnly prop set to true + ); } - return {bindingName, instruction}; + return {bindingName, instruction, extraParams}; } function createHostListeners( diff --git a/packages/core/src/render3/instructions.ts b/packages/core/src/render3/instructions.ts index d2fc45b4eb..b5466a9467 100644 --- a/packages/core/src/render3/instructions.ts +++ b/packages/core/src/render3/instructions.ts @@ -938,7 +938,7 @@ export function elementAttribute( } /** - * Update a property on an Element. + * Update a property on an element. * * If the property name also exists as an input property on one of the element's directives, * the component property will be set instead of the element property. This check must @@ -949,17 +949,21 @@ export function elementAttribute( * renaming as part of minification. * @param value New value to write. * @param sanitizer An optional function used to sanitize the value. + * @param nativeOnly Whether or not we should only set native properties and skip input check + * (this is necessary for host property bindings) */ export function elementProperty( - index: number, propName: string, value: T | NO_CHANGE, sanitizer?: SanitizerFn | null): void { + index: number, propName: string, value: T | NO_CHANGE, sanitizer?: SanitizerFn | null, + nativeOnly?: boolean): void { if (value === NO_CHANGE) return; const lView = getLView(); const element = getNativeByIndex(index, lView) as RElement | RComment; const tNode = getTNode(index, lView); - const inputData = initializeTNodeInputs(tNode); + let inputData: PropertyAliases|null|undefined; let dataValue: PropertyAliasValue|undefined; - if (inputData && (dataValue = inputData[propName])) { + if (!nativeOnly && (inputData = initializeTNodeInputs(tNode)) && + (dataValue = inputData[propName])) { setInputsForProperty(lView, dataValue, value); if (isComponent(tNode)) markDirtyIfOnPush(lView, index + HEADER_OFFSET); if (ngDevMode) { diff --git a/packages/core/test/linker/integration_spec.ts b/packages/core/test/linker/integration_spec.ts index 079bb2132b..c0afbf6e7c 100644 --- a/packages/core/test/linker/integration_spec.ts +++ b/packages/core/test/linker/integration_spec.ts @@ -1671,20 +1671,18 @@ function declareTests(config?: {useJit: boolean}) { expect(el.title).toBeFalsy(); }); - fixmeIvy('FW-711: elementProperty instruction should not be used in host bindings') - .it('should work when a directive uses hostProperty to update the DOM element', () => { - TestBed.configureTestingModule( - {declarations: [MyComp, DirectiveWithTitleAndHostProperty]}); - const template = ''; - TestBed.overrideComponent(MyComp, {set: {template}}); - const fixture = TestBed.createComponent(MyComp); + it('should work when a directive uses hostProperty to update the DOM element', () => { + TestBed.configureTestingModule({declarations: [MyComp, DirectiveWithTitleAndHostProperty]}); + const template = ''; + TestBed.overrideComponent(MyComp, {set: {template}}); + const fixture = TestBed.createComponent(MyComp); - fixture.componentInstance.ctxProp = 'TITLE'; - fixture.detectChanges(); + fixture.componentInstance.ctxProp = 'TITLE'; + fixture.detectChanges(); - const el = getDOM().querySelector(fixture.nativeElement, 'span'); - expect(getDOM().getProperty(el, 'title')).toEqual('TITLE'); - }); + const el = getDOM().querySelector(fixture.nativeElement, 'span'); + expect(getDOM().getProperty(el, 'title')).toEqual('TITLE'); + }); }); describe('logging property updates', () => { diff --git a/packages/core/test/render3/host_binding_spec.ts b/packages/core/test/render3/host_binding_spec.ts index aa3ad8d636..792fd688a5 100644 --- a/packages/core/test/render3/host_binding_spec.ts +++ b/packages/core/test/render3/host_binding_spec.ts @@ -367,6 +367,70 @@ describe('host bindings', () => { expect(initHookComp.title).toEqual('input2-changes-init-check'); }); + it('should support host bindings with the same name as inputs', () => { + let hostBindingInputDir !: HostBindingInputDir; + + class HostBindingInputDir { + // @Input() + disabled = false; + + // @HostBinding('disabled') + hostDisabled = false; + + static ngDirectiveDef = defineDirective({ + type: HostBindingInputDir, + selectors: [['', 'hostBindingDir', '']], + factory: () => hostBindingInputDir = new HostBindingInputDir(), + hostBindings: (rf: RenderFlags, ctx: HostBindingInputDir, elIndex: number) => { + if (rf & RenderFlags.Create) { + allocHostVars(1); + } + if (rf & RenderFlags.Update) { + elementProperty(elIndex, 'disabled', bind(ctx.hostDisabled), null, true); + } + }, + inputs: {disabled: 'disabled'} + }); + } + + /** */ + class App { + isDisabled = true; + + static ngComponentDef = defineComponent({ + type: App, + selectors: [['app']], + factory: () => new App(), + template: (rf: RenderFlags, ctx: App) => { + if (rf & RenderFlags.Create) { + element(0, 'input', ['hostBindingDir', '']); + } + if (rf & RenderFlags.Update) { + elementProperty(0, 'disabled', bind(ctx.isDisabled)); + } + }, + consts: 1, + vars: 1, + directives: [HostBindingInputDir] + }); + } + + const fixture = new ComponentFixture(App); + const hostBindingEl = fixture.hostElement.querySelector('input') as HTMLInputElement; + expect(hostBindingInputDir.disabled).toBe(true); + expect(hostBindingEl.disabled).toBe(false); + + fixture.component.isDisabled = false; + fixture.update(); + expect(hostBindingInputDir.disabled).toBe(false); + expect(hostBindingEl.disabled).toBe(false); + + hostBindingInputDir.hostDisabled = true; + fixture.update(); + expect(hostBindingInputDir.disabled).toBe(false); + expect(hostBindingEl.disabled).toBe(true); + }); + it('should support host bindings on second template pass', () => { /**
*/ const Parent = createComponent('parent', (rf: RenderFlags, ctx: any) => {