Wrap around eslint-plugin-jsx-a11y/anchor-has-content to support createInterpolateElement usage #50065

rowasc · 2023-04-25T14:36:41Z

What?

Changes:

packages/eslint-plugin/configs/jsx-a11y.js: disables the rule 'jsx-a11y/anchor-has-content' in favor of wordpress/a11y-interpolated-anchor, which wraps around the former to allow both checking regular JSX and not failing on interpolated elements.
a11y-interpolated-anchor.js adds the new wrapper rule. This rule is then enabled in the custom.js rules of the eslint-plugin.

Why?

When using anchors and createInterpolateElement together, we need to eslint-ignore the line to avoid running into the anchor-has-content error even if the anchor is valid.
This pull request introduces a wrapper around the rule so that we do not need to constantly ignore it.

How?

The new rule uses jsx-a11y/anchor-has-content under the hood to validate non-interpolated anchors (as normal). I considered just writing the rule from scratch, and I can still do that but as a first attempt, I decided it made sense to add a wrapper and not reinvent the linter rules for normal anchors.
The new rule also does some very basic checks for the potential validity of anchor tags used within createInterpolateElement. In particular, it checks that anchors are not empty.
The trade-off of having any linting at all for the interpolated anchor is that it could give a false sense of safety, but it would still miss issues (see the tests for some examples).

Testing Instructions

Testing Instructions for Keyboard

"Break" an anchor that is created with createInterpolateElement, by making it empty of content (e.g: <a> </a>)
- It should trigger a @wordpress/a11y-anchor-has-content linter error that says Anchors must have content and the content must be accessible by a screen reader. Check the first parameter to createInterpolateElement
Add some empty anchor with JSX like so <a href=""></a></>, without createInterpolateElement
- It should trigger a @wordpress/a11y-anchor-has-content linter error that says Anchors must have content and the content must be accessible by a screen reader
Run the tests for the eslint-plugin rules npm run test:unit -- packages/eslint-plugin/rules/__tests__/jsx-a11y-anchor-has-content.js. The a11y-anchor-has-content.js tests should pass (and all others).

github-actions · 2023-04-25T14:37:04Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @rowasc! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

packages/eslint-plugin/configs/custom.js

packages/eslint-plugin/rules/__tests__/a11y-anchor-has-content.js

rowasc · 2023-04-25T14:41:55Z

packages/eslint-plugin/rules/__tests__/a11y-anchor-has-content.js

