-
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
[Mobile] Move unselected block accessibility handling to block-holder (v2) #15225
[Mobile] Move unselected block accessibility handling to block-holder (v2) #15225
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.
Looks super nice to me!
Just added a small comment about Empty Image block.
|
||
return sprintf( | ||
/* translators: accessibility text. 1: image alt text. 2: image caption. */ | ||
_x( '%1$s. %2$s', 'Image block accessibility text: alt, caption' ), |
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.
We are missing the empty state for this block. It was previously given by MediaPlaceholder
, but now it is blocked by BlockContainer
.
We might want to check url
to return the empty image block label.
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.
Ok adding it 👍
Btw, I don't see the point in having just %s in a translatable string, I'll just concatenate here.
@@ -31,6 +32,30 @@ const supports = { | |||
export const settings = { | |||
title: __( 'Heading' ), | |||
description: __( 'Introduce new sections and organize content to help visitors (and search engines) understand the structure of your content.' ), | |||
getAccessibilityLabel( attributes ) { |
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 is concerning as it isn't part of the public API for block registration. It would be used only by the mobile app only, 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.
This I think could be a good/easy way for 3rd parties to add a simple descriptive accessibility label to the blocks that will be read by mobile screen readers.
We are also adding a default label for when this is not implemented.
It would be awesome if web could also use it, if it doesn't then yes, it's only for mobile apps.
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.
You need to keep in mind that block registration is heavily used by 3rd party developers. That's why every change proposed goes through RFC (Request For Comments) process. You can check #13693 for reference.
One way of providing this functionality is to temporary use "unstable" prefix as described here:
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.
Thank you for the explanation!
It looks like the __experimental
prefix will fit well. We can implement it on mobile, and if it proves to be a good enough, we can do a proper RFC.
Does this sound good?
cc @Tug
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.
Sorry for the confusion, I didn't intend to have it merged as it is, my intent was merely to experiment on an alternative to #15214 which passes a function into the block attributes. I don't currently have a solution for this, but I'm open to suggestions.
A prefix could work yeah if that's what gutenberg devs recommend.
A RFC could be another approach if we think it's useful for the web as well.
At the time I wrote this I was thinking about either using the blocks.registerBlockType
hook to add our custom property or splitting index.js into a native and web version but those aren't very good.
1930c5f
to
0b7ac5a
Compare
0b7ac5a
to
57c3d42
Compare
57c3d42
to
aa8f662
Compare
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 works great, but it seems like we might need more branching from web.
As we talked, this is meant to be a step into a more robust solution.
I'd say let's go with this, while we work on that solution, considering the future of how we want to navigate through blocks (with keyboard and Screen Readers).
…elected-accessibility-use-settings
…elected-accessibility-use-settings
…elected-accessibility-use-settings
…elected-accessibility-use-settings
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.
LGTM! 👍
Description
This is an alternative to #15214
This PR aims to introduce a new way to describe the overall state of a block with a screen reader running on native mobile.
When a user has a screen reader running and wants to navigate through the list of block we will want the screen reader to state for each block:
For this we introduce an experimental property at the block settings level:
__experimentalGetAccessibilityLabel
. It would handle giving the last 2 items (status and description), the first one (type) can be automated at theBlockListBlock
level.This feature might probably be dropped if we decide to revert our "hierarchical" navigation of blocks on mobile and unify with Gutenberg web. That would mean first being able to "auto select" a block when navigating through it (might not be trivial on mobile).
Testing Instructions
See wordpress-mobile/gutenberg-mobile#937