-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix(md-icon): accessibility issue with unique ids #7440
fix(md-icon): accessibility issue with unique ids #7440
Conversation
|
||
for (var i=0; i<children.length;i++) { | ||
var child = children[i]; | ||
if(child.nodeName==='path') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only on the path
node? circle
etc, can also hold an id
attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I will remove the if.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@david-gang I suggest, walking recursively through the children and remove the id
if present.
Like so
(function recursiveIteration(element) {
angular.forEach(element.childNodes, function(item) {
item.removeAttribute('id');
if (item.childNodes) {
recursiveIteration(item)
}
});
})(myIconElement);
Or another way, would be the querySelector
. Like so
angular.forEach(myIconElement.querySelectorAll('[id]'), function(item) {
item.removeAttribute('id');
});
But I'm not sure what's the most elegant / performant solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devversion The second option looks very nice. It should also be performant because it is a native function and the call stack is flat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@david-gang Yes definitely, it looks more elegant, but I heard some negative about attribute selectors
in IE and other browsers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just updated your test - now it works. See the results here: http://jsperf.com/recursive-iteration-vs-queryselectorall
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is just called once per icon type, unconnected to the number of times i use the icon in the application, and even in ie11 the performance is 10^5 ops per second, so this should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's okay 😄 Just wanted to share with you the performance differences.
We remove also recursively the descendants ids.
fix(md-icon): accessibility issue with unique ids
When the same svg icon is placed multiple times in the same page, it raises an aria warning that every element should have an unique id.
The solution is to remove the id from the svg and the path children.
closes: #6796