Make WordPress Core

Opened 8 years ago

Closed 5 years ago

#34323 closed enhancement (invalid)

Make customizer media control placeholders clickable

Reported by: natewr's profile NateWr Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.1
Component: Customize Keywords:
Focuses: ui, accessibility Cc:

Description

This patch transforms the image placeholder for media controls in the customizer into a click-able area which loads the media manager.

Here is a demonstration GIF attached to Slack #core-customize. https://wordpress.slack.com/archives/core-customize/p1444593134000052

This patch represents a bare minimum of changes to implement this enhancement. However, while creating it I came across a couple of things worth considering:

  1. api.HeaderTool.CurrentView has a getHeight function which was previously used to set a fixed height to the placeholder div. This is no longer needed, so I just chopped the one line out of the setPlaceholder function. However, as far as I could tell, this getHeight function doesn't appear to be getting used. It does set a savedHeight value to the view's model, but I couldn't find that used anywhere. Perhaps it could get cut out completely?
  1. api.HeaderControl uses api.HeaderTool.CurrentView to render the placeholder. It seems most appropriate for the click event listener to be registered within api.HeaderTool.CurrentView. For this to happen, though, the view would need some way to pass the click event to the control, or the view would need to know how to open the media manager. I couldn't see any simple way to do that. To keep this initial patch as small as possible, I went ahead and added event listeners into the control. But perhaps someone with a better knowledge of the overall architecture wants to suggest a better way to handle this event listener.

I did a bit of browser testing on this and all was ok:

Chrome 45.0.2454.101/Ubuntu 14.04
Firefox 41.0.1/Ubuntu 14.04
Chrome/Android
IE11/Win7 (VM)
IE10/Win7 (VM)
IE9/Win7 (VM)

I have also attached a theme for testing. It will add all the core controls which extend the media control: Header, Background, Media, Image, Upload.

Attachments (4)

customizer_clickable_placeholders.patch (3.1 KB) - added by NateWr 8 years ago.
Initial patch
customizer-core-controls-test-theme.zip (2.1 KB) - added by NateWr 8 years ago.
A test theme to add all of the effected controls to the customizer
34323.diff (2.7 KB) - added by celloexpressions 8 years ago.
Make placeholders clickable buttons.
34323.2.diff (2.2 KB) - added by celloexpressions 8 years ago.
Clarify that placeholders are only "clickable" for mouse and keyboard users, add a white background to indicate a "clickable" element visually per customizer UI conventions and add a hover state with a darkened border.

Download all attachments as: .zip

Change History (47)

@NateWr
8 years ago

Initial patch

@NateWr
8 years ago

A test theme to add all of the effected controls to the customizer

This ticket was mentioned in Slack in #core-customize by natewr. View the logs.


8 years ago

#2 @swissspidy
8 years ago

  • Keywords has-patch needs-testing added

#3 @afercia
8 years ago

  • Focuses accessibility added

I'd recommend every new UI control should be introduced with a basic level of accessibility. Currently, this "clickable div" is not even focusable with a keyboard. Every UI control should be operable using just the keyboard. And it's just a div, doesn't convey any semantics at all.

Additionally, there are a lot of clickable things here :) Maybe too many:

  • click on the label and the modal opens
  • click on the description (which is inside the label and there's a ticket for that) and the modal opens
  • click on the placeholder and the modal opens
  • click on the button and the modal opens

https://cldup.com/M5DWNTfOua.png

I'm not even sure there's the need for a new control in the first place. If now the placeholder is clickable, what's the purpose of the "Select Image" button?

Maybe we should consider to remove things instead, not add new (redundant) ones.

#4 @NateWr
8 years ago

I did consider the accessibility issues, but for the reasons you elaborate I felt a focusable element would only add clutter to someone using a screen-reader or navigating by keyboard.

To be clear, I haven't added the "No image selected" placeholder. That text already exists, and for screen-readers or tab-based navigation the behaviour has not changed at all. This patch only makes the existing placeholder clickable, and styles it to appear more like a placeholder.

As you mention, though, it might make more sense to get rid of the placeholder text.

#5 @afercia
8 years ago

Tabbing isn't the only way screen reader users have to navigate through content. Screen readers use arrow keys to navigate sequentially through elements, shortcuts for jumping through specific element types, etc. Any screen reader will read out that div as "clickable No image selected". Being announced as "clickable" users will likely try to press Enter on that div.
The only way to implement UI controls that will be usable for all users it's to use native controls or, if there's really a good reason to use a non-semantic element, then ARIA roles and attributes should be used to rebuild the semantics of that pseudo-control.

As you mention, though, it might make more sense to get rid of the placeholder text.

I didn't mean that. I meant we already have a button that says "Select Image". I don't see any reason to add one more, redundant, control to do the same thing :)

#6 follow-up: @NateWr
8 years ago

Thanks @afercia, that's useful insight. I didn't realize a pointer: cursor would effect screen readers.

The main reason I considered adding the clickable placeholder is because, for me, I expect to be able to click on that text to add an image. Maybe it's just common convention these days, or maybe it's just me, but when I see placeholder items like that, I expect to be able to interact with them.

If the downsides outweigh the upsides, though, perhaps it's not the right approach.

#7 in reply to: ↑ 6 @afercia
8 years ago

Replying to NateWr:

Thanks @afercia, that's useful insight. I didn't realize a pointer: cursor would effect screen readers.

It's not the pointer: cursor but the attached JavaScript event that makes screen readers announce "clickable" :)

#8 @celloexpressions
8 years ago

The title and description shouldn't be clickable, the placeholder and button should be. In terms of accessibility, I'd imagine only the button should be actionable, with the title, description, and current/no image being read out.

That being said, while we should avoid introducing further accessibility issues, I'm not thrilled about the idea of holding up this fairly straightforward improvement to do a major accessibility revamp of these controls. That should probably happen in a different ticket.

#9 @NateWr
8 years ago

@afercia If you have a vision of how to make the customizer controls more accessible, and are willing to provide some direction and guidance, I'd be happy to work on a separate patch with more significant revisions. Just ping me in Slack or open a new ticket and point me to it.

#10 @celloexpressions
8 years ago

  • Milestone changed from Awaiting Review to Future Release

Let's either merge the basic clickable placeholder functionality (which would be done by making it a button) in with #36175 and refocus this ticket to also address the issue of too many labels/clickable elements here, or get a patch in for the functionality here and then open a new ticket to address the broader accessibility issues affecting all customizer controls. @afercia would you have a preference between those?

#11 @afercia
8 years ago

@celloexpressions I'd say the second one, make the placeholder a button here (and remove the current "Select Image" button, right?). Then open a new ticket for the too many labels/clickable elements.

#12 @afercia
8 years ago

P.S. about the labels, there are also #33085 and #33064.

Last edited 8 years ago by afercia (previous) (diff)

#13 follow-up: @celloexpressions
8 years ago

Sounds good; #36175 will remove the select button for images when there, although it will stay for other media types and when nothing's set. Should we make the new button aria-hidden so that there aren't two buttons for accessibility here?

#14 in reply to: ↑ 13 @afercia
8 years ago

Replying to celloexpressions:

Should we make the new button aria-hidden so that there aren't two buttons for accessibility here?

I'd like to see it in action first, I'd say two buttons are maybe a bit too much for all users, not just for accessibility but let's see it before? By the way, generally is not so good hiding a control with aria-hidden because it will still be focusable (not to mention assistive technologies implement aria-hidden in different ways when it comes to controls).

@celloexpressions
8 years ago

Make placeholders clickable buttons.

#15 @celloexpressions
8 years ago

  • Milestone changed from Future Release to 4.6
  • Version changed from 4.4 to 4.1

@afercia how does 34323.diff look?

