Opened 9 years ago
Closed 9 years ago
#36337 closed defect (bug) (fixed)
Shift-Click to Edit on Custom Logo selects "Remove" button.
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (20)
#2
@
9 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.
#3
@
9 years ago
- Keywords has-patch dev-feedback added; needs-patch removed
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.
#4
follow-up:
↓ 6
@
9 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:
↓ 7
@
9 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
@
9 years ago
Replying to westonruter:
Instead of a
focus-first
class name I suggest something likecontrol-focus
, as it is the element which receives focus when the control'sfocus
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
@
9 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?
#9
@
9 years ago
36337.3.diff looks good for 4.5 IMO.
#10
@
9 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...
#12
follow-up:
↓ 14
@
9 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.
9 years ago
#14
in reply to:
↑ 12
@
9 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
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.