+		   };`,
+		},
+		{
+			// ⚠️ This is invalid code that would get flagged by jsx-a11y/anchor-is-valid, but NOT by eslint-plugin-jsx-a11y/anchor-has-content or @wordpress/a11-anchor-has-content.


I added these so that it's obvious to readers what's covered/not covered (also because I was afraid of us forgetting the scope of the linter otherwise), but LMK if they feel irrelevant or if you have suggestions on how to improve them.

It’s also possible to add names for tests according to https://eslint.org/docs/latest/integrate/nodejs-api#ruletester. Maybe that would be a way to provide additional context instead of using inline comments.

Thank you so much, completely missed the name field. I've switched to using name in most cases.

packages/eslint-plugin/rules/__tests__/a11y-anchor-has-content.js

rowasc · 2023-04-25T14:45:51Z

packages/eslint-plugin/rules/a11y-anchor-has-content.js

+	schema: [], // no options
+};
+
+const getATagsContent = ( str ) => {


I could improve this with a tokenizer a la createInterpolate element and catch more complex cases (e.g unmatched tags) if we want to. Originally did that, but felt it was adding more complexity than needed and the lint rules ended up being a bit flaky (better to do a focused createInterpolateA11y type linter for that).

It’s better to start with something simpler and improve over time. The bigger the scope, the longer it takes to review and land. Everything is going to be better than disabling the rule everywhere the helper function is used.

rowasc · 2023-04-25T15:22:53Z

packages/eslint-plugin/configs/custom.js

@@ -7,6 +7,7 @@ module.exports = {
 		'@wordpress/no-global-active-element': 'error',


⚠️ Missing: Docs! which I completely forgot until now. Leaving as draft until I get a little time do write some.

https://github.com/WordPress/gutenberg/pull/50065/files?short_path=e4b51f2#diff-e4b51f2bcad9f627a441e4941e91e50987e0ece0d4e4873675c46daaca18ccbe some initial docs here, they cover all basic scenarios

packages/eslint-plugin/rules/a11y-anchor-has-content.js

packages/eslint-plugin/rules/__tests__/a11y-anchor-has-content.js

gziolo · 2023-04-28T21:41:59Z

This is solid work, and this PR only needs some final touches to bring it to the finish line. Excellent job tackling this issue. I see that one of the CI jobs errors while running npm run lint:js so there needs to be and edge case in the implementation that unit tests don't catch:

Run the linter on some code that uses // eslint-disable-next-line jsx-a11y/anchor-has-content ( where the anchor has content )
It should not have an jsx-a11y/anchor-has-content error.
Delete the ignore rule // eslint-disable-next-line jsx-a11y/anchor-has-content in that code
It should not have an jsx-a11y/anchor-has-content error nor a @wordpress/a11y-anchor-has-content

The old rule is disabled now, so it should be safe to remove all those comments from the code. Do I miss something here?

gziolo · 2023-04-28T21:46:42Z

@ciampo and @tyxla, you might be interested in helping to bring this PR to the finish line as it will improve the linting of WordPress components and have a tiny impact on having better a11y support that is about to be audited in #50166.

tyxla · 2023-05-02T11:41:07Z

Sure thing, thanks for the ping @gziolo!

@rowasc how can we help?

packages/eslint-plugin/rules/jsx-a11y-anchor-has-content.js

rowasc · 2023-05-07T23:01:58Z

@gziolo thank you so much for the review, and I am sorry for the delay here, I got back from my meetup and finally got a chance to get back to this today.

The old rule is disabled now, so removing all those comments from the code should. Do I miss something here?

No, not missing anything, sorry! I probably messed up the explanation, I will give it another go. Added the lint-ignore removal here now so that I can make sure they are all ✅ too.

⚠️ I see the unit tests for PHP were failing in the previous commit, but it seems like it's unrelated and probably fine now - will monitor and come back if they fail again.

@tyxla I would appreciate a review if you get a chance, especially after all the changes from today, but no rush at all. Other than that I will just follow your/all reviewers' direction as needed :) moving to "Ready to review" now that I added some docs.

tyxla

Lovely work, @rowasc! Really cool idea and nice work to wrap around the existing rule.

I've left a bunch of comments, but it's mostly code style and nothing major or blocking.

I think this already looks in a pretty stable state and the tests cover a great deal of the available scenarios. 🚀

packages/eslint-plugin/configs/jsx-a11y.js

packages/eslint-plugin/docs/rules/jsx-a11y-anchor-has-content.md

packages/eslint-plugin/rules/jsx-a11y-anchor-has-content.js

tyxla · 2023-05-08T14:12:11Z

packages/eslint-plugin/rules/jsx-a11y-anchor-has-content.js

+ * @param {*} tags     markup used in createInterpolateElement
+ * @param {*} tagNames names that we recognize as anchors, as configured in the eslint rules
+ * @return {Array} tagNames an array of the content of the <a> (or other) tags


We could vertically align the different sections for better readability. ESLint will usually complain, but then it is likely just not enabled here.

Thanks, agree. This is actually something I had to do because ESLint complained :( I had them aligned but it kept pestering me about it. Later I realized it was because the alignment rule is different if there is no description in the Doc block

#50065 (comment) Apparently it also gets messed up in other cases - added more context in the link

tyxla · 2023-05-08T14:13:10Z

packages/eslint-plugin/rules/jsx-a11y-anchor-has-content.js

+	const regex = new RegExp( `<(${ tagText })>(.*?)</(${ tagText })>`, 'g' );
+	const tagParts = [ ...tags.matchAll( regex ) ];
+	return tagParts.map( ( element ) => {
+		return element[ 2 ].trim();


Perhaps we actually intend to have the leading/trailing space?

yep, but in that case, it would not result in an empty anchor content (for the purposes of the rule, that's all that we care about) - however, the comment made me realize I can improve this abstraction a bit since it's not really clear what the intention is. Thanks!

As an example <a> Hello world </a> would be totally fine, because even if you remove trailing spaces you don't end up with an empty anchor, so the rule won't complain. However <a> </a> is not valid, as the result would be a '' after the trim, and the rule would complain at you.

tyxla · 2023-05-08T14:13:39Z

packages/eslint-plugin/rules/jsx-a11y-anchor-has-content.js

+		// An unknown function call in a translate function may produce a valid string
+		// but verifying it is not straight-forward, so we bail
+		if ( args.filter( Boolean ).length === 0 ) {
+			nodeStr = null;


Why is this necessary if we override it immediately on L91?

😬 this was just a mistake while refactoring and I didn't have a test to cover it, thanks for catching it
Added a quick scenario to cover it https://github.com/WordPress/gutenberg/pull/50065/files#diff-4c555e1042f88bdebf5cba43ba582ae78b721c7d64c1d0cc527ea1975ef191edR61

tyxla · 2023-05-08T14:15:00Z

packages/eslint-plugin/rules/jsx-a11y-anchor-has-content.js

+			const componentOptions = options.components || [];
+			const typeCheck = [ 'a' ].concat( componentOptions );
+			if (
+				typeCheck.filter( ( type ) => type === node?.name?.name )


Do we also need to filter out nullish values before we check the length?

I didn't think so, but I've added a check to be safe in case something is misconfigured, thanks for catching it

tyxla

Lovely work, @rowasc! Really cool idea and nice work to wrap around the existing rule.

I've left a bunch of comments, but it's mostly code style and nothing major or blocking.

I think this already looks in a pretty stable state and the tests cover a great deal of the available scenarios. 🚀

gziolo · 2023-05-08T15:20:45Z

No, not missing anything, sorry! I probably messed up the explanation, I will give it another go. Added the lint-ignore removal here now so that I can make sure they are all ✅ too.

Excellent. Removing all comments that disable the original rule with all the unit tests covering possible cases is the best way to validate correctness and all we need to move forward 👍

For coding styles, you might want to try running ESLint with --fix flag as it might help with some comments that @tyxla left 😀

…rg/questions/react/why-should-component-names-start-with-capital-letter

…t in two steps Before, we had a method that just returned the content of anchors and the actual validation was done by the main function in the linter. The problem there was that there wasn't a clear enough abstraction indicating the intent of the matcher that identified which tags were empty, causing confusion about it https://github.com/WordPress/gutenberg/pull/50065/files#r1187499276. Now, instead of getATagsContent we have validateAnchors which throws an error with the same code we report through eslint if anchors are empty or not valid. Co-Authored-By: Boro <[email protected]>

…hing useful :) - Fix issue where we were matching the wrong messageId for nodes with the latest refacotr

…ue, it won't be considered valid

…n translations

Before we were automatically using the e.message thrown by the validator, which we had looked into and should not throw but to be safe, added a clause to rethrow if the unexpected thing happens instead of using the eslint report for normal non lint issues

Co-authored-by: Marin Atanasov <[email protected]>

The previous samples were including more context than strictly necessary, making it harder to understand the point of the correct/incorrect usage scenarios. Hopefully this is minimal and still clear enough to be a good UX

There was so much clutter that it was hard to understand the tests. We now get the same benefits with less code _shrugs_

Instead of re-throwing when there is a problem in the linter code, we use the eslint reporter to tell the user that this is not their fault :) we also give them context and nudge them to report it as a bug. This way they don't spend time chasing down something that is not their problem. Co-Authored-By: Boro <[email protected]>

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Apr 25, 2023

rowasc commented Apr 25, 2023

View reviewed changes

packages/eslint-plugin/rules/a11y-anchor-has-content.js Outdated Show resolved Hide resolved

gziolo added [Tool] ESLint plugin /packages/eslint-plugin [Type] Enhancement A suggestion for improvement. labels Apr 26, 2023

bor0 reviewed Apr 27, 2023

View reviewed changes

packages/eslint-plugin/rules/a11y-anchor-has-content.js Outdated Show resolved Hide resolved

gziolo reviewed Apr 28, 2023

View reviewed changes

packages/eslint-plugin/rules/__tests__/a11y-anchor-has-content.js Outdated Show resolved Hide resolved

rowasc commented May 7, 2023

View reviewed changes

packages/eslint-plugin/rules/jsx-a11y-anchor-has-content.js Show resolved Hide resolved

rowasc marked this pull request as ready for review May 7, 2023 23:05

rowasc requested review from ajitbohra, ntwb, nerrad and michalczaplinski as code owners May 7, 2023 23:05

tyxla reviewed May 8, 2023

View reviewed changes

rowasc requested review from TimothyBJacobs, spacedmonkey, noahtallen, ObliviousHarmony, kevin940726 and ellatrix as code owners May 17, 2023 22:02

rowasc and others added 24 commits May 30, 2023 10:33

Fix naming of JSX component which was not correct per https://iq.js.o…

c6de48f

…rg/questions/react/why-should-component-names-start-with-capital-letter

Just add more context about the intention of the method

46435d4

- Return directly when we bail, the previous code would never do anyt…

3023c32

…hing useful :) - Fix issue where we were matching the wrong messageId for nodes with the latest refacotr

Add comment to the rule fn

7cb5267

if we ever misconfigure the custom component for eslint as falsy val…

2e6f89c

…ue, it won't be considered valid

Fix tests that called JSX with lowercase, add tests for expressions i…

7702533

…n translations

Bring package-lock back to where it was in trunk

3e36e9f

Apply suggestions from code review - docs' description section

f5b7e00

Co-authored-by: Marin Atanasov <[email protected]>

Apply suggestions from code review

aa0e818

Co-authored-by: Marin Atanasov <[email protected]>

Apply suggestions from code review

e2fe0d5

Co-authored-by: Marin Atanasov <[email protected]>

Apply suggestions from code review

01ab76f

Co-authored-by: Marin Atanasov <[email protected]>

Apply suggestions from code review

10a77eb

Co-authored-by: Marin Atanasov <[email protected]>

Apply suggestions from code review

2c6e894

Co-authored-by: Marin Atanasov <[email protected]>

Apply suggestions from code review

d46e07c

Co-authored-by: Marin Atanasov <[email protected]>

Readability & description improvement for anchor validation function

d026f54

Small readability improvements for getInterpolatedNodeText

e74e387

Shortened sample code in rules docs

7385d1c

The previous samples were including more context than strictly necessary, making it harder to understand the point of the correct/incorrect usage scenarios. Hopefully this is minimal and still clear enough to be a good UX

Minimize footprint of test code while retaining quality

90288e2

There was so much clutter that it was hard to understand the tests. We now get the same benefits with less code _shrugs_

Fix code style with --fix in docs

bb9a3b6

Test code style changed with --fix output for the more convoluted tests

51296fe

Fix throws tag with {Error} - run --fix eslint

7ca23bf

rowasc force-pushed the eslint/anchor-has-content-interpolated-compat branch from 5fb816e to 8bc1ad9 Compare May 30, 2023 13:36

Trunk's package-lock (resetted it)

ca01a04

ObliviousHarmony removed their request for review June 30, 2023 17:35

geriux removed their request for review February 26, 2024 09:51

michalczaplinski removed their request for review May 29, 2024 09:41

dcalhoun removed their request for review August 28, 2024 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap around eslint-plugin-jsx-a11y/anchor-has-content to support createInterpolateElement usage #50065

Wrap around eslint-plugin-jsx-a11y/anchor-has-content to support createInterpolateElement usage #50065

rowasc commented Apr 25, 2023 •

edited

Loading

github-actions bot commented Apr 25, 2023

rowasc Apr 25, 2023

gziolo Apr 28, 2023

rowasc May 7, 2023

rowasc Apr 25, 2023

gziolo Apr 28, 2023

rowasc Apr 25, 2023

rowasc May 7, 2023

gziolo commented Apr 28, 2023

gziolo commented Apr 28, 2023 •

edited

Loading

tyxla commented May 2, 2023

rowasc commented May 7, 2023

tyxla left a comment

tyxla May 8, 2023

rowasc May 17, 2023

rowasc May 18, 2023

tyxla May 8, 2023

rowasc May 17, 2023

tyxla May 8, 2023

rowasc May 17, 2023

tyxla May 8, 2023

rowasc May 17, 2023

tyxla left a comment

gziolo commented May 8, 2023

		@@ -7,6 +7,7 @@ module.exports = {
		'@wordpress/no-global-active-element': 'error',

Wrap around eslint-plugin-jsx-a11y/anchor-has-content to support createInterpolateElement usage #50065

Are you sure you want to change the base?

Wrap around eslint-plugin-jsx-a11y/anchor-has-content to support createInterpolateElement usage #50065

Conversation

rowasc commented Apr 25, 2023 • edited Loading

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

github-actions bot commented Apr 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Apr 28, 2023

gziolo commented Apr 28, 2023 • edited Loading

tyxla commented May 2, 2023

rowasc commented May 7, 2023

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyxla left a comment

Choose a reason for hiding this comment

gziolo commented May 8, 2023

rowasc commented Apr 25, 2023 •

edited

Loading

gziolo commented Apr 28, 2023 •

edited

Loading