Skip to content
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

Speak 'Block moved up/down' after using keyboard actions to move up/down #64966

Merged

Conversation

n2erjo00
Copy link
Contributor

@n2erjo00 n2erjo00 commented Sep 1, 2024

What?

Describing to screen reader what just happened when user uses keyboard short cuts to move block up or down.

Why?

Closes #61168

Issue was reported by the linked issue.

How?

Using speak function to speak up after operation is done.

Testing Instructions

  1. Open editor and insert two or three blocks. Blocks don't need be anything fancy
  2. Open up screen reader of your choice and select one of the blocks. Move it up with key board shortcut
    • Mac: ⌘ + ⌥ + ⇧ + T or Y
    • Windows: CTRL + ALT + SHIFT + T or Y

Testing Instructions for Keyboard

Screenshots or screencast

@n2erjo00 n2erjo00 requested a review from ellatrix as a code owner September 1, 2024 12:54
Copy link

github-actions bot commented Sep 1, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: n2erjo00 <n2erjo00@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: afercia <afercia@git.wordpress.org>
Co-authored-by: alexstine <alexstine@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: talksina <talksina@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@t-hamano t-hamano self-requested a review September 1, 2024 13:54
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

Simply adding a message may convey incorrect information to the screen reader, because the message will always be read when the shortcut is executed.

For example, the following scenarios need to be considered:

  • When a block is at the beginning of the content, it cannot be moved forward
  • When a block is at the end of the content, it cannot be moved backward
  • Multiple blocks may be selected
  • There may be only one block in the content
  • In a flex layout, blocks are moved left and right, not up and down

Also, I thinks the message Block moved {up|down}. alone does not convey what changes have been made to the content. For example, a message like Move 2 blocks from position 3 right by one place is needed.

I think we can use the getBlockMoverDescription() function to cover all of these scenarios.

@t-hamano t-hamano added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Accessibility Feedback Need input from accessibility [Package] Block editor /packages/block-editor labels Sep 1, 2024
@n2erjo00
Copy link
Contributor Author

n2erjo00 commented Sep 1, 2024

Thanks @t-hamano for the feedback. I'll be taking a look next week

@n2erjo00 n2erjo00 requested a review from t-hamano September 28, 2024 13:59
@t-hamano
Copy link
Contributor

t-hamano commented Oct 2, 2024

Thanks for the update!

In my opinion, there is no need to make the processing an external variable. We can probably do it like this:

export default function BlockTools( {
	children,
	__unstableContentRef,
	...props
} ) {
	// ...

	function onKeyDown( event ) {
		// ...
		if (
			isMatch( 'core/block-editor/move-up', event ) ||
			isMatch( 'core/block-editor/move-down', event )
		) {
			const clientIds = getSelectedBlockClientIds();
			if ( clientIds.length ) {
				const direction = isMatch( 'core/block-editor/move-up', event )
					? 'up'
					: 'down';
				event.preventDefault();
				const rootClientId = getBlockRootClientId( clientIds[ 0 ] );
				// Here's the logic
				if ( direction === 'up' ) {
					moveBlocksUp( clientIds, rootClientId );
				} else {
					moveBlocksDown( clientIds, rootClientId );
				}

				const description = getBlockMoverDescription(
					// parameters
				);
				speak( description );
			}
		} else if ( isMatch( 'core/block-editor/duplicate', event ) ) {
			// ...
		}
	}
	// ...
}

This will also allow us to standardize some of the duplicate processing between move-up and move-down.

@n2erjo00
Copy link
Contributor Author

n2erjo00 commented Oct 2, 2024

@t-hamano Good point much cleaner that way. I implemented the changes

@t-hamano
Copy link
Contributor

t-hamano commented Oct 4, 2024

@n2erjo00 Thanks for the update!

I'm sorry, I missed an important point. getBlockMoverDescription() does not tell us the location of the moved block, but where the block is being moved to.

Perhaps we need to build a new logic for shortcuts or add a new argument like isMoved to getBlockMoverDescription().

Also, I'm trying to check in this comment whether the current block mover reading itself is correct.

Sorry for keeping you waiting, but I'd like to think of an ideal approach.

@afercia
Copy link
Contributor

afercia commented Oct 7, 2024

Just noting a previous attempt to add audible messages was tried in #24894 more than 4 years ago.

I'm not sure these messages should use getBlockMoverDescription() by altering the position count. That's meant to be used for the aria-describedby attribute and it would be best to separate the concerns. #24894 tried to replicate the existing logic and provide meaningful messages for all the cases.

We could consider to simplify the message but I think two different cases should be considered:

Rationale:

  • As an user, when using the buttons I already know what is going to happen. The description provides me a meaningful information, for example:
    • Move %1$s block from position %2$d down to position %3$d
    • Move %1$s block from position %2$d left to position %3$d
    • Block %1$s is at the end of the content and can’t be moved down
  • In this case, I don't need to hear again Block moved to position %2$d down to position %3$d etc. I only need a confirmation e.g. 'Block moved'. Repeating the new position information in the message would be noisy.
  • Instead, when using the keyboard shortcuts, I don't have any information about the initial position. In this case, the confirmation message should provide the new position information.