There will be two UI controls here. The placeholder was deemed necessary in 4.1 when these controls were redesigned (I don't remember why), but we also need an explicit button because it may not be clear with the placeholder alone that that's how you change it (especially since they previously weren't clickable). Maybe the best option would be to hide it with tabindex=0 for keyboard users? Or even just make it a clickable div? Making it actionable is primary for mouse and touch users' experience to be slightly improved.

#16 @celloexpressions
8 years ago

Note: I intentionally excluded header images from the patch because they're a separate codebase and we will get these enhancements once it leverages the revamped core controls instead of being separate.

#17 @afercia
8 years ago

@celloexpression tested and I'm not sure having two buttons is ideal. Especially the first one will be announced as:

No image selected - button

and that doesn't say what activating the button actually does. What about to keep just the first button (the placeholder) and change the text in:

No image selected. Choose one.

(or something like that). Same for the logo.

#18 @celloexpressions
8 years ago

  • Keywords ui-feedback added

Personally I'd prefer to keep the visual button as noted above. Making the placeholder also clickable is a fairly simple enhancement that doesn't have many negatives, but also looking at removing the button that looks like a button makes this a considerably more significant UX change. Is there a way to do that without making the accessible experience worse; such as simply attaching the events to make the existing div clickable as I noted above?

This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.


8 years ago

This ticket was mentioned in Slack in #design by celloexpressions. View the logs.


8 years ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


8 years ago

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


8 years ago

#23 @karmatosed
8 years ago

In user tests despite having the button there, a lot of people click the main area. So I'm very keen on this being brought in as functionality. However, some still click the button and relying on discoverability if you have none - that worries me from a UX perspective.

I would love there to be a balance that also saw us able to have the button and click area.

#24 @karmatosed
8 years ago

  • Keywords ux-feedback added

#25 @celloexpressions
8 years ago

Could we make the placeholder clickable for now but revisit whether we need the placeholder in addition to a separate button in another ticket? I'm worried that we may be biting off too much here if we start getting into more involved changes/potentially removing UI.

This ticket was mentioned in Slack in #design by hugobaeta. View the logs.


8 years ago

#27 @celloexpressions
8 years ago

Needs a refresh for [37426].

Also still need a decision on the appropriateness of making the placeholder clickable/tappable for mouse/touch users but keeping it inaccessible via other input methods in favor of the existing button, which is to remain for now.

@celloexpressions
8 years ago

Clarify that placeholders are only "clickable" for mouse and keyboard users, add a white background to indicate a "clickable" element visually per customizer UI conventions and add a hover state with a darkened border.

#28 @celloexpressions
8 years ago

34323.2.diff clarifies in code that placeholder clickability is only intended to be useful for mouse and touch users, as there is already a <button> for keyboard and screen reader users to use. Revised patch also refines the UI by adding a darkened border color for a hover state and giving placeholders a white background. White is (as much as possible) reserved as an indicator for "clickable" elements in the customizer UI, seen in navigation, input fields, etc.

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


8 years ago

#30 @chriscct7
8 years ago

  • Owner set to celloexpressions
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core by voldemortensen. View the logs.


8 years ago

#32 @celloexpressions
8 years ago

@afercia knowing it's not ideal, would you be okay with 34323.2.diff?

#33 @afercia
8 years ago

@celloexpressions I'm sorry my personal opinion hasn't changed. I will ask again also the other a11y team members. When using screen readers or other assistive technologies the Tab key is not the only way to navigate through content. With all the screen readers I've tested with, using arrows to navigate I can reach the placeholder and activate it. NVDA announces both the label and the placeholder as "clickable" elements, see below:

https://cldup.com/4pRhkoTwuu.png

So trying to recap what 34323.2.diff does:

  • as far as I see it works only on the background image control, not on the header image control for example
  • clicking the "Background Image" label opens the modal
  • clicking the "placeholder" opens the modal
  • clicking the button opens the modal
  • all the 3 controls can be reached and activated with a screen reader
  • the button is announced as "Background Image" because screen readers read the associated label, not the button content

From an accessibility perspective, this is far from ideal. I'd say the same also from an UI and design perspective. I still think the idea behind this patch tries to solve a problem introducing another problem.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


8 years ago

#35 follow-up: @celloexpressions
8 years ago

Noting that this will change every image control except for header images.

I think the question is whether we're okay with a slightly worse experience for screen reader users in exchange for a slightly better experience for touch and mouse users with little impact on keyboard-only/no screen reader. That's a tough decision to make but I think we need to consider whether the net effect is positive for enough users without making things too much worse for others.

#36 @celloexpressions
8 years ago

Could we use aria-hidden to prevent the placeholder from showing up for screen readers?

This ticket was mentioned in Slack in #core by ocean90. View the logs.


8 years ago

#38 in reply to: ↑ 35 @ocean90
8 years ago

  • Keywords needs-patch added; has-patch needs-testing removed
  • Milestone changed from 4.6 to Future Release

Replying to celloexpressions:

I think the question is whether we're okay with a slightly worse experience for screen reader users in exchange for a slightly better experience for touch and mouse users with little impact on keyboard-only/no screen reader.

I'm sure that there is a solution which provides a good experience for all parties.

#39 @karmatosed
8 years ago

I would also suggest we should really try and find balance here. Having the button and then testing / working on a solution later could absolutely be the best route. That is if we still keep it just as accessible as it is now - or make it more. What isn't good is taking a hit on accessibility.

#40 @karmatosed
7 years ago

  • Keywords has-ui-feedback has-ux-feedback added; ui-feedback ux-feedback removed

#41 @celloexpressions
7 years ago

  • Keywords has-ui-feedback has-ux-feedback removed
  • Owner celloexpressions deleted

Assuming the aria-hidden suggestion above isn't a good approach, I don't have further thoughts here currently. I'm sure there's a way to make this work well for everyone, but this needs some fresh ideas for making that happen in a logical way.

Removing has-* kewords as this is still not resolved and that implies that there is a solution/resolution/direction.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


7 years ago

#43 @afercia
5 years ago

  • Keywords needs-patch removed
  • Milestone Future Release deleted
  • Resolution set to invalid
  • Status changed from assigned to closed

The media placeholder "clickable divs" have been replaced with buttons in [44796] / #43151 with the only exception of the header "placeholder", which is a special case and needs to be addressed separately in #46422.
This change aligns with the pattern used in Gutenberg. Therefore, I'd lean towards closing this ticket. Please do feel free to reopen if I'm missing something.

Note: See TracTickets for help on using tickets.