-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: Inserter Search Results Not Announcing #10755
Fix: Inserter Search Results Not Announcing #10755
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks so much.
Tested this in Windows with Firefox + NVDA as well and it worked a treat. 👍
@@ -205,6 +205,15 @@ export class InserterMenu extends Component { | |||
itemsPerCategory, | |||
openPanels, | |||
} ); | |||
|
|||
const resultCount = Object.keys( itemsPerCategory ).reduce( ( memo, curr ) => memo + itemsPerCategory[ curr ].length, 0 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix this, but for future reference: please use full words for variables whenever possible. It's easier to read current
than curr
. I assume this should be current
, right? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tofumatt, I'll make sure to read over the documentation to make sure I follow the standards going forward! 👍
* added functionality to get the result count and announcer it in the live region * chore: Tweak variable names/readability * Sort props
@@ -205,6 +205,17 @@ export class InserterMenu extends Component { | |||
itemsPerCategory, | |||
openPanels, | |||
} ); | |||
|
|||
const resultCount = Object.keys( itemsPerCategory ).reduce( ( accumulator, currentCategorySlug ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coming to this way late, but in some recent refactoring I've come across remnants of this code. My first impression was: Why does this do a reduce
operation on itemsPerCategory
if filteredItems
array is already available much more directly (i.e. filteredItems.length
)? It then occurred to me that itemsPerCategory
can yield slightly different results, notably in how it filters out reusable blocks. But then again, it doesn't seem to be that we'd want reusable blocks to be excluded from the announced results.
In my testing, this is exactly what happens ("0 results found" when there's a reusable block search result present).
Ultimately, then, it would seem that filteredItems.length
would be both more direct and more accurate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ultimately, then, it would seem that
filteredItems.length
would be both more direct and more accurate?
Proposed for change at: #22279
Description
The inserter search results do not announce to screen readers the current number of filtered results. This implements some logic to calculate that number and announce to the user how many results have been found.
How has this been tested?
First and foremost, I typed in various strings of text to change the number of results. Then I made sure the text in the live region calculated the correct number and matched up with the actual count.
Then I used VoiceOver and Safari (there is a bug in other browsers where aria-live doesn't work) to test that the value of the aria-live region is read to a screen reader. At least from my initial testing everything seemed to work correctly.
Screenshots
Types of changes
Makes the inserter more accessible by fixing this issue: #10583
Checklist: