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

[kirasong]

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?

[melchoyce]

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.

[adamsilverstein]

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.

[adamsilverstein]

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]

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.

[westonruter]

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

[adamsilverstein]

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.

[kirasong]

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?

[melchoyce]

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

[ocean90]

36337.3.diff​ looks good for 4.5 IMO.

[afercia]

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

[westonruter]

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

[jorbin]

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

[westonruter]

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.