fix(icon): stop breaking id references when caching icon ids #11342

Splaktar · 2018-06-26T23:32:24Z

PR Checklist

Please check that your PR fulfills the following requirements:

The commit message follows our guidelines
Tests for the changes have been added or this is not a bug fix / enhancement
Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Enhancement
[x] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

SVGs with embedded ids can be broken when they are read out of the icon cache.

Issue Number:
Fixes #8689

What is the new behavior?

SVGs with embedded ids can be read out of the icon cache without breaking because the id references are now updated in addition to the ids themselves.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Thanks to @ystreibel for starting this work in #11315!

googlebot · 2018-06-26T23:32:27Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

src/components/icon/icon.spec.js

src/components/icon/js/iconService.js

ystreibel · 2018-06-27T06:55:50Z

Hi everyone! You could use my commit for this PR!

jelbourn

LGTM

googlebot · 2018-06-28T19:13:17Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

googlebot · 2018-06-28T19:40:46Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-10-17T14:19:15Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

josephperrott · 2018-10-17T14:19:23Z

Confirmed CLA manually.

Splaktar · 2018-10-20T02:37:07Z

This is causing 1 presubmit test failure that needs to be investigated and feedback provided so that a fix can be considered.

@ystreibel

…#11342)  ## PR Checklist Please check that your PR fulfills the following requirements: - [x] The commit message follows [our guidelines](https://github.com/angular/material/blob/master/.github/CONTRIBUTING.md#-commit-message-format) - [x] Tests for the changes have been added or this is not a bug fix / enhancement - [x] Docs have been added, updated, or were not required ## PR Type What kind of change does this PR introduce?  ``` [x] Bugfix [ ] Enhancement [x] Documentation content changes [ ] Code style update (formatting, local variables) [ ] Refactoring (no functional changes, no api changes) [ ] Build related changes [ ] CI related changes [ ] Infrastructure changes [ ] Other... Please describe: ``` ## What is the current behavior? SVGs with embedded `id`s can be broken when they are read out of the icon cache.  Issue Number: Fixes angular#8689 ## What is the new behavior? SVGs with embedded `id`s can be read out of the icon cache without breaking because the `id` references are now updated in addition to the `id`s themselves. ## Does this PR introduce a breaking change? ``` [ ] Yes [x] No ```   ## Other information Thanks to @ystreibel for starting this work in angular#11315!

src/components/icon/js/iconService.js

innerHTML is not supported on svg or symbol elements in IE11 use XMLSerializer instead of innerHTML when innerHTML is not defined Fixes #11543. Relates to #11342. Relates to #11162.

innerHTML is not supported on svg or symbol elements in IE11 use XMLSerializer instead of innerHTML when innerHTML is not defined add a test for empty SVGs to match a g3 test Fixes #11543. Relates to #11342. Relates to #11162.

Splaktar · 2019-02-25T02:05:51Z

It looks like our move to less verbose RegEx, in place of very verbose querySelectorAll strings, is causing a performance problem with large SVG files.

use querySelectorAll as much as possible reduce the use of RegEx on large chunks of DOM Fixes #11651. Relates to #8689. Relates to #11342. Relates to #11635.

@coennijhuis

## PR Checklist Please check that your PR fulfills the following requirements: - [X] The commit message follows [our guidelines](https://github.com/angular/material/blob/master/.github/CONTRIBUTING.md#-commit-message-format) - [X] Tests for the changes have been added or this is not a bug fix / enhancement - [X] Docs have been added, updated, or were not required ## PR Type What kind of change does this PR introduce?  ``` [X] Bugfix [ ] Enhancement [ ] Documentation content changes [ ] Code style update (formatting, local variables) [ ] Refactoring (no functional changes, no api changes) [ ] Build related changes [ ] CI related changes [ ] Infrastructure changes [ ] Other... Please describe: ``` ## What is the current behavior?  When the same icon is used multiple times, they all have the same id. Issue Number: Fixes #11395. This fixes the issue found by @coennijhuis in PR #11342. ## What is the new behavior? When the same icon is used multiple times, they all have a different id. `_cache` followed by a number is appended to the id. ## Does this PR introduce a breaking change? ``` [ ] Yes [X] No ```   ## Other information

Splaktar added type: bug type: docs needs: review This PR is waiting on review from the team P3: important Important issues that really should be fixed when possible. labels Jun 26, 2018

Splaktar added this to the 1.1.11 milestone Jun 26, 2018

Splaktar assigned josephperrott Jun 26, 2018

Splaktar requested a review from jelbourn June 26, 2018 23:32

googlebot added the cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ label Jun 26, 2018

Splaktar mentioned this pull request Jun 26, 2018

fix(md-svg-icon): id mangling on cache destroys xhref references #11315

Closed

3 tasks

jelbourn reviewed Jun 26, 2018

View reviewed changes

src/components/icon/icon.spec.js Outdated Show resolved Hide resolved

src/components/icon/js/iconService.js Outdated Show resolved Hide resolved

src/components/icon/js/iconService.js Show resolved Hide resolved

src/components/icon/js/iconService.js Show resolved Hide resolved

Splaktar added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs and removed needs: review This PR is waiting on review from the team labels Jun 26, 2018

Splaktar assigned Splaktar and unassigned josephperrott Jun 26, 2018

Splaktar force-pushed the fixSvgIdMangling branch from d1d5ab0 to 32009fe Compare June 28, 2018 07:12

Splaktar removed the cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ label Jun 28, 2018

jelbourn approved these changes Jun 28, 2018

View reviewed changes

jelbourn added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Jun 28, 2018

Splaktar force-pushed the fixSvgIdMangling branch from 32009fe to 4bde3c4 Compare June 28, 2018 19:40

googlebot added cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ and removed cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ labels Jun 28, 2018

Splaktar added pr: merge ready This PR is ready for a caretaker to review pr: lgtm This PR has been approved by the reviewer and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Jun 28, 2018

Splaktar assigned josephperrott Jun 28, 2018

jelbourn added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Jun 28, 2018

Splaktar assigned josephperrott Oct 17, 2018

josephperrott added cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ and removed cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ labels Oct 17, 2018

Splaktar added Blocked Progress on this issue is blocked. Primarily used for PRs that are blocked by presubmit feedback. pr: presubmit-failures labels Oct 20, 2018

Splaktar removed Blocked Progress on this issue is blocked. Primarily used for PRs that are blocked by presubmit feedback. pr: presubmit-failures labels Oct 23, 2018

Splaktar assigned mmalerba and unassigned josephperrott Oct 23, 2018

mmalerba merged commit 841e8b2 into master Oct 23, 2018

Splaktar deleted the fixSvgIdMangling branch October 23, 2018 19:28

Splaktar mentioned this pull request Dec 16, 2018

icon: unable to get property 'replace' of undefined or null reference at transformClone #11543

Closed

Splaktar commented Dec 17, 2018

View reviewed changes

src/components/icon/js/iconService.js Show resolved Hide resolved

Splaktar mentioned this pull request Dec 17, 2018

fix(icon): exceptions thrown by IE11 #11545

Merged

3 tasks

Splaktar mentioned this pull request Feb 25, 2019

icon(svg): large icon from cache causes 100% CPU during RegEx replacement #11651

Closed

Splaktar mentioned this pull request Feb 25, 2019

fix(icon): large SVG files can cause icon caching to hang #11653

Merged

3 tasks

Splaktar added a commit that referenced this pull request Mar 1, 2019

fix(icon): large SVG files can cause icon caching to hang

132a575

use querySelectorAll as much as possible reduce the use of RegEx on large chunks of DOM Fixes #11651. Relates to #8689. Relates to #11342. Relates to #11635.

Splaktar added a commit that referenced this pull request Mar 1, 2019

fix(icon): large SVG files can cause icon caching to hang

63ba492

use querySelectorAll as much as possible reduce the use of RegEx on large chunks of DOM Fixes #11651. Relates to #8689. Relates to #11342. Relates to #11635.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(icon): stop breaking id references when caching icon ids #11342

fix(icon): stop breaking id references when caching icon ids #11342

Splaktar commented Jun 26, 2018

googlebot commented Jun 26, 2018

ystreibel commented Jun 27, 2018

jelbourn left a comment

googlebot commented Jun 28, 2018

googlebot commented Jun 28, 2018

googlebot commented Oct 17, 2018

josephperrott commented Oct 17, 2018

Splaktar commented Oct 20, 2018

Splaktar commented Feb 25, 2019

fix(icon): stop breaking id references when caching icon ids #11342

fix(icon): stop breaking id references when caching icon ids #11342

Conversation

Splaktar commented Jun 26, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

googlebot commented Jun 26, 2018

ystreibel commented Jun 27, 2018

jelbourn left a comment

Choose a reason for hiding this comment

googlebot commented Jun 28, 2018

googlebot commented Jun 28, 2018

googlebot commented Oct 17, 2018

josephperrott commented Oct 17, 2018

Splaktar commented Oct 20, 2018

Splaktar commented Feb 25, 2019