-
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
Changes from 9 commits
433d8c8
92b55a4
acf87e8
e7a7e14
764581f
f487087
f8b3f2c
76fe327
81450c6
7f3dcb3
d3294f7
8d7ebda
e83fde4
f9daad9
fbeba8c
4bbd25b
95166be
3199fab
1d752c9
d3d17f8
cd4bb11
1836bf6
20038ca
4c7809c
db6a23a
a781649
4cb4ad7
4400544
a55b735
ac1efc5
ec967ea
a39aef6
8ba8904
ef0a086
a036757
0436928
121b7d8
ab3f0a3
ab049ed
01320c3
47aeee4
2077aac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,8 +26,9 @@ import { | |
import { withDispatch, withSelect } from '@wordpress/data'; | ||
import { compose } from '@wordpress/compose'; | ||
import { addAction, removeAction, hasAction } from '@wordpress/hooks'; | ||
import { getBlockType } from '@wordpress/blocks'; | ||
import { getBlockType, getUnregisteredTypeHandlerName } from '@wordpress/blocks'; | ||
import { BlockEdit } from '@wordpress/block-editor'; | ||
import { sprintf, _x } from '@wordpress/i18n'; | ||
|
||
/** | ||
* Internal dependencies | ||
|
@@ -197,6 +198,28 @@ export class BlockHolder extends React.Component<PropsType, StateType> { | |
); | ||
} | ||
|
||
getAccessibilityLabel() { | ||
const { title, attributes, name } = this.props; | ||
|
||
if ( name === getUnregisteredTypeHandlerName() ) { // is the block unrecognized? | ||
return title; //already localized | ||
} | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Should this be |
||
title, //already localized | ||
); | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. I like that this give us a good default label. 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 commentThe 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?
|
||
} | ||
|
||
return blockName; | ||
} | ||
|
||
render() { | ||
const { isSelected, borderStyle, focusedBorderColor } = this.props; | ||
|
||
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it doesn't seem to work on android though... |
||
accessibilityLabel={ this.getAccessibilityLabel() } | ||
accessibilityRole={ 'button' } | ||
onPress={ this.onFocus } > | ||
|
||
<View style={ [ styles.blockHolder, borderStyle, { borderColor } ] }> | ||
|
@@ -243,6 +267,7 @@ export default compose( [ | |
const isSelected = isBlockSelected( clientId ); | ||
const isFirstBlock = order === 0; | ||
const isLastBlock = order === getBlocks().length - 1; | ||
const title = getBlockType( name ) !== undefined ? getBlockType( name ).title : ''; | ||
|
||
return { | ||
attributes, | ||
|
@@ -254,6 +279,7 @@ export default compose( [ | |
isLastBlock, | ||
isSelected, | ||
name, | ||
title, | ||
}; | ||
} ), | ||
withDispatch( ( dispatch, { clientId, rootClientId } ) => { | ||
|
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.