Make Contributor buttons accessible

[tunetheweb]

Fixes #1040
Fixes #1045

This also makes the 2020 contributors template an extension of 2019 template to avoid duplication of code.

@nico3333fr could you check I've implemented what you mean? And also the French translation?
I've deployed a test version here to make that easier for you to test: https://20200716t131108-dot-webalmanac.uk.r.appspot.com

[rviscomi]

LGTM just one script suggestion

[tunetheweb]

BTW one weird thing that I noticed about this - and update since I posted above demo link.

What should the initial state of the button be?

Originally I had "pressed" since they are all yellow. However then when you press it (to select just this team), it doesn't change state since it is still press - which is weird that pressing the button doesn't change state.

So I changed it to all be "unpressed" initially, which makes more semantic sense to me when you do press a button. However it does create a clash with the colouring.

Maybe initially colouring should all be white rather than all yellow? Thoughts?

[tunetheweb]

Planning a release so going to go ahead and merge this. I think it's good, and certainly an improvement at very least.

Would love to hear your feedback though @nico3333fr and @ericwbailey if there are any further improvements we can make here.

[ericwbailey]

Apologies on the late reply, I've got a demanding client and was head's down with a site launch. @bazzadp, thank you for the deployed test version, it made auditing this a lot easier.

A couple of questions back to you, but overall I think this is in good shape.

[ericwbailey]

I think this is a correct usage pattern, but we might want to check for a double-announce or mis-announce when embedded SVG is used to provide a label. I personally prefer the visually-hidden CSS technique, as it allows things like browser translation.

[tunetheweb]

Yeah this is based on usage pattern 8 from Deque’s Accessible SVG post, which seems the best, even if it does cause a double announcement in at least one case. Do browsers not translate SVG components like the <title> tag here?

[ericwbailey]

As I understand it, anything passed through aria-* has the chance of not being picked up by browser/3rd party translation (like, I18n-style), as it's an attribute. I usually err on the side of avoiding it because of that.

[tunetheweb]

But it’s aria-labelledby not aria-label so points to an element for that very reason, so bit confused what you are saying? It’s an SVG element rather than an HTML element but I think that’s still OK.

[tunetheweb]

And it’s a <title> SVG element not a title attribute. Is that the cause of confusion because you didn’t notice that and thought it was the attribute?

[ericwbailey]

Nope, just involving the ARIA attribute in the chain. Again, I don't think it would be a blocking merging issue if the issue was still open, just a corner case usability consideration.

[tunetheweb]

Thanks for the great review @ericwbailey! And yeah like the test deploy to help review implementation rather than code. Though in this case I ended up merging before you review so you could have tested on the live site!

Any thoughts on my comment above on the default state being slightly weird (it’s the only time yellow is used to mean not pressed). When I ended up deploying to the live site was actually different to the test link.

[ericwbailey]

Oop, forgot to address that. I like your idea of all white to begin with.