I do realize distinguishin the two cases is more complex. If it turns out to be too comples, I'd have bo objections with a somehow noisy, redundant message for the buttons.

@n2erjo00
Copy link
Contributor Author

Would the proper course of action to be to create similar function as getBlockMoverDescription (maybe getBlockSpeackDescription working title) but it would return simplified message: "Block {name} moved {n} steps {direction}" and this PR point of view the return message is then passed to speak function

CC @t-hamano @afercia

@afercia
Copy link
Contributor

afercia commented Oct 11, 2024

https://github.com/WordPress/gutenberg/pull/24894/files was actually using a dedicated function to provide detailed confirmation messages. It was a little overkill maybe.

@alexstine @talksina from your perspective, when a block is moved would you prefer a detailed confirmation message that announces the new position and direction, which may get a little noisy, or a shorter message? Examples:

  • Detailed message: Block Paragraph moved 1 step down.
  • Short message: Block moved.

@alexstine
Copy link
Contributor

The short message is probably okay considering even "1 step down" really doesn't communicate exactly where the block is.

@n2erjo00
Copy link
Contributor Author

@t-hamano Added short confirmation message after block has been moved up|down

CC @afercia @alexstine

@t-hamano
Copy link
Contributor

@afercia @alexstine Thanks for the feedback! So let's use a short message here.

@n2erjo00 I think two changes are needed to make the message more precise:

  • End with a period
  • Consider singular and plural
const message = sprintf(
	// translators: %d: the name of the block that has been moved
	_n(
		'%d block moved.',
		'%d blocks moved.',
		clientIds.length
	),
	clientIds.length
);
speak( message );

The code is similar to here.

@n2erjo00
Copy link
Contributor Author

@t-hamano Added period and logic for singular/plural

@t-hamano t-hamano self-requested a review November 1, 2024 13:14
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! It seems to work as expected.

The video below shows how NVDA reads it out loud:

0640d4d865b75b3c736678a7b32e3e66.mp4

@t-hamano t-hamano added the [Type] Enhancement A suggestion for improvement. label Nov 1, 2024
@t-hamano
Copy link
Contributor

t-hamano commented Nov 1, 2024

Note: I would like to close #24894, but add the people who worked on it as props.

Co-authored-by: enricobattocchi <lopo@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: mtias <matveb@git.wordpress.org>

@t-hamano t-hamano merged commit 98f6210 into WordPress:trunk Nov 1, 2024
66 of 67 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.7 milestone Nov 1, 2024
karthick-murugan pushed a commit to karthick-murugan/gutenberg that referenced this pull request Nov 13, 2024
…own (WordPress#64966)

* Speak 'Block moved up/down' after using keyboard actions to move up/down

* Created internal helper function to create description for speak function

* Refactor away from helper function and simplify

* Add short confirmation message after block has been moved up|down

* End with period. Speak singular or plural

* Speak the amount of moved blocks

Co-authored-by: n2erjo00 <n2erjo00@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: afercia <afercia@git.wordpress.org>
Co-authored-by: alexstine <alexstine@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: talksina <talksina@git.wordpress.org>
Co-authored-by: enricobattocchi <lopo@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: mtias <matveb@git.wordpress.org>
@n2erjo00 n2erjo00 deleted the announce-move-actions-to-screen-reader branch November 13, 2024 11:12
@talksina
Copy link

I have to report an issue - at least in Italian version.
When moving blocks with keystrokes - ctrl+alt+t up, ctrl+alt+y down, it just says "one block moved". Nothing else.
I'd expect my screen reader to say "block 1 moved down to row 2"
and if I move it further it would say "block 2 moved down to row 3"... same for moving upwards.
"block moved" as announcement is meaningless.

@talksina
Copy link

I have reported it on #61168 issue some months ago and I'm pleased it has been worked on, but I hope the notification will be made more detailed.

@afercia
Copy link
Contributor

afercia commented Nov 22, 2024

HI @talksina thanks for your feedback. as @t-hamano mentioned on the related issue #61168 (comment), in a previous comment on this pull requests #64966 (comment) whether to use a shorter message or a more informative one was discussed. The shorter message option prevailed. However, maybe it's worth reconsidering. One of the assumption was:

When using the buttons in the block toolbar, the button provides a description with the position information. For example, the aria label and description would be:

  • label: Move down
  • description: Move Paragraph block from position 1 down to position 2

As such, in this specific case, the position information is provided before moving the block. The shorter confirmation message would make sense in thei case. However, I think a message isn't provided at all when using the buttons.

Instead, when using the keyboard shortcuts, there's no information about the starting position. There's only the short confirmation message

@afercia
Copy link
Contributor

afercia commented Nov 22, 2024

One more thing to fix I just noticed: when a block can't be moved because:

  • it's at the first position and can't be moved up
  • it's at the last position and can't be moved down
  • it's the only block

The keyboard shortcuts are still triggered and the speak message still announces "Block moved" even though it didn't.
I will create a new issue to summarize all the remaining points to address.

@afercia
Copy link
Contributor

afercia commented Nov 22, 2024

New issue created: #67237

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Accessibility Feedback Need input from accessibility [Package] Block editor /packages/block-editor [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Moving blocks via keyboard: screen readers don't notify operation done
5 participants