✨ make HdIcon accessible #481
Conversation
majakomel
requested review from
aym3nb,
drapisarda,
ilyasmez,
LarsFe,
leandroinacio,
metal-gogo,
SinisaG,
viniciuskneves and
volcanioo
as code owners
There was a problem hiding this comment.
Hi @majakomel,
Congrats on your first PR 🎉
And thank you for tackling one of our open issues, we really appreciate it 😃
I've dropped some comments, please check them and ask questions if you have any.
src/components/HdIcon.vue
Outdated
| const descId = `${this.id || randId}-desc`; | ||
| const descTag = document.createElementNS('http://www.w3.org/2000/svg', 'desc'); | ||
| descTag.id = descId; | ||
| svg.appendChild(descTag); |
There was a problem hiding this comment.
You're using appendChild here and insetBefore for the titleTag, any reason for this?
There was a problem hiding this comment.
Yes, it's recommended that the title is the first child because of backward compatibility with SVG 1.1 (https://developer.mozilla.org/en-US/docs/Web/SVG/Element/title). It doesn't seem to matter for desc.
There was a problem hiding this comment.
Hey @majakomel
Thanks for submitting the PR, nice job!
I saw that @ilyasmez already went through most of the code so I will just add my 2 cents here 😅
src/components/HdIcon.vue
Outdated
| const randId = Math.floor(Math.random() * 1000); | ||
| if (this.title) { | ||
| const titleId = `${this.id || randId}-title`; | ||
| const titleTag = document.createElementNS('http://www.w3.org/2000/svg', 'title'); | ||
| titleTag.textContent = this.title; | ||
| titleTag.id = titleId; | ||
| svg.insertBefore(titleTag, svg.firstChild); | ||
| svg.setAttribute('aria-labelledby', titleId); | ||
| } |
There was a problem hiding this comment.
I would suggest you to split title and description into separated functions just as a matter of structuring the code.
There was a problem hiding this comment.
Maybe you can use if like that, this practice may be more readable
if (this.title) return false;
const randId = Math.floor(Math.random() * 1000);
const titleId = `${this.id || randId}-title`;
const titleTag = document.createElementNS('http://www.w3.org/2000/svg', 'title');
titleTag.textContent = this.title;
titleTag.id = titleId;
svg.insertBefore(titleTag, svg.firstChild);
svg.setAttribute('aria-labelledby', titleId);| it('adds title and desc tag to the svg', async () => { | ||
| const wrapper = wrapperBuilder({ | ||
| props: { | ||
| src: 'fake/icon4.svg', | ||
| title: 'This make me accessible.', | ||
| description: 'Some description.', | ||
| id: 'icon-id', | ||
| }, | ||
| }); | ||
|
|
||
| await wrapper.vm.$nextTick(); | ||
|
|
||
| expect(wrapper.html()).toMatchSnapshot(); | ||
| }); |
There was a problem hiding this comment.
I would suggest you to add a test to cover the "random id" case as well.
|
Sorry for the delay and thanks for suggestions @viniciuskneves & @volcanioo, I reshuffled the code a bit, looks better separated into 2 methods :) |
src/components/HdIcon.vue
Outdated
| const randId = Math.floor(Math.random() * 1000); | ||
| if (this.title) this.addTitleTag(svg, randId); | ||
| if (this.description) this.addDescTag(svg, randId); |
There was a problem hiding this comment.
Ehhh, I think the idea of addAccessibilityTags is valid. There I would call the other methods 🙈
Sorry 😅
tests/unit/components/HdIcon.spec.js
Outdated
| it("generates the title and desc id if it's not provided", async () => { | ||
| const wrapper = wrapperBuilder({ | ||
| props: { | ||
| src: 'fake/icon5.svg', | ||
| title: 'This make me accessible.', | ||
| description: 'Some description.', | ||
| }, | ||
| }); | ||
|
|
||
| await wrapper.vm.$nextTick(); | ||
|
|
||
| expect(wrapper.html()).toMatchSnapshot(); |
tests/unit/components/HdIcon.spec.js
Outdated
| const titleTag = wrapper.find('title') | ||
| const descTag = wrapper.find('desc') | ||
| expect(titleTag.element.id).toMatch(/\d+-title/) | ||
| expect(descTag.element.id).toMatch(/\d+-desc/) |
There was a problem hiding this comment.
Ohhh, the random is biting here, right? 🙈
There was a problem hiding this comment.
Yeah, it didn't fail in the first run :D
Hi!
This PR improves HdIcon accessibility as suggested in #414
Changes follow the tips on accessible SVGs from the linked article - the title and desc tags are added to the SVG, along with
aria-labelledbyandroleattributes.Looking forward to the review 🙂