From a681c8553a1065ab7195c985a3234db0c1603a35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mi=C5=A1ko=20Hevery?= Date: Tue, 12 Nov 2019 17:12:36 -0800 Subject: [PATCH] fix(ivy): shadow all DOM properties in `DebugElement.properties` (#33781) Fixes #33695 PR Close #33781 --- packages/core/src/debug/debug_node.ts | 61 ++++---- packages/core/test/debug/debug_node_spec.ts | 131 ++++++++++++------ packages/core/test/linker/integration_spec.ts | 3 +- packages/core/test/test_bed_spec.ts | 7 +- .../forms/test/template_integration_spec.ts | 21 +++ 5 files changed, 151 insertions(+), 72 deletions(-) diff --git a/packages/core/src/debug/debug_node.ts b/packages/core/src/debug/debug_node.ts index 479d6ed4c9..d01dd664ea 100644 --- a/packages/core/src/debug/debug_node.ts +++ b/packages/core/src/debug/debug_node.ts @@ -278,14 +278,12 @@ class DebugElement__POST_R3__ extends DebugNode__POST_R3__ implements DebugEleme const tData = lView[TVIEW].data; const tNode = tData[context.nodeIndex] as TNode; - const properties = collectPropertyBindings(tNode, lView, tData); - const className = collectClassNames(this); - - if (className) { - properties['className'] = - properties['className'] ? properties['className'] + ` ${className}` : className; - } - + const properties: {[key: string]: string} = {}; + // Collect properties from the DOM. + copyDomProperties(this.nativeElement, properties); + // Collect properties from the bindings. This is needed for animation renderer which has + // synthetic properties which don't get reflected into the DOM. + collectPropertyBindings(properties, tNode, lView, tData); return properties; } @@ -446,6 +444,34 @@ class DebugElement__POST_R3__ extends DebugNode__POST_R3__ implements DebugEleme } } +function copyDomProperties(element: Element | null, properties: {[name: string]: string}): void { + if (element) { + // Skip own properties (as those are patched) + let obj = Object.getPrototypeOf(element); + const NodePrototype: any = Node.prototype; + while (obj !== null && obj !== NodePrototype) { + const descriptors = Object.getOwnPropertyDescriptors(obj); + for (let key in descriptors) { + if (!key.startsWith('__') && !key.startsWith('on')) { + // don't include properties starting with `__` and `on`. + // `__` are patched values which should not be included. + // `on` are listeners which also should not be included. + const value = (element as any)[key]; + if (isPrimitiveValue(value)) { + properties[key] = value; + } + } + } + obj = Object.getPrototypeOf(obj); + } + } +} + +function isPrimitiveValue(value: any): boolean { + return typeof value === 'string' || typeof value === 'boolean' || typeof value === 'number' || + value === null; +} + /** * Walk the TNode tree to find matches for the predicate. * @@ -649,8 +675,7 @@ function _queryNativeNodeDescendants( * defined in templates, not in host bindings. */ function collectPropertyBindings( - tNode: TNode, lView: LView, tData: TData): {[key: string]: string} { - const properties: {[key: string]: string} = {}; + properties: {[key: string]: string}, tNode: TNode, lView: LView, tData: TData): void { let bindingIndexes = tNode.propertyBindings; if (bindingIndexes !== null) { @@ -670,22 +695,6 @@ function collectPropertyBindings( } } } - - return properties; -} - - -function collectClassNames(debugElement: DebugElement__POST_R3__): string { - const classes = debugElement.classes; - let output = ''; - - for (const className of Object.keys(classes)) { - if (classes[className]) { - output = output ? output + ` ${className}` : className; - } - } - - return output; } diff --git a/packages/core/test/debug/debug_node_spec.ts b/packages/core/test/debug/debug_node_spec.ts index d70f4462f2..64bd8ef083 100644 --- a/packages/core/test/debug/debug_node_spec.ts +++ b/packages/core/test/debug/debug_node_spec.ts @@ -14,7 +14,7 @@ import {ComponentFixture, TestBed, async} from '@angular/core/testing'; import {By} from '@angular/platform-browser/src/dom/debug/by'; import {hasClass} from '@angular/platform-browser/testing/src/browser_util'; import {expect} from '@angular/platform-browser/testing/src/matchers'; -import {ivyEnabled} from '@angular/private/testing'; +import {ivyEnabled, onlyInIvy} from '@angular/private/testing'; @Injectable() class Logger { @@ -192,6 +192,11 @@ class TestApp { class TestCmpt { } +@Component({selector: 'test-cmpt-renderer', template: ``}) +class TestCmptWithRenderer { + constructor(public renderer: Renderer2) {} +} + @Component({selector: 'host-class-binding', template: ''}) class HostClassBindingCmp { @HostBinding('class') @@ -259,6 +264,7 @@ class TestCmptWithPropInterpolation { SimpleContentComp, TestCmptWithPropBindings, TestCmptWithPropInterpolation, + TestCmptWithRenderer, WithTitleDir, ], providers: [Logger], @@ -736,60 +742,101 @@ class TestCmptWithPropInterpolation { expect(fixture.componentInstance.customed).toBe(true); }); - it('should include classes in properties.className', () => { - fixture = TestBed.createComponent(HostClassBindingCmp); - fixture.detectChanges(); + describe('properties', () => { + it('should include classes in properties.className', () => { + fixture = TestBed.createComponent(HostClassBindingCmp); + fixture.detectChanges(); - const debugElement = fixture.debugElement; + const debugElement = fixture.debugElement; - expect(debugElement.properties.className).toBe('class-one class-two'); + expect(debugElement.properties.className).toBe('class-one class-two'); - fixture.componentInstance.hostClasses = 'class-three'; - fixture.detectChanges(); + fixture.componentInstance.hostClasses = 'class-three'; + fixture.detectChanges(); - expect(debugElement.properties.className).toBe('class-three'); + expect(debugElement.properties.className).toBe('class-three'); - fixture.componentInstance.hostClasses = ''; - fixture.detectChanges(); + fixture.componentInstance.hostClasses = ''; + fixture.detectChanges(); - expect(debugElement.properties.className).toBeFalsy(); - }); + expect(debugElement.properties.className).toBeFalsy(); + }); - it('should preserve the type of the property values', () => { - const fixture = TestBed.createComponent(TestCmptWithPropBindings); - fixture.detectChanges(); + it('should preserve the type of the property values', () => { + const fixture = TestBed.createComponent(TestCmptWithPropBindings); + fixture.detectChanges(); - const button = fixture.debugElement.query(By.css('button')); - expect(button.properties).toEqual({disabled: true, tabIndex: 1337, title: 'hello'}); - }); + const button = fixture.debugElement.query(By.css('button')); + expect(button.properties.disabled).toEqual(true); + expect(button.properties.tabIndex).toEqual(1337); + expect(button.properties.title).toEqual('hello'); + }); - it('should include interpolated properties in the properties map', () => { - const fixture = TestBed.createComponent(TestCmptWithPropInterpolation); - fixture.detectChanges(); + it('should include interpolated properties in the properties map', () => { + const fixture = TestBed.createComponent(TestCmptWithPropInterpolation); + fixture.detectChanges(); - const buttons = fixture.debugElement.children; + const buttons = fixture.debugElement.children; - expect(buttons.length).toBe(10); - expect(buttons[0].properties.title).toBe('0'); - expect(buttons[1].properties.title).toBe('a1b'); - expect(buttons[2].properties.title).toBe('a1b2c'); - expect(buttons[3].properties.title).toBe('a1b2c3d'); - expect(buttons[4].properties.title).toBe('a1b2c3d4e'); - expect(buttons[5].properties.title).toBe('a1b2c3d4e5f'); - expect(buttons[6].properties.title).toBe('a1b2c3d4e5f6g'); - expect(buttons[7].properties.title).toBe('a1b2c3d4e5f6g7h'); - expect(buttons[8].properties.title).toBe('a1b2c3d4e5f6g7h8i'); - expect(buttons[9].properties.title).toBe('a1b2c3d4e5f6g7h8i9j'); - }); + expect(buttons.length).toBe(10); + expect(buttons[0].properties.title).toBe('0'); + expect(buttons[1].properties.title).toBe('a1b'); + expect(buttons[2].properties.title).toBe('a1b2c'); + expect(buttons[3].properties.title).toBe('a1b2c3d'); + expect(buttons[4].properties.title).toBe('a1b2c3d4e'); + expect(buttons[5].properties.title).toBe('a1b2c3d4e5f'); + expect(buttons[6].properties.title).toBe('a1b2c3d4e5f6g'); + expect(buttons[7].properties.title).toBe('a1b2c3d4e5f6g7h'); + expect(buttons[8].properties.title).toBe('a1b2c3d4e5f6g7h8i'); + expect(buttons[9].properties.title).toBe('a1b2c3d4e5f6g7h8i9j'); + }); - it('should not include directive-shadowed properties in the properties map', () => { - TestBed.overrideTemplate( - TestCmptWithPropInterpolation, ``); - const fixture = TestBed.createComponent(TestCmptWithPropInterpolation); - fixture.detectChanges(); + it('should not include directive-shadowed properties in the properties map', () => { + TestBed.overrideTemplate( + TestCmptWithPropInterpolation, + ``); + const fixture = TestBed.createComponent(TestCmptWithPropInterpolation); + fixture.detectChanges(); - const button = fixture.debugElement.query(By.css('button')); - expect(button.properties.title).toBeUndefined(); + const button = fixture.debugElement.query(By.css('button')); + expect(button.properties.title).not.toEqual('goes to input'); + }); + + it('should return native properties from DOM element even if no binding present', () => { + TestBed.overrideTemplate(TestCmptWithRenderer, ``); + const fixture = TestBed.createComponent(TestCmptWithRenderer); + fixture.detectChanges(); + const button = fixture.debugElement.query(By.css('button')); + fixture.componentInstance.renderer.setProperty(button.nativeElement, 'title', 'myTitle'); + expect(button.properties.title).toBe('myTitle'); + }); + + it('should not include patched properties (starting with __) and on* properties', () => { + TestBed.overrideTemplate( + TestCmptWithPropInterpolation, ``); + const fixture = TestBed.createComponent(TestCmptWithPropInterpolation); + fixture.detectChanges(); + + const host = fixture.debugElement; + const button = fixture.debugElement.query(By.css('button')); + expect(Object.keys(host.properties).filter(key => key.startsWith('__'))).toEqual([]); + expect(Object.keys(host.properties).filter(key => key.startsWith('on'))).toEqual([]); + expect(Object.keys(button.properties).filter(key => key.startsWith('__'))).toEqual([]); + expect(Object.keys(button.properties).filter(key => key.startsWith('on'))).toEqual([]); + }); + + onlyInIvy('Show difference in behavior') + .it('should pickup all of the element properties', () => { + TestBed.overrideTemplate( + TestCmptWithPropInterpolation, ``); + const fixture = TestBed.createComponent(TestCmptWithPropInterpolation); + fixture.detectChanges(); + + const host = fixture.debugElement; + const button = fixture.debugElement.query(By.css('button')); + + expect(button.properties.title).toEqual('myTitle'); + }); }); it('should trigger events registered via Renderer2', () => { diff --git a/packages/core/test/linker/integration_spec.ts b/packages/core/test/linker/integration_spec.ts index f9cc8c610c..3522819021 100644 --- a/packages/core/test/linker/integration_spec.ts +++ b/packages/core/test/linker/integration_spec.ts @@ -921,6 +921,7 @@ function declareTests(config?: {useJit: boolean}) { @Directive({selector: '[host-properties]', host: {'[id]': 'id', '[title]': 'unknownProp'}}) class DirectiveWithHostProps { id = 'one'; + unknownProp = 'unknownProp'; } const fixture = @@ -933,7 +934,7 @@ function declareTests(config?: {useJit: boolean}) { const tc = fixture.debugElement.children[0]; expect(tc.properties['id']).toBe('one'); - expect(tc.properties['title']).toBe(undefined); + expect(tc.properties['title']).toBe('unknownProp'); }); it('should not allow pipes in hostProperties', () => { diff --git a/packages/core/test/test_bed_spec.ts b/packages/core/test/test_bed_spec.ts index a282deaed4..ca8cf78982 100644 --- a/packages/core/test/test_bed_spec.ts +++ b/packages/core/test/test_bed_spec.ts @@ -164,7 +164,8 @@ describe('TestBed', () => { fixture.detectChanges(); const divElement = fixture.debugElement.query(By.css('div')); - expect(divElement.properties).toEqual({id: 'one', title: 'some title'}); + expect(divElement.properties.id).toEqual('one'); + expect(divElement.properties.title).toEqual('some title'); }); it('should give the ability to access interpolated properties on a node', () => { @@ -172,8 +173,8 @@ describe('TestBed', () => { fixture.detectChanges(); const paragraphEl = fixture.debugElement.query(By.css('p')); - expect(paragraphEl.properties) - .toEqual({title: '( some label - some title )', id: '[ some label ] [ some title ]'}); + expect(paragraphEl.properties.title).toEqual('( some label - some title )'); + expect(paragraphEl.properties.id).toEqual('[ some label ] [ some title ]'); }); it('should give access to the node injector', () => { diff --git a/packages/forms/test/template_integration_spec.ts b/packages/forms/test/template_integration_spec.ts index dcece0fabf..a78ad61c26 100644 --- a/packages/forms/test/template_integration_spec.ts +++ b/packages/forms/test/template_integration_spec.ts @@ -57,6 +57,27 @@ import {NgModelCustomComp, NgModelCustomWrapper} from './value_accessor_integrat expect(form.valid).toBe(false); })); + it('should report properties which are written outside of template bindings', async() => { + // For example ngModel writes to `checked` property programmatically + // (template does not contain binding to `checked` explicitly) + // https://github.com/angular/angular/issues/33695 + @Component({ + selector: 'app-root', + template: `` + }) + class AppComponent { + active = 'one'; + } + TestBed.configureTestingModule({imports: [FormsModule], declarations: [AppComponent]}); + const fixture = TestBed.createComponent(AppComponent); + // We need the Await as `ngModel` writes data asynchronously into the DOM + await fixture.detectChanges(); + const input = fixture.debugElement.query(By.css('input')); + expect(input.properties.checked).toBe(true); + expect(input.nativeElement.checked).toBe(true); + }); + + it('should add novalidate by default to form element', fakeAsync(() => { const fixture = initTest(NgModelForm);