X Tutup
Skip to content

fix(compiler): support properties on SVG elements#5653

Closed
teropa wants to merge 1 commit intoangular:masterfrom
teropa:fix-svg-element-property-detection
Closed

fix(compiler): support properties on SVG elements#5653
teropa wants to merge 1 commit intoangular:masterfrom
teropa:fix-svg-element-property-detection

Conversation

@teropa
Copy link
Copy Markdown
Contributor

@teropa teropa commented Dec 6, 2015

Have DomElementSchemaRegistry support namespaced elements,
so that it does not fail when directives are applied in SVG (or xlink).
Without this fix, directives or property bindings cannot be
used in SVG.

Partially closes #5547

@teropa
Copy link
Copy Markdown
Contributor Author

teropa commented Dec 6, 2015

This introduces some duplicated code between DomElementSchemaRegistry and DomRenderer, because both now do the same namespace split & check. I'm not sure what the best way to eliminate that would be.

@vicb
Copy link
Copy Markdown
Contributor

vicb commented Dec 6, 2015

@teropa thanks for the fix.

I think we have to live with the code duplication for now as the compiler code might not be loaded if the templates are pre-compiled.

However could you try to somehow factorize https://github.com/angular/angular/blob/master/modules/angular2/src/compiler/html_parser.ts#L249 and your code (may be in html_tags ?).

Please allow changes Partially closes #5547 to related to #... in your commit message, otherwise GH might automagically close the issue. Thanks.

@vicb vicb added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Dec 6, 2015
@teropa teropa force-pushed the fix-svg-element-property-detection branch from 1a9e5ff to cf6c0d3 Compare December 7, 2015 08:46
@teropa
Copy link
Copy Markdown
Contributor Author

teropa commented Dec 7, 2015

@vicb Pushed an attempt to combine the two via html_tags. Also added a simple E2E test for SVG.

@marclaval
Copy link
Copy Markdown
Contributor

The new test is failing in all IEs, Androids (4.1 to 4.3) and Safari 7/iOS7:

DOMElementSchema should detect properties on namespaced elements FAILED
    Expected false to be truthy.

@teropa teropa force-pushed the fix-svg-element-property-detection branch 2 times, most recently from 9c69656 to d5086fd Compare December 7, 2015 14:46
@teropa
Copy link
Copy Markdown
Contributor Author

teropa commented Dec 7, 2015

Thanks @Mlaval, should be OK now

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

@vicb
Copy link
Copy Markdown
Contributor

vicb commented Dec 7, 2015

LGTM with a small cleanup, thanks !

Have DomElementSchemaRegistry support namespaced elements,
so that it does not fail when directives are applied in SVG (or xlink).
Without this fix, directives or property bindings cannot be
used in SVG.

Related to angular#5547
@teropa teropa force-pushed the fix-svg-element-property-detection branch from d5086fd to ebddac6 Compare December 7, 2015 17:27
@vicb
Copy link
Copy Markdown
Contributor

vicb commented Dec 7, 2015

@teropa could you please rebase ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about adding a test that would be falsy ?

@vicb vicb added pr_state: LGTM action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Dec 8, 2015
@vicb vicb added this to the beta.0 milestone Dec 8, 2015
@mary-poppins
Copy link
Copy Markdown

Merging PR #5653 on behalf of @jelbourn to branch presubmit-jelbourn-pr-5653.

@angular-automatic-lock-bot
Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SVG components don't work in alpha.47

6 participants

X Tutup