Components: Adds AccessibleSVG component and use consistently for block icons

[youknowriad]

closes #9372

• This PR adds an AccessibleSVG component
• Use this component in BlockIcon

Test that the svg icons contain the required A11y props.

[jasmussen]

This is impressive. From my quick testing, it seems like all the block icons had the proper attributes as they need. Fine work! I can't really speak too much for the code — it looks good to me. But from everything else, 👍 👍

Thanks for working on this.

[tofumatt]

I think some documentation around what happens here would be useful as right now this is elegant, but a bit Magic™.

Overall a cool solution though, and definitely one that prevents third-party developers from making mistakes.

I trust you to add some kind of documentation or something, so 👍 with docs. Feel free to ping for further review if needed 😄

[tofumatt]

That is, I suppose, a better solution that prevents developers who don't bother to use our components from causing an error. It's definitely less explicit though and I don't see any documentation around it: either that using AccessibleSVG directly is preferred or even that if the icon prop is an SVG that we'll add properties to it. We should probably at least document this, as it might be quite jarring to a block developer to see their icon markup change unexpectedly like this with no documentation.

[youknowriad]

I personally don't think it's important enough for people to know precisely that we're adding some props to their SVGs to make them accessible. I guess it doesn't harm to add a mention in the documention but if we do so, we should avoid using AccessibleSVG directly in the components because the behavior is documented in that case.

[tofumatt]

That's fair. I think I'd prefer the documentation because it teaches people about an accessibility issue and it means our code can be: 1. a bit leaner (no imports) 2. more idiomatic (regular SVGs)

So maybe just a note of "we add accessibility properties to SVGs" in the block property documentation with a link to the original issue for context or something.

[tofumatt]

I get that we don't have to use our component directly, but I wonder why we wouldn't. This is less explicit and we're already changing all of the lines 🤷‍♂️

[youknowriad]

I don't mind, I was hesitant as well.

[tofumatt]

As mentioned though, feel free to leave them SVG if you add docs. I think that's probably the better approach, upon reflection.

[youknowriad]

@tofumatt I added a note to the docs, feel free to tweak it (I'm not great at documentation :P)

[tofumatt]

Docs look good; I made a few tweaks (mainly changing a11y to accessibility).

[afercia]

Test that the svg icons contain the required A11y props.

❤️

[caxco93]

I think we should extract the SVG icons to another package, maybe @wordpress/icons. This will increase their reusability and maintainability. Right now I was looking at #9642 and it would require to declare the same SVG again for the MediaPlaceholder icon.

Here's the PR where I extract the SVG icons to a new package

Then we could simply do it like this

@youknowriad @tofumatt

[jasmussen]

CC: @gziolo because you mentioned https://github.com/airbnb/babel-plugin-inline-react-svg/blob/master/README.md

[mtias]

@youknowriad coming a bit late to this, but I feel this should be just called Svg (with capital) as being accessible should be core to all the components we offer.

[tofumatt]

That's kinda confusing if you quickly scan it 🤷‍♂️ (<Svg /> vs <svg />), but also the point is the stock SVGs aren't always accessible and the aim of the wrapper is to make them so.