Try to improve the accessibility of featured images #7200
Conversation
This commit reproduces #3829, but with slightly better code. The rebase was also too complex.
jasmussen
added
[Type] Enhancement
| /> | ||
| <div> | ||
| <MediaUpload | ||
| title={ __( 'No image selected' ) } |
There was a problem hiding this comment.
This title prop is the title of the modal I think, Are you certain you want this? Shoudl this also be Add image?
|
Great feedback. No, didn't want to change the modal title, and letting the buttons float is cool too:
|
| ) } | ||
| /> | ||
| <MediaUpload | ||
| title={ __( 'Add Image' ) } |
There was a problem hiding this comment.
I think we should use the same title in the modal here as well "Set featured image'?
|
@jasmussen thanks for this PR 🙂One of the things I was trying to explain on #1116 is that there's no need for two buttons that do the same thing:
To me, seems redundant and potentially confusing for assistive technology users. I'd propose to further simplify. I'd say the "state" ("No image selected") is pretty evident because there's no image... do we really need a text to communicate the state? Also, the button says "No image selected" so it doesn't explicitly communicate the underlying action. Removing the second button and changing the placeholder-button text could work perhaps, and would further simplify the code:
Thoughts? Lastly, when an image has been set, the image itself is within a button so it's using a control with native keyboard interaction. That's good. We just need to use a smart |
|
I'm happy to address the alt text in a separate PR |
|
I think this is a good solution:
|
|
I would second what has been said here, I don't think we need to have buttons here and would love to see that explored. |
|
🤘 I'll update this tomorrow to reflect feedback. |
|
I think I addressed the feedback now, both with the modal title and the removal of the additional button.
What's still missing is the alt title for the image — @anevins12 if you'd like to address that separately, that would be cool, thanks! |
| @@ -30,12 +26,12 @@ function PostFeaturedImage( { featuredImageId, onUpdateImage, onRemoveImage, med | |||
| <div className="editor-post-featured-image"> | |||
| { !! featuredImageId && | |||
| <MediaUpload | |||
| title={ postLabel.set_featured_image } | |||
| title={ __( 'Set featured image' ) } | |||
There was a problem hiding this comment.
Any reason to avoid the labels from the post object postLabel.set_featured_image and postLabel.remove_featured_image. These labels are tweakable server-side. We could use the proposed labels as a fallback if the set_featured_image or remove_featured_image are not defined?
There was a problem hiding this comment.
I've no strong opinion here. In this case I changed it so it was consistent with the other labels.
"Set featured image" is the only label we haven't customized. If you'd like, I can restore all instances where we use "Set featured image" as the label to use the postLabel again.
There was a problem hiding this comment.
I feel we should restore it essentially for Backwards Compatibility because CPT are probably customizing this label. And I feel we consistently use CPT's labels across the app. In which components are we avoiding those?
There was a problem hiding this comment.
I have no moral objections to reapplying those labels. But here's what's in master:
const DEFAULT_SET_FEATURE_IMAGE_LABEL = __( 'Set featured image' );
const DEFAULT_REMOVE_FEATURE_IMAGE_LABEL = __( 'Remove featured image' );
Those labels match the ones from postLabel.
If this branch gets merged sans changes, then we will have the following labels:
__( 'Set featured image' )— this could match the postLabel__( 'Replace Image' )— no match in postLabel__( 'Remove Image' )— could match "Remove featured image", but it feels super redundant and would take up more space.
The postLabels are definitely designed for the old interface:
If we can update the postLabel labels, that would be ideal, but I imagine that's not possible.
What do you think we should do?
There was a problem hiding this comment.
I personally don't think the labels for the featured images are that important because it's a generic thing:
- We can try to add the
replace_featured_imagelabel to the CPT labels. (Not certain how we define the existence of these labels) - I'm also fine considering the featured image generic enough to avoid these labels but maybe we can ask someone more familiar with the plugins to know whether these are heavily used or not? I don't feel confident making the decision myself.
There was a problem hiding this comment.
Solid thoughts.
@pento, any idea here?
There was a problem hiding this comment.
@afercia you also have a treasure trove of core experience — do you have any insights here?
There was a problem hiding this comment.
@jasmussen sorry, I'm late to the party. Is your question related to how many plugins might use custom labels? Not a great plugins expert here, but I guess it's pretty common for plugins to customize them, just a couple examples: EDD and Woocommerce:
Easy digital downloads:
'featured_image' => __( '%1$s Image', 'easy-digital-downloads' ),
'set_featured_image' => __( 'Set %1$s Image', 'easy-digital-downloads' ),
'remove_featured_image' => __( 'Remove %1$s Image', 'easy-digital-downloads' ),
'use_featured_image' => __( 'Use as %1$s Image', 'easy-digital-downloads' ),
Woocommerce:
'featured_image' => __( 'Product image', 'woocommerce' ),
'set_featured_image' => __( 'Set product image', 'woocommerce' ),
'remove_featured_image' => __( 'Remove product image', 'woocommerce' ),
'use_featured_image' => __( 'Use as product image', 'woocommerce' ),
|
Separate PR, but this new UI is primed for also including drag-and-drop support! |
There was a problem hiding this comment.
What's still missing is the alt title for the image — @anevins12 if you'd like to address that separately, that would be cool, thanks!
Code looks good but merge after creating a follow-up issue for that, please 😄
| ) } | ||
| /> | ||
| <div> | ||
| <MediaUpload |
There was a problem hiding this comment.
This looks like the exact same component/args/markup as above. Any chance we could refactor this a bit to reduce the duplication?
| outline: none; | ||
| // Space consecutive buttons evenly. | ||
| .components-button + .components-button { | ||
| margin-top: 1em; |
There was a problem hiding this comment.
Eventually I'll propose sorting our CSS attributes alphabetically; until then I'll live with this 😉
There was a problem hiding this comment.
:) I like your style.
Though elaborate a bit — are you referring to the top margin going before the right margin? For north south east west things I always go clockwise, i.e. top, right, bottom, left.
There was a problem hiding this comment.
I know some folks do that (I used to as well), but yes I am. It makes sense, but I prefer consistency over sense.
| } | ||
|
|
||
| &:hover { | ||
| color: theme( primary ); | ||
| // Don't size up images beyond their intrinsic size. |
There was a problem hiding this comment.
"size up" is a bit odd, maybe:
// This keeps images at their intrinsic size (eg. a 50px
// image will never be wider than 50px).
There was a problem hiding this comment.
I hate to nitpick but the left-aligned buttons under each other, both starting with "Re" in English, I find quite hard to tell apart. If it should be a new/separate UX issue that's cool but "Replace"/"Remove" are so similar in length/look and the buttons have the same style, so I find those buttons very hard to scan quickly.
If we wanna do that as a follow-up though, that's cool. Let me know and I'll file it.
|
I could change it to say "Replace featured image", then the two buttons would be closer to each other in length. But that would be arguably worse with regards to the similarity between the buttons. What do you think? I personally really liked the old "Replace image" "Remove image" buttons, but the gnarly reasons for why this isn't smart start here: #7200 (comment) — back compat, in other words :( |
|
I think they could use different styles (maybe a more “scary”/red remove button). The issue is they are similar lengths with similar-looking wording in English on top of each other with no style distinctions. As long as they both start with “Re” I think it’s a problem.
… On 20 Jun 2018, at 09:38, Joen Asmussen ***@***.***> wrote:
I could change it to say "Replace featured image", then the two buttons would be closer to each other in length. But that would be arguably worse with regards to the similarity between the buttons. What do you think?
I personally really liked the old "Replace image" "Remove image" buttons, but the gnarly reasons for why this isn't smart start here: #7200 (comment) <#7200 (comment)> — back compat, in other words :(
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub <#7200 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFi99eiuHynkcHfWrPfS8c1v9kPJCrGks5t-fwPgaJpZM4UeC3I>.
|
|
Here's what we do on wp.com:
We could make it a positioned |
|
OMG that's so much better. 👍 |
|
Tweaked:
This is a biggish change, so CC: @afercia @karmatosed for re-review. I also added a focus style to the image itself, the focus style was hidden underneath the image itself. |
Please, no 🙂 Buttons with visible text are always better for usability, accessibility, controls name should be apparent to users etc. A "X" button actually decreases the accessibility level. If the problem is in the button text, than the we could also consider to update the labels used by WordPress. |
|
Oops, darn. Would adding "Remove" or changing the "x" to "remove" work? It could be "Remove image" with "image" as off-screen text for screenreader. As a sighted user, having the different actions in different places and looking different vastly improves usability. |
That is the primary problem. But those are from the |
|
In this case let's take a step back have 2 buttons, but then have another issue to iterate - I think holding it up for that discussion isn't the best option. @afercia whilst I understand the accessibility concerns and I am recommending we go back to 2 buttons temporarily, I would like to find an option that is both accessible and usable. Having double buttons looking the same simply isn't usable for a lot of people. Are there any examples you can think of that achieve this we can learn from? |
|
Okay, we're back to this:
I think it might be good to merge this in and iterate separately if need be, including adjusting the We can also go crazy and add a "scary" button color:
But that also seems overkill to me. |
|
Or you could make the "replace image” button blue or something if the red is too much. Just *anything* to distinguish them visually.
|
|
The blue button is supposed to be a "primary" button, and I feel like UX-wise there is no primary action here. Both actions are secondary, in that you're basically "done" with what you set out to do, which is to set a featured image. |
|
Good point, I worried about that.
I am for red then, but I am not a designer so feel free to ignore. It looks weird but it is more usable to me.
|
@karmatosed hm so this is especially related to the "remove" button sitting close to the replace one, correct? I think the most recent design discussions and implementations about placement of multiple buttons and about how to distinguish the one used for destructive action were for the Media widgets and the new pattern recently implemented in the tags/categories edit screens. The media widgets have multiple button, but the control for destructive action is completely separated:
The pattern used for tags/categories is more interesting, perhaps. If I recall correctly, it was discussed at length from a design perspective:
I'm not saying to make the "replace" image blue, but maybe differentiating the "remove" one as @jasmussen suggested could be a good option? @jasmussen don't be afraid to open a ticket on Trac to change the labels used by WordPress 🙂if that makes things more usable and adds value, I guess everybody would be happy to change them. |
|
That red, text-style button would work for me... I know there are also a11y/UX concerns with buttons looking like links/not like buttons, but if "Replace" was a standard button and "Remove image" was a red-text/link-styled button I would find this more usable... and I think it's prettier than the scary red button. Thanks @afercia! 👍 |
I'm not at all afraid of doing that, and we should certainly do this regardless. But we can move faster with Gutenberg than we can with the rest of core, which means it should be staggered — first this, then that. |
|
Yes, there's a "scary" term used in a few style guides I've seen, including the wp.com one. There's nothing for WordPress, the core css class is just "delete". I can go with destructive. |
|
Okay, it's now destructive instead of scary :) |
|
I dig it 👍🏻 |
The goal of this PR is to fix #1116. It replaces #3829 as this is better code, and the rebase was also too complex.
This PR consists of two commits.
The first commit, 76f1946, is mostly a copy of the old PR (#3829), but relies on the exact same labels that we receive from
postLabel.The 2nd commit, 6a40827, removes the reliance on the labels we receive frmo
postLabelentirely, because they do not match what we are doing with multiple buttons, and in my understanding, those separate and multiple buttons are key to improving accessibility.I would like comments on both approaches, and the approach of this PR overall. Here's a GIF: