Polish a11y package. #21148
Polish a11y package. #21148
Conversation
|
Size Change: +19 B (0%) Total Size: 857 kB
ℹ️ View Unchanged
|
378506e to
1f39e28
Compare
|
I decided to remove the stricter type checking from this PR in order to get the rest of the improvements in sooner and deal with the tricky stuff in another PR. Unfortunately, a Paragraph-Quote transform e2e test is failing for no apparent reason. There seems to be a lot of issues with the e2e tests lately; hopefully those get fixed soon. Anyway, this is ready for review. |
1f39e28 to
b5f381c
Compare
| * | ||
| * @return {HTMLDivElement} The ARIA live region HTML element. | ||
| */ | ||
| const addContainer = function( ariaLive ) { | ||
| ariaLive = ariaLive || 'polite'; |
There was a problem hiding this comment.
Technically this would previously fall back to 'polite' for any falsy value given (null, false, 0, etc), and now it will only fall back when explicitly undefined (omitted). Generally speaking, I think this is a fine approach, but it may be something to consider for potential side-effects.
There was a problem hiding this comment.
Yeah, I generally lean towards enforcing that functions only be called with certain types, so in this case I'd consider calling this function with anything other than one of the 2 valid strings or undefined to be an error that should be caught by the linter/compiler.
I initially tried to use a type union of ('assertive'|'polite') as the ariaLive JSDoc type, but despite something similar appearing as an example in our docs, the auto-generated docs weren't handling it properly and was just outputting (|). I'm going to make another PR for that change later and see if anyone can figure out how to fix the issue.
There was a problem hiding this comment.
I initially tried to use a type union of
('assertive'|'polite')as theariaLiveJSDoc type, but despite something similar appearing as an example in our docs, the auto-generated docs weren't handling it properly and was just outputting(|)I'm going to make another PR for that change later and see if anyone can figure out how to fix the issue.
Yeah, this is related to the issue described at #18045, and tangentially #19952 (#19952 (comment)).
packages/a11y/src/clear.js
Outdated
| const regions = Array.from( | ||
| document.getElementsByClassName( 'a11y-speak-region' ) | ||
| ); | ||
| for ( const region of regions ) { | ||
| region.textContent = ''; | ||
| } |
There was a problem hiding this comment.
Is Array.from necessary? As far as I can tell, getElementsByClassName is iterable without needing to be coerced to an array.
Apparently there are some interesting "gotchas" when using for ... in, not sure if those apply to for ... of.
I guess, generally speaking, I don't consider the previous implementation to be very problematic, at least enough that we'd do this dance of converting to an array just to use for ... of.
There was a problem hiding this comment.
@aduth getElementsByClassName returns an HTMLCollection, which is not iterable. You may be thinking of a NodeList, which is iterable; that's the type returned from other functions like querySelectorAll. It's one of those weird inconsistencies caused by different browser APIs being created several years apart.
I went with getElementsByClassName because it's faster than querySelectorAll.
for ... of is essentially just the no-gotcha version of for ... in. The for ... of in JS is equivalent to the for ... in or foreach in most other languages. It's one of those cases where JavaScript implemented something weirdly early on (e.g. var), resulting in a non-weird alternative being added later (e.g. let).
I could switch back to querySelectorAll and then I wouldn't have to convert to an array before using for ... of. Alternatively, I could keep the switch to getElementsByClassName, but go back to the old for loop, and that also wouldn't require an array conversion. Which do you prefer? Personally, I prefer keeping the for ... of loop simply for ease of reading.
There was a problem hiding this comment.
I don't have a strong preference. It's fine.
But if part of the argument is that getElementsByClassName is more performant, I might imagine (not verified) that the cast to an Array type is possibly counteracting part of any benefit we'd get.
There was a problem hiding this comment.
I tried to switch to querySelectorAll and use the for ... of loop on that, but the TypeScript linting said that it wasn't iterable. And, because we're targeting ES5, that is correct. There is a TypeScript compiler option called downlevelIteration that allows you to use the for ... of syntax for ES5 targets by rewriting it to a standard for loops upon compilation.
However, that seems like a bit much effort for this one little thing, so I've decided to just revert to using an old-fashioned for loop while keeping the switch to getElementsByClassName.
There was a problem hiding this comment.
Actually, looking closer, it looks like we're actually targeting ES6? Argh, maybe I'll investigate this later, but I think this PR is good to go.
There was a problem hiding this comment.
ES6 should be available in this module, yes. There's a few where we're running code directly, but those are typically not ones we're running the browser.
You can tell based on the main of the package's package.json. If it's pointing to a file in build, then it can be assumed to be safe to use ES6:
gutenberg/packages/a11y/package.json
Line 21 in b4bfbb7
If there's any issues from the tooling in using ES6 in these packages, that might be something we'd want to look into.
There was a problem hiding this comment.
To be clear, what I meant by "targeting ES6" was that it appeared that this package was compiling to ES6, so using for ... of with querySelectorAll would not need to be rewritten to run in the browser.
But looking again, I can't remember what I saw that made me think the package was targeting an ES6 output. Maybe something changed recently, or maybe I just misread something earlier. Oh well. 🤷♂️
b5f381c to
cc70110
Compare
cc70110 to
3271da3
Compare
Description
This PR is a collection of small code quality improvements to the
a11ypackage to make the code easier to comprehend, more efficient, more consistent, and more compliant with the WordPress Coding Standards.