Skip to content
This repository has been archived by the owner on Aug 29, 2023. It is now read-only.

icon: multiple SVGs on a page should have unique IDs #11395

Closed
coennijhuis opened this issue Aug 2, 2018 · 4 comments · Fixed by #11465
Closed

icon: multiple SVGs on a page should have unique IDs #11395

coennijhuis opened this issue Aug 2, 2018 · 4 comments · Fixed by #11465
Assignees
@Splaktar
Labels
a11y This issue is related to accessibility has: Pull Request A PR has been created to address this issue P2: required Issues that must be fixed. resolution: fixed type: bug
Milestone
1.1.18

Comments

@coennijhuis
Copy link

coennijhuis commented Aug 2, 2018 •
edited by Splaktar

BUG:

When adding multiple identical SVGs to a page, they should all have unique IDs.

CodePen and steps to reproduce the issue:

CodePen link: https://codepen.io/anon/pen/jpxBZx

Issue: All IDs for the icons are the same

Detailed Reproduction Steps:

  1. Click the CodePen link
  2. Check the icons in the preview pane. There are 5 identical icons.
  3. Open developer toolbar (in your browser of choice)
  4. Inspect the icons.
  5. Check the ID inside the g tag are all the same. (these should all be unique)

What is the expected behavior?

The expected behavior is that all the IDs in the tags are unique.

What is the current behavior?

The current behavior is that the IDs are all the same

What is the use-case or motivation for changing an existing behavior?

Accessibility standards state that an ID should only be used once in page. (WCAG)

Which versions of AngularJS, Material, OS, and browsers are affected?

  • AngularJS: 1.7.2
  • AngularJS Material: 1.1.10
  • OS: all
  • Browsers: all

Is there anything else we should know? Stack Traces, Screenshots, etc.

NA
@Splaktar Splaktar changed the title [MD-ICON]: Multiple SVGs on a page should have unique IDs icon: multiple SVGs on a page should have unique IDs Aug 2, 2018
@Splaktar Splaktar self-assigned this Aug 2, 2018
@Splaktar Splaktar added type: bug a11y This issue is related to accessibility P4: minor Minor issues. May not be fixed without community contributions. labels Aug 2, 2018
@Splaktar Splaktar added this to the 1.1.11 milestone Aug 2, 2018
@Splaktar Splaktar added for: external contributor needs: investigation The cause of this issue is not well understood and needs to be investigated by the team or community resolution: duplicate and removed P4: minor Minor issues. May not be fixed without community contributions. for: external contributor needs: investigation The cause of this issue is not well understood and needs to be investigated by the team or community labels Aug 2, 2018
@Splaktar Splaktar removed this from the 1.1.11 milestone Aug 2, 2018
@Splaktar
Copy link
Member

Splaktar commented Aug 2, 2018

Duplicate of #8689, but that one did not include a CodePen demo, so thank you very much for that. I'll copy the link to the CodePen over to that issue.

It looks like the proposed PR fix in #11342 does not solve the problem in this CodePen. I'll investigate some more.

@Splaktar Splaktar closed this as completed Aug 2, 2018
@Splaktar Splaktar added P3: important Important issues that really should be fixed when possible. has: Pull Request A PR has been created to address this issue and removed resolution: duplicate labels Sep 29, 2018
@Splaktar Splaktar added this to the 1.1.11 milestone Sep 29, 2018
@Splaktar
Copy link
Member

Re-opening this as the id mangling in #11342 is not the same as the lack of id caching in this issue.

@Splaktar Splaktar reopened this Sep 29, 2018
@EmielH EmielH mentioned this issue Sep 29, 2018
Merged
3 tasks
@Splaktar Splaktar mentioned this issue Sep 29, 2018
Merged
3 tasks
EmielH added a commit to breinbv/material that referenced this issue Oct 2, 2018
@EmielH
fix(icon): stop breaking SVGs that have id references
f2e9654 
Fixes angular#11395.
@Splaktar
Copy link
Member

Splaktar commented Oct 23, 2018 •
edited

This has been a problem since id caching was first implemented in 1.0.7.

@EmielH (#11465 (comment)) found that all of the icons loaded on the initial page load are loaded without caching. Caching then starts after the page load has been completed.

@Splaktar
Copy link
Member

From 1.0.7:

Currently we always rename the icons id's on cache initialization, but that causes problems, when using the cached icon multiple times in the app. A super elegant solution, is to transform the ids, when the icon is requested from the cache.

This solution solved the regression and in some cases fixed the accessibility issue. However, it did not resolve all of the a11y issues as this issue demonstrates.

@Splaktar Splaktar modified the milestones: 1.1.11, 1.1.12 Dec 14, 2018
@Splaktar Splaktar modified the milestones: 1.1.12, 1.1.13 Jan 3, 2019
@Splaktar Splaktar modified the milestones: 1.1.13, 1.1.14 Feb 10, 2019
@Splaktar Splaktar modified the milestones: 1.1.14, g3: sync Feb 15, 2019
EmielH added a commit to breinbv/material that referenced this issue Feb 26, 2019
@EmielH
fix(icon): stop breaking SVGs that have id references
795503b 
Fixes angular#11395.
@Splaktar Splaktar modified the milestones: g3: sync, 1.1.18 Apr 4, 2019
@Splaktar Splaktar added P2: required Issues that must be fixed. and removed P3: important Important issues that really should be fixed when possible. labels Apr 4, 2019
mmalerba pushed a commit that referenced this issue Apr 4, 2019
@EmielH @mmalerba
fix(icon): stop breaking SVGs that have id references (#11465)
1b37e82 
<!-- 
Filling out this template is required! Do not delete it when submitting a Pull Request! Without this information, your Pull Request may be auto-closed.
-->
## 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?
<!-- Please check the one that applies to this PR using "x". -->
```
[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?

<!-- Please describe the current behavior that you are modifying and link to one or more relevant issues. -->
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
```
<!-- If this PR contains a breaking change, please describe the impact and migration path for existing applications below. -->
<!-- Note that breaking changes are highly unlikely to get merged to master unless the validation is clear and the use case is critical. -->

## Other information
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@Splaktar Splaktar

Labels
a11y This issue is related to accessibility has: Pull Request A PR has been created to address this issue P2: required Issues that must be fixed. resolution: fixed type: bug
Projects
None yet
Milestone
1.1.18
Development

Successfully merging a pull request may close this issue.

fix(icon): stop breaking SVGs that have id references breinbv/material
2 participants
@Splaktar @coennijhuis