Skip to content
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 featured image style and accessibility #3829

Closed
wants to merge 1 commit into from

Conversation

jasmussen
Copy link
Contributor

This is an attempt to fix #1116.

It redesigns the Featured Image panel to more closely resemble both the visual style and the markup/accessibility of the Image widget.

One problem this PR has right now, is that I'm not quite sure how to update/tweak the labels, that appear to be using a postLabel function. As such when building this the labels were wrong. Any idea what this might be, @youknowriad

Screenshots of how it should look (labels were wrong, so those are edited in the Chrome inspector):

screen shot 2017-12-06 at 11 26 44

screen shot 2017-12-06 at 11 25 57

As a reminder, this is how the Image widget looks:

screen shot 2017-12-06 at 11 28 09

screen shot 2017-12-06 at 11 28 17

@jasmussen jasmussen added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] Document Settings Document settings experience [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement. labels Dec 6, 2017
@jasmussen jasmussen self-assigned this Dec 6, 2017
Copy link
Contributor

@afercia afercia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From an accessibility perspective, having two buttons that do the same thing but have different text is not ideal.
The first button says: "No image selected"
The second button says: "Add image"

That's potentially confusing and duplicates the controls for no purpose other than visuals. Also, the "placeholder" with text "No image selected" indicates the current state, not the available action of the button.

This was discussed at length for the customizer in https://core.trac.wordpress.org/ticket/34323 and my argumentation there can be summarized in "If the placeholder is clickable, what's the purpose of the "Select Image" button?"

Then, the image widget recently implemented a clickable div but that's not ideal and should be improved.

For Gutenberg, I'd recommend to consider if there's really the need of 2 separate controls that do the same thing.

What if we keep just the "placeholder" button using as text "Add image"? the state can be inferred, there's probably no need to explicitly say "No image selected".

As per the labels, I think the default ones come from the core get_post_type_labels() function.

@jasmussen
Copy link
Contributor Author

There's a lot to this, so I appreciate your time.

Would the following work?

State: No featured image:

  • "No image selected" remains unchanged, but isn't clickable. There's just an "Add Image" button

State: Featured image added:

  • The image itself is clickable with the aria-label and also title attribute (for hover tip text) "Replace Image"
  • There's a single button below: "Remove image"

Alternately, can you sketch out (just on paper or in text) what you would consider the ideal flow, accessibility wise? Thanks.

@afercia
Copy link
Contributor

afercia commented Dec 8, 2017

"No image selected" remains unchanged, but isn't clickable. There's just an "Add Image" button

Perfect.

The image itself is clickable with the aria-label and also title attribute (for hover tip text) "Replace Image"

Not so ideal for accessibility. In WordPress, we've removed the title attribute that was used on the featured image, together with tons of other title attributes. In this case using both aria-label and title attribute with the same text would be redundant (both would be announced).
ideally, I'd keep the image not clickable: the image should be announced just by its alt attribute to give users info about what the image is.
And I'd keep the two buttons below.

@jasmussen
Copy link
Contributor Author

I'd be okay with a non-clickable image and two buttons below.

But before I do anything, I feel like I should get a sanity check from @karmatosed and @melchoyce on this. I believe in the media widget, the dashed area is also a dropzone...?

@afercia
Copy link
Contributor

afercia commented Dec 8, 2017

I believe in the media widget, the dashed area is also a dropzone...?

Yes it is 😬 see my previous comment:

Then, the image widget recently implemented a clickable div but that's not ideal and should be improved.

@afercia
Copy link
Contributor

afercia commented Dec 8, 2017

Reminder: about the alt attribute, see some important considerations in #1520 (can be addressed there, I guess)

This is an attempt to fix #1116.

It redesigns the Featured Image panel to more closely resemble both the visual style and the markup/accessibility of the Image widget.

One problem this PR has right now, is that I'm not quite sure how to update/tweak the labels, that appear to be using a `postLabel` function. As such when building this the labels were wrong. Any idea what this might be, @youknowriad
@jasmussen jasmussen force-pushed the try/featured-image-improvements branch from 44f37ec to 2efe7bb Compare December 15, 2017 11:31
@jasmussen
Copy link
Contributor Author

Looking closer at the labelling, I think maybe I can't easily change the labels (seems like they're coming from the REST API), so I'll probably take another stab at this one in light of that.

Feature wise, though I'd love input by @melchoyce as I know you worked on the media widget.

@melchoyce
Copy link
Contributor

I constantly see people clicking the images and the placeholder in usability tests. I'm on support this week and can't spend time on core, but remind me after the new year and I'll try to dig up some examples.

@afercia
Copy link
Contributor

afercia commented Dec 18, 2017

I constantly see people clicking the images and the placeholder

As discussed at length in the context of https://core.trac.wordpress.org/ticket/34323 users click the "placeholder" because it looks like an actionable control 🙂 so I'd say it's a design issue in the first place. Maybe the purpose of the placeholder should be clarified first: what it should be/do, exactly? If it's meant to just inform users about the state, then it should not look like an actionable control.
Instead, if it's meant to change/upload the image, it should be labeled accordingly and not be labeled with the state.

@karmatosed
Copy link
Member

I would +1 bringing this as close to the media widget in core as we have right now. Interested in what you can bring example wise to this @melchoyce when you are back.

@jasmussen jasmussen added the Customization Issues related to Phase 2: Customization efforts label Jan 17, 2018
@jasmussen jasmussen added Needs Decision Needs a decision to be actionable or relevant and removed [Status] In Progress Tracking issues with work in progress labels Apr 11, 2018
@afercia
Copy link
Contributor

afercia commented Jun 6, 2018

@jasmussen is this still on your radar? 🙂 Seems a good improvement. See #1116 (comment)

@jasmussen
Copy link
Contributor Author

Yes still on my radar, thanks. It was recently unblocked so will take another stab as soon as I can. 👍

jasmussen pushed a commit that referenced this pull request Jun 7, 2018
This commit reproduces #3829, but with slightly better code. The rebase was also too complex.
@jasmussen
Copy link
Contributor Author

Closing this in favor of #7200, which is a replacement for this.

@jasmussen jasmussen closed this Jun 7, 2018
@jasmussen jasmussen deleted the try/featured-image-improvements branch June 7, 2018 09:25
jasmussen added a commit that referenced this pull request Jun 21, 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.

* Try customizing the labels instead of relying on those from the postLabel

* Fix modal title, and let blocks align next to each other

* Address feedback.

* Fix modal title.

* Address feedback, restore postLabel.

* Tweak "Remove" button.

* Revert last button.

* Clean up CSS.

* Add isScary prop to button.

* isScary to isDestructive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Customization Issues related to Phase 2: Customization efforts [Feature] Document Settings Document settings experience [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Decision Needs a decision to be actionable or relevant [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Featured image control improvements and alt text
4 participants