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): better support for i18n attributes on <ng-container>s #33599

Conversation

@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Nov 5, 2019

Prior to this commit, i18n runtime logic used elementAttributeInternal function (that uses setAttribute function under the hood) for all elements where i18n attributes are present. However the <ng-container> elements in a template may also have i18n attributes and calling setAttribute fails, since they are represented as comment nodes in DOM. This commit ensures that we call setAttribute on nodes with TNodeType.Element type (that support that operation) only.

This PR resolves FW-1663.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No
Copy link
Contributor

ocombe left a comment

I'm pretty sure that setAttribute and setProperty are not the same, some properties have different names compared to the original attributes. If you try to set the property class for example, it'll not work as expected, since the property for the attribute class is className instead...
Instead you should add a check to see if the element is a container, and use elementPropertyInternal on it, otherwise you should still use elementAttributeInternal

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor Author

AndrewKushnir commented Nov 5, 2019

@ocombe thanks for the review, I've updated the logic to avoid calling elementAttributeInternal for non-elements. For non-elements (like <ng-container>) we just set the necessary inputs. Could you please have a look again? Thank you.

@ocombe
ocombe approved these changes Nov 5, 2019
@AndrewKushnir AndrewKushnir changed the title fix(ivy): better support for attributes on <ng-container>s fix(ivy): better support for i18n attributes on <ng-container>s Nov 5, 2019
@AndrewKushnir AndrewKushnir force-pushed the AndrewKushnir:FW-1663_i18n_attrs_on_ng_container branch 2 times, most recently from 25770e1 to 6dc861f Nov 5, 2019
@AndrewKushnir AndrewKushnir marked this pull request as ready for review Nov 5, 2019
@AndrewKushnir AndrewKushnir requested review from angular/fw-core as code owners Nov 5, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor Author

AndrewKushnir commented Nov 5, 2019

packages/core/test/acceptance/i18n_spec.ts Outdated Show resolved Hide resolved
Prior to this commit, i18n runtime logic used `elementAttributeInternal` function (that uses `setAttribute` function under the hood) for all elements where i18n attributes are present. However the `<ng-container>` elements in a template may also have i18n attributes and calling `setAttribute` fails, since they are represented as comment nodes in DOM. This commit ensures that we call `setAttribute` on nodes with TNodeType.Element type (that support that operation) only.
@AndrewKushnir AndrewKushnir force-pushed the AndrewKushnir:FW-1663_i18n_attrs_on_ng_container branch from f40815d to 4743c6c Nov 6, 2019
@mhevery
mhevery approved these changes Nov 6, 2019
@atscott atscott closed this in 3955074 Nov 7, 2019
atscott added a commit that referenced this pull request Nov 7, 2019
Prior to this commit, i18n runtime logic used `elementAttributeInternal` function (that uses `setAttribute` function under the hood) for all elements where i18n attributes are present. However the `<ng-container>` elements in a template may also have i18n attributes and calling `setAttribute` fails, since they are represented as comment nodes in DOM. This commit ensures that we call `setAttribute` on nodes with TNodeType.Element type (that support that operation) only.

PR Close #33599
mohaxspb added a commit to mohaxspb/angular that referenced this pull request Nov 7, 2019
…lar#33599)

Prior to this commit, i18n runtime logic used `elementAttributeInternal` function (that uses `setAttribute` function under the hood) for all elements where i18n attributes are present. However the `<ng-container>` elements in a template may also have i18n attributes and calling `setAttribute` fails, since they are represented as comment nodes in DOM. This commit ensures that we call `setAttribute` on nodes with TNodeType.Element type (that support that operation) only.

PR Close angular#33599
mohaxspb added a commit to mohaxspb/angular that referenced this pull request Nov 7, 2019
…lar#33599)

Prior to this commit, i18n runtime logic used `elementAttributeInternal` function (that uses `setAttribute` function under the hood) for all elements where i18n attributes are present. However the `<ng-container>` elements in a template may also have i18n attributes and calling `setAttribute` fails, since they are represented as comment nodes in DOM. This commit ensures that we call `setAttribute` on nodes with TNodeType.Element type (that support that operation) only.

PR Close angular#33599
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.