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

Try to improve the accessibility of featured images #7200

Merged
merged 11 commits into from Jun 21, 2018

Conversation

@jasmussen
Contributor

jasmussen commented Jun 7, 2018

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 postLabel entirely, 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:

featured

jasmussen added some commits Jun 7, 2018

Try improving the a11n of the featured image feature
This commit reproduces #3829, but with slightly better code. The rebase was also too complex.

@jasmussen jasmussen self-assigned this Jun 7, 2018

@jasmussen jasmussen requested review from karmatosed, afercia and WordPress/gutenberg-core Jun 7, 2018

/>
<div>
<MediaUpload
title={ __( 'No image selected' ) }

This comment has been minimized.

@youknowriad

youknowriad Jun 7, 2018

Contributor

This title prop is the title of the modal I think, Are you certain you want this? Shoudl this also be Add image?

@youknowriad

Do you think we should show the buttons (replace, remove) on the same line design-wise?

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 7, 2018

Great feedback. No, didn't want to change the modal title, and letting the buttons float is cool too:

screen shot 2018-06-07 at 12 20 52

) }
/>
<MediaUpload
title={ __( 'Add Image' ) }

This comment has been minimized.

@youknowriad

youknowriad Jun 7, 2018

Contributor

I think we should use the same title in the modal here as well "Set featured image'?

@afercia

This comment has been minimized.

Contributor

afercia commented Jun 7, 2018

@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:

screen shot 2018-06-07 at 18 07 28

  • the "plaecholder" is a button, when clicked opens the media modal
  • same for the "Add Image" button

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:

screen shot 2018-06-07 at 18 08 32

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 alt text on the image. That would work as the accessible name of the button-image. I think @anevins12 is willing to try to address that in a separate PR. 🙂

@afercia

See my last comment!

@anevins12

This comment has been minimized.

Contributor

anevins12 commented Jun 7, 2018

I'm happy to address the alt text in a separate PR

@melchoyce

This comment has been minimized.

melchoyce commented Jun 7, 2018

I think this is a good solution:

@karmatosed

This comment has been minimized.

Member

karmatosed commented Jun 7, 2018

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.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 7, 2018

🤘

I'll update this tomorrow to reflect feedback.

jasmussen added some commits Jun 8, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 8, 2018

I think I addressed the feedback now, both with the modal title and the removal of the additional button.

featured

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' ) }

This comment has been minimized.

@youknowriad

youknowriad Jun 8, 2018

Contributor

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?

This comment has been minimized.

@jasmussen

jasmussen Jun 8, 2018

Contributor

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.

This comment has been minimized.

@youknowriad

youknowriad Jun 8, 2018

Contributor

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?

This comment has been minimized.

@jasmussen

jasmussen Jun 8, 2018

Contributor

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:

screen shot 2018-06-08 at 10 47 11

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?

This comment has been minimized.

@youknowriad

youknowriad Jun 8, 2018

Contributor

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_image label 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.

This comment has been minimized.

@jasmussen

jasmussen Jun 8, 2018

Contributor

Solid thoughts.

@pento, any idea here?

This comment has been minimized.

@jasmussen

jasmussen Jun 11, 2018

Contributor

@afercia you also have a treasure trove of core experience — do you have any insights here?

This comment has been minimized.

@afercia

afercia Jun 19, 2018

Contributor

@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' ),

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Jun 19, 2018

@karmatosed

Looks great to me from design view.

@paulwilde

This comment has been minimized.

Contributor

paulwilde commented Jun 19, 2018

Separate PR, but this new UI is primed for also including drag-and-drop support!

@tofumatt

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

This comment has been minimized.

@tofumatt

tofumatt Jun 19, 2018

Member

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;

This comment has been minimized.

@tofumatt

tofumatt Jun 19, 2018

Member

Eventually I'll propose sorting our CSS attributes alphabetically; until then I'll live with this 😉

This comment has been minimized.

@jasmussen

jasmussen Jun 20, 2018

Contributor

:) 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.

This comment has been minimized.

@tofumatt

tofumatt Jun 20, 2018

Member

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.

This comment has been minimized.

@tofumatt

tofumatt Jun 19, 2018

Member

"size up" is a bit odd, maybe:

// This keeps images at their intrinsic size (eg. a 50px
// image will never be wider than 50px).

@gziolo gziolo added this to the 3.1 milestone Jun 19, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 20, 2018

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 :(

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 20, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 20, 2018

Here's what we do on wp.com:

screen shot 2018-06-20 at 09 50 10

We could make it a positioned IconButton with an X, and have "Remove image" be the tooltip?

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 20, 2018

OMG that's so much better. 👍

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 20, 2018

Tweaked:

now

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.

@afercia

This comment has been minimized.

Contributor

afercia commented Jun 20, 2018

Here's what we do on wp.com:

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.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 20, 2018

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.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 20, 2018

If the problem is in the button text, than the we could also consider to update the labels used by WordPress.

That is the primary problem.

But those are from the postLabel, and unless those are easy to change, I need a plan for how to proceed and get this PR merged.

@karmatosed

This comment has been minimized.

Member

karmatosed commented Jun 20, 2018

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?

jasmussen added some commits Jun 20, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 20, 2018

Okay, we're back to this:

screen shot 2018-06-20 at 12 27 20

I think it might be good to merge this in and iterate separately if need be, including adjusting the postLabels if need be.

We can also go crazy and add a "scary" button color:

screen shot 2018-06-20 at 12 29 50

But that also seems overkill to me.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 20, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 20, 2018

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.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 20, 2018

@afercia

This comment has been minimized.

Contributor

afercia commented Jun 20, 2018

Are there any examples you can think of that achieve this we can learn from?

@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:

screen shot 2018-06-20 at 12 33 16

The pattern used for tags/categories is more interesting, perhaps. If I recall correctly, it was discussed at length from a design perspective:

screen shot 2018-06-20 at 12 35 20

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.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 20, 2018

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! 👍

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 20, 2018

Okay, now it's this:

screen shot 2018-06-20 at 13 33 03

@tofumatt if you could look at the extra prop:20f5489 — this felt the sensible way to do it for me?

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 20, 2018

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.

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.

@tofumatt

I don't see "scary" as a prop or style elsewhere in the code. Is it in a style guide or something?

I'd maybe go with isDestructive over isScary.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 21, 2018

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.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 21, 2018

Okay, it's now destructive instead of scary :)

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 21, 2018

I dig it 👍🏻

@jasmussen jasmussen merged commit 5b2fcd4 into master Jun 21, 2018

2 checks passed

codecov/project 46.91% (+0.28%) compared to 102445b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasmussen jasmussen deleted the try/featured-image-improvements-v2 branch Jun 21, 2018

@ryanwelcher ryanwelcher referenced this pull request Oct 22, 2018

Merged

Adding messaging for open and closed states #10900

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment