Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ivy): shadow all DOM properties in `DebugElement.properties` #33781

Closed
wants to merge 1 commit into from

Conversation

@mhevery
Copy link
Member

mhevery commented Nov 13, 2019

Fixes #33695

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@mhevery mhevery requested a review from angular/fw-core as a code owner Nov 13, 2019
packages/core/src/debug/debug_node.ts Outdated Show resolved Hide resolved
@anupraaj

This comment has been minimized.

Copy link

anupraaj commented Nov 13, 2019

Copy link
Member

pkozlowski-opensource left a comment

I understand the direction and I think this is the right thing to do. But there are few questions / nit comments that we should resolve before merging.

Copy link
Contributor

kara left a comment

Can we add a more detailed commit message that explains what we are fixing and why?

expect(button.properties.title).not.toEqual('ShadowInput');
});

onlyInIvy('Ivy shadows native DOM properties').it('should reflect native properties', () => {

This comment has been minimized.

Copy link
@kara

kara Nov 14, 2019

Contributor

I'm puzzled by this onlyInIvy marker. Why are we changing the behavior so View Engine and Ivy don't align? I thought this was a compatibility bug fix 🤔 .

What's also confusing to me is if I understand #33695 correctly, the problem is that the checked property is being added directly on the element by the control value accessor directive and it's not being reflected in DebugElement.properties. But this test appears to be a different case where there is an attribute that we expect to be reflected as a property.

  1. It seems like we're missing a test that matches the original bug report
  2. Is the new test a consequence of the fix for the original bug that happens to be a breaking change?

I think I'm just missing some context. Would be good to have more info in the commit message.

This comment has been minimized.

Copy link
@mhevery

mhevery Nov 14, 2019

Author Member
  1. I have removed onlyInIvy
  2. This is the test, that properties would not show op if there was no binding.
  3. I have updated the commit message to provide more context.

This comment has been minimized.

Copy link
@mhevery

mhevery Nov 15, 2019

Author Member

fixed

@mhevery mhevery requested a review from angular/fw-forms as a code owner Nov 14, 2019
Copy link
Member Author

mhevery left a comment

Fixes in b1447d1

packages/core/src/debug/debug_node.ts Outdated Show resolved Hide resolved
packages/core/src/debug/debug_node.ts Outdated Show resolved Hide resolved
packages/core/test/debug/debug_node_spec.ts Outdated Show resolved Hide resolved
packages/core/test/debug/debug_node_spec.ts Outdated Show resolved Hide resolved
expect(button.properties.title).not.toEqual('ShadowInput');
});

onlyInIvy('Ivy shadows native DOM properties').it('should reflect native properties', () => {

This comment has been minimized.

Copy link
@mhevery

mhevery Nov 14, 2019

Author Member
  1. I have removed onlyInIvy
  2. This is the test, that properties would not show op if there was no binding.
  3. I have updated the commit message to provide more context.
packages/core/src/debug/debug_node.ts Outdated Show resolved Hide resolved
@mhevery

This comment has been minimized.

Copy link
Member Author

mhevery commented Nov 14, 2019

packages/forms/test/directives/ng_model.spec.ts Outdated Show resolved Hide resolved
const properties: {[key: string]: string} = {};
// Collect properties from the DOM.
copyDomProperties(this.nativeElement, properties);
// Collect properties from the bindings. (For bindings to component/directives these would not

This comment has been minimized.

Copy link
@kara

kara Nov 14, 2019

Contributor

My understanding was that we don't include directive inputs in the properties list. Or are you talking about something else with "bindings to component/directives"?

This comment has been minimized.

Copy link
@mhevery

mhevery Nov 14, 2019

Author Member

Ohh, you are right, I was totally misunderstanding the purpose of that call. Removed.

Copy link
Member Author

mhevery left a comment

Fixes in a4c55f5

const properties: {[key: string]: string} = {};
// Collect properties from the DOM.
copyDomProperties(this.nativeElement, properties);
// Collect properties from the bindings. (For bindings to component/directives these would not

This comment has been minimized.

Copy link
@mhevery

mhevery Nov 14, 2019

Author Member

Ohh, you are right, I was totally misunderstanding the purpose of that call. Removed.

packages/forms/test/directives/ng_model.spec.ts Outdated Show resolved Hide resolved
@@ -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');

This comment has been minimized.

Copy link
@pkozlowski-opensource

pkozlowski-opensource Nov 15, 2019

Member

Is this change necessary? It is VE specific and shouldn't impact ivy, right?

This comment has been minimized.

Copy link
@mhevery

mhevery Nov 15, 2019

Author Member

Yes this is needed. unknownProp is not defined and so VE returns undefined however Ivy returns element.title which defaults to "" so it fails.

Copy link
Member

pkozlowski-opensource left a comment

LGTM with 2 minor nits:

@mhevery

This comment has been minimized.

Copy link
Member Author

mhevery commented Nov 15, 2019

@pkozlowski-opensource

This comment has been minimized.

Copy link
Member

pkozlowski-opensource commented Nov 15, 2019

LGTM, thnx for all the clean-ups @mhevery

@mhevery mhevery force-pushed the mhevery:issue_33695 branch from daed916 to f244c7d Nov 16, 2019
@kara
kara approved these changes Nov 18, 2019
Copy link
Contributor

kara left a comment

LGTM

@mhevery mhevery force-pushed the mhevery:issue_33695 branch from f3356c7 to d9d1966 Nov 18, 2019
@kara

This comment has been minimized.

Copy link
Contributor

kara commented Nov 18, 2019

merge-assistance: global approval

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.