Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Feb 29, 2024

Conversation

nnmrts
Copy link
Contributor

@nnmrts nnmrts commented Feb 25, 2024

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.

@nnmrts nnmrts requested a review from a team as a code owner February 25, 2024 20:22
@dgreif dgreif requested review from a team and accessibility-bot February 26, 2024 21:04
@accessibility-bot
Copy link

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

  • 💻 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.
  • ⚠️ If this is urgent, please visit us in #accessibility on Slack and tag the first responder(s) listed in the channel topic.

@andrialexandrou
Copy link

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.

@theinterned theinterned removed their request for review February 27, 2024 13:01
Copy link

@andrialexandrou andrialexandrou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good as a proposal. Can you add a test that validates for this whitespace scenario in tests/a11y-svg-has-accessible-name.js and ensure the linters pass as well? I can get this approved after that.

Thanks for the suggestion!

lib/rules/a11y-svg-has-accessible-name.js Outdated Show resolved Hide resolved
nnmrts and others added 2 commits February 28, 2024 04:37
Co-authored-by: Andri Alexandrou <andrialexandrou@github.com>
@nnmrts
Copy link
Contributor Author

nnmrts commented Feb 29, 2024

@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.

Copy link

@andrialexandrou andrialexandrou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem at all, thanks for making these contributions. To your question, placement there seems just fine.

Approving!

@andrialexandrou andrialexandrou merged commit 30f5d80 into github:main Feb 29, 2024
3 checks passed
@andrialexandrou
Copy link

andrialexandrou commented Feb 29, 2024

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

