Skip to content

Conversation

@lumirlumir
Copy link
Member

@lumirlumir lumirlumir commented Sep 29, 2025

Prerequisites checklist

What is the purpose of this pull request?

The original intention of this PR was to refactor the code to use getLocFromIndex, but I found a small bug while working on it, so I fixed it as well.

Which language are you using?

CommonMark and GFM.

What did you do?

<img src="image.png" aria-hidden alt="alt">

When I run the code above, the rule crashes for the following reason: a missing null check.

(FYI, the code above is recognized as valid syntax even without true or false following it.)

image

What did you expect to happen?

No crash would happen.

Link to minimal reproducible Example

'<img src="image.png" aria-hidden alt="alt">'

Adding the above code to the valid section of require-alt-text.test.js would help identify the problem.

What changes did you make? (Give an overview)

The original intention of this PR was to refactor the code to use getLocFromIndex, but I found a small bug while working on it, so I fixed it as well.

This PR originated from #536. While working on that, I found the scope had become too large, so I split this refactoring into a separate PR.

In this PR, I've used the native getLocFromIndex method added in #376 instead of the custom findOffsets util.

Notable changes:

  • Replace the findOffsets util with the native getLocFromIndex.
  • Use ESQuery selectors to reduce duplicated logic.
  • Fix a bug.

Related Issues

Ref: #376, #536

Is there anything you'd like reviewers to focus on?

N/A

@eslintbot eslintbot added this to Triage Sep 29, 2025
@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Sep 29, 2025
@lumirlumir lumirlumir changed the title fix: add null check and use getlocfromindex in require-alt-text fix: add null check and use getLocFromIndex in require-alt-text Sep 29, 2025
);
if (
ariaHiddenMatch &&
ariaHiddenMatch[1] &&
Copy link
Member Author

@lumirlumir lumirlumir Sep 29, 2025

Choose a reason for hiding this comment

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

This is the only place where the bug fix is needed (it's a one-line change).

Other changes are related to code refactoring.

I could use optional chaining, but I used this check to match the implementation on line 97.

image

@lumirlumir lumirlumir marked this pull request as ready for review September 29, 2025 11:40
@lumirlumir lumirlumir requested review from a team and mdjermanovic September 29, 2025 11:40
@lumirlumir lumirlumir marked this pull request as draft September 29, 2025 13:21
@lumirlumir
Copy link
Member Author

lumirlumir commented Sep 29, 2025

To apply the feedback from #541, I'll mark this PR as a draft shortly.


All clear.

@lumirlumir lumirlumir marked this pull request as ready for review September 29, 2025 13:32
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit 41ae6c8 into main Sep 29, 2025
23 checks passed
@mdjermanovic mdjermanovic deleted the refactor-use-getlocfromindex-in-require-alt-text branch September 29, 2025 14:54
@github-project-automation github-project-automation bot moved this from Needs Triage to Complete in Triage Sep 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

3 participants