-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
New: output block position for screen readers. Update: refactor the way the screen reader information on block button is delivered. #29060
New: output block position for screen readers. Update: refactor the way the screen reader information on block button is delivered. #29060
Conversation
…new/output-block-position-sr
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @alexstine! 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. |
Absolutely. Should have to begin with. I want to prefix this by saying this is not something I think needs to be fixed, but visually looks odd. Which clearly doesn't have precedence here. But I don't know, which is why i bring it up. First, a bit about VoiceOver. When VoiceOver is running it shows a floating text box with the text that it is reading aloud. When VoiceOver's settings are set to "high verbosity" or "medium verbosity" the punctuation and letter-casing given don't follow grammatical rules with the label as it stands in this patch. VoiceOver defaults to high verbosity. What happens is In the document outline on a post or page, VoiceOver adds text to the end of the label. The text is written like the label text doesn't have ending punctuation, adding things like a comma immediately after or additional text in parenthesis. The important bit is that VoiceOver reads it fine. It's only the phrase shown by VoiceOver in the text box which has anything "wrong" with it, with something like "Paragraph Block, period, comma, To reproduce (if it's even needed), start VoiceFlow with the verbosity set to Medium or High. In WordPress, navigate to a new page or post then tab to the document outline and press space to open, and navigate down the items in the list. When VoiceFlow processes each item in the list it sounds fine, e.g. "paragraph block, selected block" but shows "Paragraph block, period, open parenthesis, (text in lowercase) selected block, close parenthesis, comma, button. Now that I've gone on for too long for something which is probably a moot point and I don't feel strongly about, if action is needed, probably removing the period at the end would be fine. Thanks for your patience! |
…new/output-block-position-sr
Hmm, I tested with similar settings on Mac Voiceover and could not replicate. How weird. Maybe there is something I disabled because Voiceover was talking too much for me. Anyway, I appreciate the report. Let's see if anyone else can figure out how to replicate. I still have my doubts due to how Voiceover handles the reading of aria-describedby, but at the same time, I do not know a better way to pull this off. :( Thanks. |
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 for working on this! I tested with VoiceOver/Safari (default verbosity settings, I never changed them at all) and the descriptions are read out correctly after the labels, with a slight pause in between.
The code looks good, apart from a couple of issues I left comments about. Would be good to get this tested by someone experienced with speech-to-text software; is there anyone in the accessibility team that can help out with that?
@tikifez You seem to be testing List View. This PR modifies the descriptions in Navigation Mode, which can be entered by pressing the 'escape' key when you have a block focused. |
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 for your contribution @alexstine!
I left a couple of comments, but nothing major. So far its looking good.
There are some end to end tests that need to be fixed—previously these just tested the label which contained the positional information. I think it'd be good to expand these to test the described-by element now so that there's not a loss in test coverage. Is that something you're able to tackle @alexstine? If not I may be able to help with that next week.
packages/block-editor/src/components/block-list/block-selection-button.js
Outdated
Show resolved
Hide resolved
@@ -161,19 +161,24 @@ export function getBlockLabel( blockType, attributes, context = 'visual' ) { | |||
* @param {Object} attributes The values of the block's attributes. | |||
* @param {?number} position The position of the block in the block list. | |||
* @param {string} [direction='vertical'] The direction of the block layout. | |||
* @param {number} total The total number of blocks. | |||
* @param {string} parentTitle In the event of a child block, this param gets the title of the parent block. | |||
* | |||
* @return {string} The block label. | |||
*/ | |||
export function getAccessibleBlockLabel( |
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'm a little surprised there aren't unit tests for this function. Probably my fault as I think I initially wrote it.
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.
Oh, just realise there are unit tests. So these will need to be updated, with new test cases added for the additional branches of code. Happy to help with that, either directly or by providing guidance, let me know.
There are docs for testing, but not sure how good they are.
There's another implication of this change I just noticed. I think we could also consider renaming getAccessibleBlockLabel to getAccessibleBlockDescription, but happy to make that a separate change so that we can avoid expanding the scope of this. |
packages/block-editor/src/components/block-list/block-selection-button.js
Outdated
Show resolved
Hide resolved
packages/blocks/src/api/utils.js
Outdated
direction = 'vertical', | ||
total, | ||
parentTitle |
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.
This function is used in a React Native file, which will need updating as well:
gutenberg/packages/block-editor/src/components/block-list/block.native.js
Lines 191 to 195 in da8555d
const accessibilityLabel = getAccessibleBlockLabel( | |
blockType, | |
attributes, | |
order + 1 | |
); |
The new total
parameter here I understand corresponds to the block count, which comes from:
export const getGlobalBlockCount = createSelector( |
…new/output-block-position-sr
When I apply the PR and try to SyntaxError: gutenberg\packages\block-editor\src\components\block-list\block-selection-button.js: I did run npm install first just in case. |
@carolinan I fixed it. Looks like a small regression/bug from when I merged trunk in to my branch, it created another variable with the same name conflicting constants. All fixed up now. |
@talldan Removing the title change seemed to be the best idea for this at least for the moment. Could always re-visit it in the future for a smaller change. I believe what I've done here has addressed most concerns. The only issue still making the tests fail is unit tests. As much as I would like to take on unit tests for this, I just don't have the time to learn about JS unit tests at the moment. Can you help me find someone who may be interested in fixing a few issues with the unit tests? Much appreciated, we're almost there with this. I will work on adding the changes to the .native file later this week. Thanks. |
So much has changed since I opened this PR. I'm gonna close it out for now and revisit this later down the road. I learned a lot with the first implementation but I think a bigger problem needs to be tackled first. |
Description
Fixes #24556
How has this been tested?
I tested this using NV Access on Windows 7 using Firefox.
Screenshots
Types of changes
Bug Fix and New Feature.
Checklist: