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): incorrectly validating html foreign objects inside svg #34178

Conversation

@crisbeto
Copy link
Member

crisbeto commented Dec 2, 2019

Fixes ngtsc incorrectly logging an unknown element diagnostic for HTML elements that are inside an SVG foreignObject with the xhtml namespace.

Fixes #34171.

@googlebot googlebot added the cla: yes label Dec 2, 2019
// HTML elements inside an SVG `foreignObject` are declared in the `xhtml` namespace.
// We need to strip it before handing it over to the registry because all HTML tag names
// in the registry are without a namespace.
const name = element.name.replace(REMOVE_XHTML_REGEX, '');

This comment has been minimized.

Copy link
@crisbeto

crisbeto Dec 2, 2019

Author Member

I went with this approach because it's how we're doing it in ViewEngine. An alternate solution could be to redeclare all the HTML tags with an xhtml namespace in the schema, but I don't know enough about what the implications would be to change the schema at this point.

@crisbeto crisbeto force-pushed the crisbeto:34171/svg-foreign-object-unknown-element branch from da2c068 to d64da9b Dec 2, 2019
@ngbot ngbot bot modified the milestone: needsTriage Dec 2, 2019
@crisbeto crisbeto marked this pull request as ready for review Dec 2, 2019
@crisbeto crisbeto requested a review from angular/fw-compiler as a code owner Dec 2, 2019
@crisbeto crisbeto force-pushed the crisbeto:34171/svg-foreign-object-unknown-element branch from d64da9b to e77be0c Dec 2, 2019
@crisbeto

This comment has been minimized.

Copy link
Member Author

crisbeto commented Dec 2, 2019

The CI failure will be fixed by #34179.

@mhevery
mhevery approved these changes Dec 2, 2019
@JoostK
JoostK approved these changes Dec 2, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Dec 2, 2019

@crisbeto does it make sense to add similar test for JIT?

@crisbeto crisbeto force-pushed the crisbeto:34171/svg-foreign-object-unknown-element branch from e77be0c to 4405bb8 Dec 2, 2019
@crisbeto crisbeto requested a review from angular/fw-core as a code owner Dec 2, 2019
@crisbeto

This comment has been minimized.

Copy link
Member Author

crisbeto commented Dec 2, 2019

I've added a JIT test @AndrewKushnir, although the way the check works is completely different so I don't think that it'll be an issue.

Copy link
Contributor

AndrewKushnir left a comment

Thanks for adding a test @crisbeto 👍 I've added a few minor comments, but othehrwise LGTM.

packages/core/test/acceptance/ng_module_spec.ts Outdated Show resolved Hide resolved
packages/core/test/acceptance/ng_module_spec.ts Outdated Show resolved Hide resolved
packages/core/test/acceptance/ng_module_spec.ts Outdated Show resolved Hide resolved
packages/core/test/acceptance/ng_module_spec.ts Outdated Show resolved Hide resolved
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Dec 2, 2019

@crisbeto crisbeto force-pushed the crisbeto:34171/svg-foreign-object-unknown-element branch from 4405bb8 to 3aa4208 Dec 2, 2019
@crisbeto

This comment has been minimized.

Copy link
Member Author

crisbeto commented Dec 2, 2019

Addressed the feedback @AndrewKushnir. I think I was being a bit too quick when I was putting together the extra test.

Fixes ngtsc incorrectly logging an unknown element diagnostic for HTML elements that are inside an SVG `foreignObject` with the `xhtml` namespace.

Fixes #34171.
@crisbeto crisbeto force-pushed the crisbeto:34171/svg-foreign-object-unknown-element branch from 3aa4208 to c3a0f65 Dec 2, 2019
@mhevery mhevery closed this in e6909bd Dec 3, 2019
mhevery added a commit that referenced this pull request Dec 3, 2019
)

Fixes ngtsc incorrectly logging an unknown element diagnostic for HTML elements that are inside an SVG `foreignObject` with the `xhtml` namespace.

Fixes #34171.

PR Close #34178
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.