fix: support unquoted aria-hidden value in require-alt-text - #719
Conversation
|
|
||
| const imgTagPattern = /<img(?:\s(?:[^>"']|"[^"]*"|'[^']*')*)?\/?>/giu; | ||
| const ariaHiddenTruePattern = | ||
| /\saria-hidden\s*=\s*(?:"true"|'true'|true)(?=\s|\/?>)/iu; |
There was a problem hiding this comment.
There does not have to be whitespace around an HTML attribute, e.g. <a before="true"aria-hidden="true"after="true"></a> is valid.
In other words the before "aria-hidden" can be a quote or whitespace and alpha characters if the attribute value is quoted.
There was a problem hiding this comment.
I’d like to update it as follows to account for the additional cases.
| Case | Before | After | Reason |
|---|---|---|---|
| Boundary before aria-hidden | \s | (?:\s|["']) | Allows aria-hidden to be preceded not only by whitespace, but also directly by the closing quote (" or ') of the previous attribute. |
| Boundary after true | (?:"true"|'true'|true)(?=\s|/?>) | (?:"true"|'true'|true(?=\s|/?>)) | Allows another attribute to immediately follow "true" or 'true' without whitespace, while keeping the existing boundary check for unquoted true so that cases like aria-hidden=truefoo are not matched incorrectly. |
I’m thinking of updating the final pattern as follows. Is there any case I may have misunderstood? (https://regexr.com/8o87r)
/(?:\s|["'])aria-hidden\s*=\s*(?:"true"|'true'|true(?=\s|\/?>))/iuThere was a problem hiding this comment.
Yes the new pattern works for the mentioned cases.
Can you please add test cases for this and replace > with > (no need to escape it in this case) in the pattern?
There was a problem hiding this comment.
I updated the regular expression to address the regexp/prefer-character-class ESLint error.
The previous pattern used a disjunction of single-character alternatives:
(?:\s|["'])Since this matches either whitespace, " or ', it can be expressed more directly as a character class without changing its meaning:
[\s"']So I updated the pattern as follows:
/[\s"']aria-hidden\s*=\s*(?:"true"|'true'|true(?=\s|\/?>))/iuHere, [\s"'] allows aria-hidden to be preceded by whitespace or by the closing quote of the previous attribute. For the unquoted true form, the lookahead (?=\s|\/?>) is kept to ensure that values such as aria-hidden=truefoo are not matched.
I also added test cases for attributes without whitespace between them and confirmed that the previously discussed cases still work as expected. (https://regexr.com/8o8jo)
Prerequisites checklist
AI acknowledgment
What is the purpose of this pull request?
This PR updates require-alt-text to recognize an unquoted aria-hidden=true attribute.
HTML permits attribute values to be written without quotes, but the existing implementation handles the following forms differently:
Since both forms set the value of aria-hidden to true, this PR updates the rule to skip the alternative text check in both cases.
What changes did you make? (Give an overview)
aria-hidden=trueattributes.aria-hidden=truearia-hidden = trueARIA-HIDDEN=TRUEaria-hidden=falseandaria-hidden=truefooare still reported.The regex test cases for this change are available on RegExr.
Related Issues
fixes #718
Is there anything you'd like reviewers to focus on?
To keep the scope of this change minimal, the implementation adds a dedicated regular expression for detecting
aria-hidden=trueinstead of extendinggetHtmlAttributeRe().There are two reasons for this approach:
altattribute parsing behavior and avoids recreating thearia-hiddenregular expression each time an image tag is processed.aria-hiddenrequires checking whether its value is exactlytrue, whilegetHtmlAttributeRe("alt")captures the value of thealtattribute to determine whether it consists only of whitespace. I considered these attributes to have different validation purposes and criteria.I would appreciate feedback on whether this dedicated regular expression is appropriate or whether it would be better to generalize
getHtmlAttributeRe()to handle both quoted and unquoted attribute values. :)