fix `a11y-svg-has-accessible-name` considering whitespace JSXText

nnmrts commented 4 months ago

Hello!

This PR fixes an issue I encountered with a11y-svg-has-accessible-name. I'm not sure if it's relevant to any "vanilla" use-cases of this plugin, eslint and/or react. My stack is probably very different (deno, fresh, preact) from many users of this plugin and the sole reason I encounter this issue (it's probably Preact, right?), but this PR shouldn't introduce any breaking change to "normal" users.

In my case, the issue was the following:

const Test = () => {
    return (
        <svg>
            <title>Title</title>
        </svg>
    )
};

export default Test.

Even though I added a title element here, this plugin errored a11y-svg-has-accessible-name. After inspecting how this rule is implemented, I saw that it checks if the first child is of type JSXElement and openingElement?.name?.name === 'title'.

For some reason the JSX parser, whichever it is in my case, inserts a node of type JSXText with a value of \n\t\t\t as the first child of the svg element. I get where it's coming from, since there is actual whitespace in my source code, but I feel like whichever parser does this, should just ignore whitespace here. Maybe this is an upstream issue, but this PR, which filters these empty nodes out before checking, seemed like a quicker fix.

accessibility-bot commented 4 months ago

:wave: Hello and thanks for pinging us! You've entered our first responder queue. An accessibility first responder will review this soon.

:computer: On PRs for our review: please provide a review environment with steps to validate, screenshots (with alt text), or videos demonstrating functionality we should be checking. This will help speed up our review and feedback cycle.
:warning: If this is urgent, please visit us in #accessibility on Slack and tag the first responder(s) listed in the channel topic.

andrialexandrou commented 4 months ago

Hi @nnmrts thanks for your contribution! I'll be able to review this in the next day or so and let you know. Thanks for raising that this is causing issues for you and we certainly want to get you unblocked as soon as possible.

nnmrts commented 4 months ago

@andrialexandrou Thanks for taking a look! Sorry that it took me 2 full days, I added a test now, which initially didn't pass until I fixed some prettier issues, so I had to change lib/rules/a11y-svg-has-accessible-name.js again as well.

EDIT: I hope it's okay that I added the test as the second case, I felt like it's related to the first one.

andrialexandrou commented 4 months ago

@nnmrts your work is available in the latest release, which you can update via npm:

github / eslint-plugin-github

fix `a11y-svg-has-accessible-name` considering whitespace JSXText #508