-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Address PR feedback on block labels #19664
Conversation
LGTM. |
78cea2d
to
2862a18
Compare
I've rebased to incorporate changes from #19597, and tidied up a few things. |
docs/designers-developers/developers/data/data-core-block-editor.md
Outdated
Show resolved
Hide resolved
@@ -20,6 +20,33 @@ import { __ } from '@wordpress/i18n'; | |||
import BlockIcon from '../block-icon'; | |||
import ButtonBlockAppender from '../button-block-appender'; | |||
|
|||
function BlockNavigationRow( { block, isSelected, onClick, children } ) { |
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.
Minor: On the point of naming, I don't think it would be commonly expected to refer to an entry in a list as a "row", but rather as an "item", e.g. BlockNavigationListItem
.
Edit: Also noting that the class name itself assigned to the div
is already referring to this as an "item".
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've done a few things here based on the PR comments:
- Moved this component into a separate file
item.js
, renamed the component toBlockNavigationItem
. - This component should ideally have the classname
block-editor-block-navigation-item
on its root element, but there's no need for a class name there. Given that, I've now changed the class names for the inner elements toblock-editor-block-navigation-item__block
(as this element represents an individual block) andblock-editor-block-navigation-item__button
. - The navigation list appender was still defined in
BlockNavigationList
but was using the styles from theBlockNavigationItem
. I've made this a new componentBlockNavigationItem.Appender
and it uses the classnameblock-editor-block-navigation-item__appender
internally.
// Strip any HTML (i.e. RichText formatting) before returning. | ||
return toPlainText( 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.
Following from my comment at #18132 (comment)
If we're expecting the accessible label to be associated with an attribute, I still find it strange we'd need to do this, because in my view, if the attribute value can represent a label, it should already be plain text.
I guess I can understand based on what you're saying in #18132 (comment) that this could help enable re-use of attributes where the attribute itself serves some purpose (which may require it to contain markup) but can also represent its title (but only after some post-processing). The example you gave is something like a heading's title
. I kinda feel like this should still be left to the block though, or at least:
- If we're already going to need to provide some way for a block to define the logic associated with deriving an accessible label from attributes, then this would force some consistency and simplify the interface (i.e.
__experimentalLabel
is not an overloaded block setting) - What would be the full extent of the "global" post-processing we apply here? I find it troubling that we assume that this value could contain rich text. What else could it contain? My point is that if we force this to be handled by a block, then we don't need to answer this question, because that's left to the block to decide.
The way I'd see it is something more like:
registerBlockType( 'core/heading', {
attributes: {
title: {
type: 'string',
},
},
getLabel: ( { attributes } ) => toPlainText( attributes.title ),
// ...
} );
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 main issue is formatting, for example the Navigation Link block, which has a label that can be formatted (so could contain tags like em
/strong
). I didn't want this to appear in the block navigation.
The label is no longer a function, instead you can denote a single attribute:
__experimentalLabel: 'label'
so there's no way for toPlainText
to be called from there.
In terms of why it's no longer a function I'll need to defer to @mtias or @youknowriad on this, but I think it probably needs some time to mature or to think about the wider use cases.
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 main issue is formatting, for example the Navigation Link block, which has a label that can be formatted (so could contain tags like
em
/strong
). I didn't want this to appear in the block navigation.
Yeah, this makes sense to me that we wouldn't want these tags to be represented. My concern is more that with the implementation as proposed, it (implicitly) bakes in handling of specific attribute values where a string may or may not contain some HTML. It all seems very loosely-defined. The strings containing HTML is an implementation detail of rich text, and is neither something the block editor should be aware of, nor that it actively considers (we just process all strings, regardless whether they're "rich text" strings). It also raises questions for which I don't think we have a good baseline: What criteria do we have for considering whether it makes sense to bake-in such processing? Should we also strip shortcodes from strings? Should we Array#join
an array of strings, if that's how a block chooses to represent some attribute value?
I'd also invite some perspective from Matias and Riad on whether we should avoid the function.
My initial impression is that, by delegating this to the block implementer, it may be slightly less convenient, but is much clearer how this relationship is represented. A block implementer knows that they're working with a rich text value, and they would know that the label must be provided as plain text. It's up to them to satisfy that requirement. We could always consider to add the attribute-name form as an abstracted shorthand in the future, if we can answer the clearer picture for how those problematic attribute value forms be interpreted by the block editor.
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.
What are the negatives, though? Perhaps a slight overhead in cleaning these strings.
The positives seem to outweigh the negatives to me—not only is this easier for block implementors, but also the editor won't show an unusual looking html string if a block implementor forgot to convert the text. It feels like it's a very easy thing to miss for a block developer, and it'd look really buggy to the end user if it were missed.
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.
Would it be strange to serialise the block and strip the HTML from 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.
After all, this is what a screen reader would read if there were no label. It would read the whole content of the 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.
Would it be strange to serialise the block and strip the HTML from that?
After all, this is what a screen reader would read if there were no label. It would read the whole content of the block?
Most of the points I raised above were related to the block navigation UI:
When there's no label it's just the block title that's output. Also worth noting that this is output as a string, so any markup will appear as plain text (e.g. <em>text</em>
instead of text). It doesn't use dangerouslySetInnerHTML
It's the same for the accessiblity label, since this is now implemented in navigation mode, the content of the block wouldn't be read, just the block title. If tags were present in the label it'd be output in the string used as the aria-label:
aria-label="<em>text</em>"
All of that html would be read "less em greater text less slash em greater".
A block outputs HTML, so not an issue there.
If that can be avoided, why not?
Perhaps an alternative could be to not build it into the selector, but instead where the label is consumed.
const blockName = getBlockName( state, clientId ); | ||
|
||
const { | ||
__experimentalGetAccessibilityLabel: getAccessibilityLabel, |
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 think we need some more consistency with how these things are named.
We reference both "accessible" and "accessibility" labels? Can we pick one?
Or also: Why do we need to qualify it? Can't we just call it a label? I worry the qualifier might make this some overly specialized thing, if all we truly care about is having some meaningful way to describe the instance of the block, which can be (but not necessarily only be) used as its accessible label. This ties to what I mention in https://github.com/WordPress/gutenberg/pull/18132/files#r365328275 and https://github.com/WordPress/gutenberg/pull/18132/files#r367984986.
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 reference both "accessible" and "accessibility" labels? Can we pick one?
True, I can sort out the consistency.
Or also: Why do we need to qualify it? Can't we just call it a label? I worry the qualifier might make this some overly specialized thing, if all we truly care about is having some meaningful way to describe the instance of the block, which can be (but not necessarily only be) used as its accessible label.
I'm not sure I fully understand this criticism. It's the same reason we often use aria-label
, aria-description
or screen-reader-text
to add extra information over and above the existing visual labels. Sometimes that's because labels aren't present, but there are plenty of cases where they are, but are not adequate for screen reader usage (block movers are an example).
Replying to your linked comment:
Even in the implementations we've revised so far, we're only ever handling the "accessibility" case. So what's the point of "visual"?
So there's a bit of history here. Originally, the mobile devs implemented an __experimentalGetAccessibilityLabel
for blocks. That's why the majority of the existing cases are accessibility labels.
The navigation link block is the only block that implements a 'visual' label at the moment. There was a specific requirement to do that at the time.
Now I'm attempting to tidy things up (but perhaps making things worse!). My original PR #18132 was an attempt to try to consolidate these APIs into a single label, but the reality is that the accessible label often doesn't work visually, and vice versa.
Paragraph and Heading are two instances where there's probably a bit more information in the accessibility label than we'd consider showing visually. But they've already been assessed as being useful for screen readers.
Perhaps the mistake is that they shouldn't both be called 'labels' as that implies an association. Maybe one should be getAccessibilityDescription
and the other a label? They are used differently at the moment (one in navigation mode, the other in the block navigator) so this probably makes sense.
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.
Perhaps the mistake is that they shouldn't both be called 'labels' as that implies an association. Maybe one should be getAccessibilityDescription and the other a label? They are used differently at the moment (one in navigation mode, the other in the block navigator) so this probably makes sense.
The trouble with AccessibilityDescription is that this is a separate property on Accessibility objects on various platforms. Windows, Linux, and Mac have an AccessibleDescription which is secondary information to the AccessibilityLabel. AccessibilityLabel is the primary thing that says what a button or other thing is named, or will do when executed. The description is additional information. On iOS, this is called AccessibilityHint. It is what VoiceOver speaks after a delay, on a Home Screen element, for example, this is "Double tap to open". In mobile Gutenberg, this is often used to communicate helpful things like "Double-tap to add a block" or instructions pertaining to a particular block type.
So in terms of Accessibility terminology, AccessibilityLabel is the perfectly correct and unambiguous term to use 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.
Thanks for the background @MarcoZehe.
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 not sure I fully understand this criticism. It's the same reason we often use
aria-label
,aria-description
orscreen-reader-text
to add extra information over and above the existing visual labels.
I think the parallels you mention are appropriate, but I see them as backing up my worries. I may be mistaken, but the way I understand those attributes is as something of an escape hatch where the design of a visual label would not sufficient on its own in providing a meaningful name. And that, whenever possible, we should optimize for a label which can be well-understood in all contexts (visual or non-visual).
It may very well be true that in some cases we'd want to present these labels with some added contextual clarification (in other words, maybe we do want to use them in aria-label
, perhaps with some prefix or suffix text). But are those additional details going to be distinct per-block, or something generic that we can decide at a framework level? In the latter case, it affords us the opportunity to remove this from consideration of the block implementer.
At the end of the day, what conceptual details are we wanting the block to provide here? Is it just a short description of the contents of that block? My initial reaction here is that this is something which can be implemented without tying it so specifically to the implementation detail of how that label is used. Or, at least, based on my understanding of how we're currently proposing to use that label, it seemed perhaps premature.
but the reality is that the accessible label often doesn't work visually, and vice versa.
I admit that I'm not fully aware of all of the history here, and that this is fair reasoning for why they be treated distinctly. But I think my approaching this from a perspective of ignorance might be useful in considering to reevaluate this. We should always be open to the possibility that we've been doing it wrong all along. What is it specifically that makes one appropriate visually and not for screen readers, and vice-versa, like you mention? Are those differences reasonable, or symptomatic of a larger problem?
If it helps, my motivation here is largely to avoid forcing the block implementer needing to be aware of a distinction or needing to make a choice (especially where a choice could have negative accessibility implications if done incorrectly), and to distill the requirement to one which maximizes reusability.
Depending whether this one can be merged in time for next week's plugin release, we should want to make sure we make the minimal changes necessary ahead of the release where we're anticipating to need to make breaking changes. I opened #19725 to mark |
a133ce7
to
0a2e6e4
Compare
@@ -32,6 +32,20 @@ _Returns_ | |||
|
|||
- `boolean`: Whether the last change was automatic. | |||
|
|||
<a name="getAccessibleBlockLabel" href="#getAccessibleBlockLabel">#</a> **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.
Should I make these experimental selectors?
To further endorse why we need AccessibilityLabels: Speech is strictly a sequential medium. Information is processed one after another. That also means information must be provided in a way that gives the most context in the most efficient way possible. That means most important info first, followed by the next less important bit, etc. And it needs to convey information that a sighted person would gather from looking at the screen, skimming parts of it, etc. Sight is a parallel medium, sighted people can take in several bits of information at once. So for blocks, for example, the type and position information are information gathered in two places: The label for the block and the position on the screen relative to others. The third bit, the block contents, can be skimmed while looking at the block itself, which also is happening while looking at its position. Now, how do we put that into perspective for a sequential medium like speech or braille? We gather these pieces together from all these different sources: The block type, the position, the contents, and string them together so a speech medium can give this information to someone who cannot see the screen, cannot skim it. That label, however, if applied visually, would clutter up the screen by providing redundant information. That's why applying screen reader labels visually sometimes just doesn't make sense and would reduce usability for sighted people. They already have that information, it would be repeated by applying the screen reader specific info to the visual label. Now, there are cases where a text label on a button does make more sense for everyone than an icon and alternative text for it, especially when it is discovered that many sighted people don't know what that icon is, due to different cultural backgrounds, for example. In such cases, aligning the screen reader label and visual label does make sense. But in cases like this where the screen reader information does contain information from different sources on the screen, redundantly showing that information visually would not make sense. I hope this further explains why it is sometimes necessary to have a special label for screen readers. And why this case for block labels in navigation mode is one of them. |
0a2e6e4
to
ea11eb9
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.
I tested this and it still works as described for the Navigation block (with this PR's updates that is)
Closing this as stale. |
Description
PR #18132 was merged accidentally prior to any code review, but rather than revert I'll attempt to tackle review feedback in follow-ups. #19597 also tackles much of the feedback (thanks @ellatrix!)
This PR:
getAccessibleBlockLabel
andgetBlockLabel
to theblock-editor
package as selectors.stripHTML
from the dom package to rich-text and renames totoPlainText
.getLabel
function that can be declared on blocks there's now separate properties:label
, which can be declared as a string, the name of an attribute that should appear in the block navigator.getAccessibilityLabel( attributes )
a function that takes attributes for tailoring an accessibility label for the block. Breadcrumb: add accessibility label #19597 exposes this in navigation mode. It falls back to using the abovelabel
when unspecifiedHow has this been tested?
Other blocks worth testing for the aria-label - Paragraph, Heading, More, Image.
Types of changes
Task
Checklist: