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): match directives on namespaced elements #33555

Closed
wants to merge 2 commits into from

Conversation

@JoostK
Copy link
Member

JoostK commented Nov 3, 2019

Prior to this change, namespaced elements such as SVG elements would not
participate correctly in directive matching as their namespace was not
ignored, which was the case with the View Engine compiler. This led to
incorrect behavior at runtime and template type checking.

This commit resolved the issue by ignoring the namespace of elements and
attributes like they were in View Engine.

Fixes #32061

Prior to this change, namespaced elements such as SVG elements would not
participate correctly in directive matching as their namespace was not
ignored, which was the case with the View Engine compiler. This led to
incorrect behavior at runtime and template type checking.

This commit resolved the issue by ignoring the namespace of elements and
attributes like they were in View Engine.

Fixes #32061
@@ -260,6 +260,28 @@ describe('directives', () => {
expect(calls).toEqual(['MyDir.ngOnInit']);
});

it('should match namespaced elements', () => {

This comment has been minimized.

Copy link
@pkozlowski-opensource

pkozlowski-opensource Nov 3, 2019

Member

"should match directives on elements with namespace"? We should say what we are trying to match

This comment has been minimized.

Copy link
@JoostK

JoostK Nov 3, 2019

Author Member

Thanks, that's what I meant it to say ;)

it('should match namespaced elements', () => {
const calls: string[] = [];

@Directive({selector: 'text[dir]'})

This comment has been minimized.

Copy link
@pkozlowski-opensource

pkozlowski-opensource Nov 3, 2019

Member

could you please add another test where we try to match a directive on the <svg> directly? A case like <svg dir> where directive's selector is svg[dir]? This case is currently broken on ng-bootstrap and we had to work-around it, see ng-bootstrap/ng-bootstrap@0353fc9

This comment has been minimized.

Copy link
@JoostK

JoostK Nov 3, 2019

Author Member

Excellent idea!

@JoostK JoostK marked this pull request as ready for review Nov 3, 2019
@JoostK JoostK requested review from angular/fw-compiler as code owners Nov 3, 2019
Copy link
Member

pkozlowski-opensource left a comment

LGTM for runtime tests

@alxhub
alxhub approved these changes Nov 6, 2019
@alxhub

This comment has been minimized.

Copy link
Contributor

alxhub commented Nov 6, 2019

@atscott atscott closed this in bca4376 Nov 7, 2019
atscott added a commit that referenced this pull request Nov 7, 2019
Prior to this change, namespaced elements such as SVG elements would not
participate correctly in directive matching as their namespace was not
ignored, which was the case with the View Engine compiler. This led to
incorrect behavior at runtime and template type checking.

This commit resolved the issue by ignoring the namespace of elements and
attributes like they were in View Engine.

Fixes #32061

PR Close #33555
mohaxspb added a commit to mohaxspb/angular that referenced this pull request Nov 7, 2019
Prior to this change, namespaced elements such as SVG elements would not
participate correctly in directive matching as their namespace was not
ignored, which was the case with the View Engine compiler. This led to
incorrect behavior at runtime and template type checking.

This commit resolved the issue by ignoring the namespace of elements and
attributes like they were in View Engine.

Fixes angular#32061

PR Close angular#33555
mohaxspb added a commit to mohaxspb/angular that referenced this pull request Nov 7, 2019
Prior to this change, namespaced elements such as SVG elements would not
participate correctly in directive matching as their namespace was not
ignored, which was the case with the View Engine compiler. This led to
incorrect behavior at runtime and template type checking.

This commit resolved the issue by ignoring the namespace of elements and
attributes like they were in View Engine.

Fixes angular#32061

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