-
Notifications
You must be signed in to change notification settings - Fork 56
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
Experiment: Move unselected block accessibility handling to block-holder #937
Experiment: Move unselected block accessibility handling to block-holder #937
Conversation
Generated by 🚫 dangerJS |
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 and works great, nice job @Tug ! 🎉
I added a few comments, mostly ideas/questions.
cc @JavonDavis . I wonder how this could impact UITests, but they seems to be passing anyway ✅
|
||
if ( name === getUnregisteredTypeHandlerName() ) { // is the block unrecognized? | ||
return title; //already localized | ||
} |
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 could add an accessibility label to missing/index.js
, in a way that VoiceOver can tell the type of block:
i.e: "Unsupported block. Gallery", "Unsupported block. Unknown"
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 don't mean to add accessibility labels to missing
block on this PR, but just saying that it might not need an special handling.
src/block-management/block-holder.js
Outdated
const blockType = getBlockType( name ); | ||
if ( blockType.getAccessibilityLabel ) { | ||
const blockAccessibilityLabel = blockType.getAccessibilityLabel( attributes ) || ''; | ||
return blockName + ( blockAccessibilityLabel ? '. ' + blockAccessibilityLabel : '' ); |
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 like that this give us a good default label.
I was thinking if it's worth it to return the whole sentence from the block's blockType.getAccessibilityLabel
directly (including the "{Block Name} block..." part, and construct this default just when blockType.getAccessibilityLabel
is not implemented.
This could make sentences easier to localize, could give us more flexibility if it's needed, but it also could be more error prone (someone could forget to add the "{Block Name} block." at the beginning).
Do you think that this approach would be beneficial?
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 am aiming towards to the current approach because it brings consistency. With this, the description for all blocks starts with 'X block. '
on the other hand I think the '. ' and concatenations needs a revisit as we discussed earlier for localization issues. would it be better to do this as follows?
sprintf(
/* translators: accessibility text. 1: block name. 2: block content information. */
_x( '%1$s block. %2$s', 'Accessibility text for a block' ),
blockName,
blockAccessibilityLabel
);
@@ -206,8 +229,9 @@ export class BlockHolder extends React.Component<PropsType, StateType> { | |||
// accessible prop needs to be false to access children | |||
// https://facebook.github.io/react-native/docs/accessibility#accessible-ios-android | |||
<TouchableWithoutFeedback | |||
accessible={ false } | |||
accessibilityLabel="block-container" | |||
accessible={ ! isSelected } |
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 like this, it makes more sense to only make it false when it's selected and we'll only interact with the inner elements when it's selected anyway
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.
Yes, it doesn't seem to work on android though...
It shouldn't and didn't 🙂Since the |
…ected-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.
Looks great - @Tug ! 🎉
I think we still can improve how the unsupported block is handled, but as we talked, this is a step solution into a one more robust.
I'd like to experiment having this kind of labeling for the unsupported block:
But I believe we can do it on a separated PR.
src/block-management/block-holder.js
Outdated
|
||
const blockName = sprintf( | ||
/* translators: accessibility text. %s: block name. */ | ||
_x( '%1$s block' ), |
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.
Should this be __
instead of _x
?
And since there's just one variable, %d
should be enough?
…e accessibility attribute
* Include Node debug option for IntelliJ
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.
The comments I left along with the changed behavior of getAccessibilityLabel
I think are causing the iOS tests to fail on CI... I branched off onto to #1041 to test this out and the tests pass on CI for iOS there, I think the getAccessibilityExtra
call isn't working as expected.
src/block-management/block-holder.js
Outdated
<View style={ [ styles.blockHolder, borderStyle, { borderColor } ] }> | ||
{ this.props.showTitle && this.renderBlockTitle() } | ||
<View | ||
accessibile={ true } | ||
accessible={ ! isSelected } |
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.
Is there a specific reason this is here and not on the TouchableWithoutFeedback
? That's the container whose accessibility value would need to be changed to allow inner elements to be accessible
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.
Yeah I wanted to basically group all the accessibility props together and View already had the accessibilityLabel
prop.
The idea is that both TouchableWithoutFeedback
and the View
are containers of our block so both have the potential to be accessible elements. I did some test and I'm not sure it changes things much where we put 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.
I did some test and I'm not sure it changes things much where we put it.
Oh, I played around with it too and could've sworn it was affecting some things for me but I was playing with a few things and I don't remember for certain, let me try again and see if I can reproduce again here
src/block-management/block-holder.js
Outdated
accessibilityLabel={ accessibilityLabel } | ||
accessibilityRole={ 'button' } |
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're forcing the inner container here to be treated like a button, was there an additional benefit of this? It's already surrounded by a Touchable
which I think's enough
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 only meant for the screenreader. Adding accessibilityRole={ 'button' }
will translate to the screenreader saying "button" at after reading the accessibilityLabel
. Meaning the user can interact on it like a normal button (ouble tap to select the block in this case)
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.
Hey @Tug 👋 I tested this and currently it is not saying "button" after reading accessibilityLabel
. But when I move accessibilityRole={ 'button' }
to TouchableWithoutFeedback
level it does say it:
<TouchableWithoutFeedback
onPress={ this.onFocus }
accessible={ ! isSelected }
accessibilityRole={ 'button' }
>
So we might want to update this like that.
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.
Indeed it does that on iOS when the block is unselected. I was able to confirm that moving accessibilityRole="button"
to TouchableWithoutFeedback
fixes it on both platforms. However, I also I noticed that it says "button" first before reading the view's accessibility label on Android so I moved everything to TouchableWithoutFeedback
and now it works as expected!
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.
It looks like CI tests are failing this time. I have an idea, we can just set an accessibilityHint
rather than an accessibilityRole and keep the accessibilityLabel where it was.
<TouchableWithoutFeedback
onPress={ this.onFocus }
accessible={ ! isSelected }
accessibilityHint={ __('Double tap to focus') }
>
<View style={ [ styles.blockHolder, borderStyle, { borderColor } ] }>
{ this.props.showTitle && this.renderBlockTitle() }
<View
accessibilityLabel={ accessibilityLabel }
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.
Another option would be ofc updating the CI tests according to the current 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.
Tried to debug the e2e tests but as I was wasting too much time on this I ended up reverting the change in this commit.
I guess it's not too critical to have "button" mentioned before the accessibility label on android. We have many improvements to do on accessibility anyway.
@pinarol could you give it a final check please :)?
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.
At least the issue is resolved on iOS, now I hear: "Paragraph block. Row n. Text. Button" 👍 Thanks
This would be annoying because it's the main change of this PR. I've checked again and it seems to be working. Could you expand on what's not working here? Anyway it's green now thanks to your commit a55b735 Thanks a lot @JavonDavis 🙇 |
@Tug Sorry for taking a little while to clarify what I meant here... When I was testing this locally and running the e2e tests the code in the |
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 tested the PR and it seems OK overall. Just one thing: the accessibilityRole
is not read. I added a comment about it in the related line. After fixing that I think we are good to go.
…ected-accessibility-use-settings
…ected-accessibility-use-settings
@Tug My bad, it seems the issue I was seeing was related to something else, |
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! 🎉
…ected-accessibility-use-settings
Alternative to #936
Gutenberg PR WordPress/gutenberg#15225
It updates e2e tests as well which were broken with this updates (we rely on the accessibility label to find blocks).
Testing Instructions
For instance:
An heading block should have all the following information spoken:
"Heading Block. Row X. . Button"
A paragraph block should have all the following information spoken:
"Paragraph Block. Row X. . Button"
An empty image block should have all the following information spoken:
"Image Block. Row X. Empty. Button"
A non-empty image block with a caption should have all the following information spoken:
"Image Block. Row X. . Button"