Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#36337 closed defect (bug) (fixed)

Shift-Click to Edit on Custom Logo selects "Remove" button.

Reported by: kirasong's profile kirasong Owned by: westonruter's profile westonruter
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.6
Component: Customize Keywords: has-patch commit
Focuses: ui, accessibility Cc:

Description

When shift-clicking on a previewed Custom Logo in the customizer to edit the Custom Logo (as suggested by the tooltip), the "Remove" button is selected by default.

Would it make more sense to select "Change Logo" by default, or swap the order of the buttons so that "Change Logo" is displayed first?

Attachments (4)

Screen Shot 2016-03-25 at 1.55.07 PM.png (140.4 KB) - added by kirasong 8 years ago.
36337.diff (743 bytes) - added by adamsilverstein 8 years ago.
36337.2.diff (2.2 KB) - added by adamsilverstein 8 years ago.
36337.3.diff (2.1 KB) - added by westonruter 8 years ago.

Download all attachments as: .zip

Change History (20)

#1 @melchoyce
8 years ago

Definitely wouldn’t switch the order of the buttons — it shouldn’t be different from header and background image.

I think focusing on Change Logo instead is probably fine.

#2 @adamsilverstein
8 years ago

This happens because the focus is designed to hit the first visible button or link.

36337.diff fixes this by explicitly excluding elements with the class remove-button. After applying the patch, shift clicking the logo focuses the 'Change Logo' button. This may not be the best solution; I think a more robust solution would enable sections to specify a focus target that would override the default 'first button' behavior.

http://cl.ly/3X1W0B1b2k0L/Customize_Dev_Test__Just_another_WordPress_site_2016-03-25_17-53-22.jpg

#3 @adamsilverstein
8 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

36337.2.diff:

Alternate approach:

  • Add a class focus-first to the 'Change logo' button.
  • Add support in focus routine for a focus tagged element, which when present gets the focus.

@westonruter
8 years ago

#4 follow-up: @westonruter
8 years ago

  • Keywords 2nd-opinion dev-feedback removed

Instead of a focus-first class name I suggest something like control-focus, as it is the element which receives focus when the control's focus method is called. See 36337.3.diff, which also simplifies some of the logic.

#5 follow-up: @westonruter
8 years ago

  • Milestone changed from Awaiting Review to Future Release

@mikeschroder is this something which you think should go into 4.5?

#6 in reply to: ↑ 4 @adamsilverstein
8 years ago

Replying to westonruter:

Instead of a focus-first class name I suggest something like control-focus, as it is the element which receives focus when the control's focus method is called. See 36337.3.diff, which also simplifies some of the logic.

Good suggestion; thanks for the review and simplified code.

#7 in reply to: ↑ 5 @kirasong
8 years ago

Replying to westonruter:

@mikeschroder is this something which you think should go into 4.5?

Mixed. It might make sense if there are a11y concerns. @afercia, do you have any thoughts on this?

#8 @melchoyce
8 years ago

Let's try to get this into 4.5 if we can.

#9 @ocean90
8 years ago

36337.3.diff looks good for 4.5 IMO.

#10 @afercia
8 years ago

The Customizer preview accessibility offers lots of room for improvements, I think this single feature glitch doesn't make things worse :) Moving focus on the "Change logo" makes more sense. I'd just suggest to expand the "Remove" button text with an aria-label attribute since "Remove" alone doesn't tell enough when read out of context. Something like:

<button type="button" class="button remove-button" aria-label="Remove logo">Remove</button>

would be better, as done in other parts of the Customizer to expand other buttons text...

#11 @kirasong
8 years ago

  • Milestone changed from Future Release to 4.5

#12 follow-up: @westonruter
8 years ago

  • Keywords commit added

@afercia good point, but I think that accessibility improvement is beyond the scope of this ticket, as there is no current arg among $button_logos to provide a more verbose {{aria-label}} like “Remove Button”. Currently it is:

'button_labels' => array(
	'select'       => __( 'Select logo' ),
	'change'       => __( 'Change logo' ),
	'remove'       => __( 'Remove' ),
	'default'      => __( 'Default' ),
	'placeholder'  => __( 'No logo selected' ),
	'frame_title'  => __( 'Select logo' ),
	'frame_button' => __( 'Choose logo' ),
),

I think we need a second core dev to give a +1 to commit 36337.3.diff

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


8 years ago

#14 in reply to: ↑ 12 @jorbin
8 years ago

Replying to westonruter:

I think we need a second core dev to give a +1 to commit 36337.3.diff

This is your second. Commit it

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


8 years ago

#16 @westonruter
8 years ago

  • Owner set to westonruter
  • Resolution set to fixed
  • Status changed from new to closed

In 37152:

Customize: Put focus on change button instead of remove button in media control.

Allow controls to specify the primary element to receive focus by adding a control-focus class.

Props adamsilverstein, westonruter.
Fixes #36337.

Note: See TracTickets for help on using tickets.