mergify bot pushed a commit to Balvajs/delete-stale-branches that referenced this pull request Mar 1, 2024
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
|
[eslint-plugin-github](https://togithub.com/github/eslint-plugin-github)
| devDependencies | patch | [`4.10.1` ->
`4.10.2`](https://renovatebot.com/diffs/npm/eslint-plugin-github/4.10.1/4.10.2)
|

---

### Release Notes

<details>
<summary>github/eslint-plugin-github (eslint-plugin-github)</summary>

###
[`v4.10.2`](https://togithub.com/github/eslint-plugin-github/releases/tag/v4.10.2)

[Compare
Source](https://togithub.com/github/eslint-plugin-github/compare/v4.10.1...v4.10.2)

#### What's Changed

##### Functionality

- Update no-then.md by [@&#8203;cefn](https://togithub.com/cefn) in
[github/eslint-plugin-github#503
- fix `a11y-svg-has-accessible-name` considering whitespace JSXText by
[@&#8203;nnmrts](https://togithub.com/nnmrts) in
[github/eslint-plugin-github#508

##### Dependancy updates

- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#482
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#484
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#485
- chore(deps): bump the all-dependencies group with 4 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#486
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#487
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#489
- chore(deps): bump actions/setup-node from 3 to 4 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#488
- chore(deps): bump the all-dependencies group with 5 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#490
- chore(deps): bump the all-dependencies group with 2 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#491
- chore(deps): bump the all-dependencies group with 5 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#492
- chore(deps): bump the all-dependencies group with 2 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#493
- chore(deps): bump the all-dependencies group with 4 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#494
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#495
- chore(deps): bump the all-dependencies group with 5 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#496
- chore(deps): bump the all-dependencies group with 4 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#498
- chore(deps): bump the all-dependencies group with 2 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#499
- chore(deps): bump the all-dependencies group with 5 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#500
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#501
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#502
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#504
- chore(deps): bump the all-dependencies group with 5 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#507
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#509

#### New Contributors

- [@&#8203;cefn](https://togithub.com/cefn) made their first
contribution in
[github/eslint-plugin-github#503
- [@&#8203;nnmrts](https://togithub.com/nnmrts) made their first
contribution in
[github/eslint-plugin-github#508

**Full Changelog**:
github/eslint-plugin-github@v4.10.1...v4.10.2

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://togithub.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yMjEuMCIsInVwZGF0ZWRJblZlciI6IjM3LjIyMS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: Balvajs Renovate Bot <406530+balvajs-renovate[bot]@users.noreply.github.com>
mergify bot pushed a commit to Balvajs/dismiss-stale-reviews that referenced this pull request Mar 1, 2024
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
|
[eslint-plugin-github](https://togithub.com/github/eslint-plugin-github)
| devDependencies | patch | [`4.10.1` ->
`4.10.2`](https://renovatebot.com/diffs/npm/eslint-plugin-github/4.10.1/4.10.2)
|

---

### Release Notes

<details>
<summary>github/eslint-plugin-github (eslint-plugin-github)</summary>

###
[`v4.10.2`](https://togithub.com/github/eslint-plugin-github/releases/tag/v4.10.2)

[Compare
Source](https://togithub.com/github/eslint-plugin-github/compare/v4.10.1...v4.10.2)

#### What's Changed

##### Functionality

- Update no-then.md by [@&#8203;cefn](https://togithub.com/cefn) in
[github/eslint-plugin-github#503
- fix `a11y-svg-has-accessible-name` considering whitespace JSXText by
[@&#8203;nnmrts](https://togithub.com/nnmrts) in
[github/eslint-plugin-github#508

##### Dependancy updates

- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#482
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#484
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#485
- chore(deps): bump the all-dependencies group with 4 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#486
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#487
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#489
- chore(deps): bump actions/setup-node from 3 to 4 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#488
- chore(deps): bump the all-dependencies group with 5 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#490
- chore(deps): bump the all-dependencies group with 2 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#491
- chore(deps): bump the all-dependencies group with 5 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#492
- chore(deps): bump the all-dependencies group with 2 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#493
- chore(deps): bump the all-dependencies group with 4 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#494
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#495
- chore(deps): bump the all-dependencies group with 5 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#496
- chore(deps): bump the all-dependencies group with 4 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#498
- chore(deps): bump the all-dependencies group with 2 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#499
- chore(deps): bump the all-dependencies group with 5 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#500
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#501
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#502
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#504
- chore(deps): bump the all-dependencies group with 5 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#507
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#509

#### New Contributors

- [@&#8203;cefn](https://togithub.com/cefn) made their first
contribution in
[github/eslint-plugin-github#503
- [@&#8203;nnmrts](https://togithub.com/nnmrts) made their first
contribution in
[github/eslint-plugin-github#508

**Full Changelog**:
github/eslint-plugin-github@v4.10.1...v4.10.2

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://togithub.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yMjEuMCIsInVwZGF0ZWRJblZlciI6IjM3LjIyMS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: Balvajs Renovate Bot <406530+balvajs-renovate[bot]@users.noreply.github.com>
renovate bot added a commit to WtfJoke/setup-tectonic that referenced this pull request Mar 4, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[eslint-plugin-github](https://togithub.com/github/eslint-plugin-github)
| [`4.10.1` ->
`4.10.2`](https://renovatebot.com/diffs/npm/eslint-plugin-github/4.10.1/4.10.2)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/eslint-plugin-github/4.10.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/eslint-plugin-github/4.10.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/eslint-plugin-github/4.10.1/4.10.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/eslint-plugin-github/4.10.1/4.10.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>github/eslint-plugin-github (eslint-plugin-github)</summary>

###
[`v4.10.2`](https://togithub.com/github/eslint-plugin-github/releases/tag/v4.10.2)

[Compare
Source](https://togithub.com/github/eslint-plugin-github/compare/v4.10.1...v4.10.2)

#### What's Changed

##### Functionality

- Update no-then.md by [@&#8203;cefn](https://togithub.com/cefn) in
[github/eslint-plugin-github#503
- fix `a11y-svg-has-accessible-name` considering whitespace JSXText by
[@&#8203;nnmrts](https://togithub.com/nnmrts) in
[github/eslint-plugin-github#508

##### Dependancy updates

- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#482
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#484
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#485
- chore(deps): bump the all-dependencies group with 4 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#486
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#487
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#489
- chore(deps): bump actions/setup-node from 3 to 4 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#488
- chore(deps): bump the all-dependencies group with 5 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#490
- chore(deps): bump the all-dependencies group with 2 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#491
- chore(deps): bump the all-dependencies group with 5 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#492
- chore(deps): bump the all-dependencies group with 2 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#493
- chore(deps): bump the all-dependencies group with 4 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#494
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#495
- chore(deps): bump the all-dependencies group with 5 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#496
- chore(deps): bump the all-dependencies group with 4 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#498
- chore(deps): bump the all-dependencies group with 2 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#499
- chore(deps): bump the all-dependencies group with 5 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#500
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#501
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#502
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#504
- chore(deps): bump the all-dependencies group with 5 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#507
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[github/eslint-plugin-github#509

#### New Contributors

- [@&#8203;cefn](https://togithub.com/cefn) made their first
contribution in
[github/eslint-plugin-github#503
- [@&#8203;nnmrts](https://togithub.com/nnmrts) made their first
contribution in
[github/eslint-plugin-github#508

**Full Changelog**:
github/eslint-plugin-github@v4.10.1...v4.10.2

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 4am on Monday" in timezone
Europe/Berlin, Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/WtfJoke/setup-tectonic).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yMjAuMiIsInVwZGF0ZWRJblZlciI6IjM3LjIyMC4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants