WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 6 weeks ago

#53803 closed defect (bug) (fixed)

Customizer: Radio button on background image position selector visible on focus

Reported by: walbo Owned by: peterwilsoncc
Milestone: 5.8.1 Priority: normal
Severity: normal Version: 5.8
Component: Customize Keywords: has-screenshots has-patch commit fixed-major
Focuses: accessibility, css Cc:

Description

After 5.8 the radio buttons on the background image position selector show on focus. Seems like the css style added by the new customizer widget screen affects the image position selector.

Steps to reproduce:

  1. Use a theme that has background image option. Ex twenty-twenty-one
  2. Go to customizer > Background Image
  3. Upload an image and change the position. Radio button is visible.

Expected behavor:
The radio button is hidden.

Attachments (6)

background-position.gif (2.7 MB) - added by walbo 2 months ago.
background-position-controls-chrome.png (266.5 KB) - added by sabernhardt 2 months ago.
background-position-controls-chrome-classic-widgets-active.png (590.5 KB) - added by sabernhardt 2 months ago.
radio buttons not visible with Classic Widgets plugin activated (WP5.8)
53803.patch (435 bytes) - added by ravipatel 2 months ago.
Radio input focus css added based on ref class
53797.patch (10.8 KB) - added by ravipatel 2 months ago.
@walbo I have created patch with ui-helper-hidden-accessible, now css and class both patch avaiable.
53803.2.patch (1.9 KB) - added by ravipatel 2 months ago.
@walbo now added a right ticket patch, chanegs class as well in file.

Change History (20)

@sabernhardt
2 months ago

radio buttons not visible with Classic Widgets plugin activated (WP5.8)

#1 @sabernhardt
2 months ago

  • Focuses css added
  • Milestone changed from Awaiting Review to 5.8.1

The .screen-reader-text:focus styles in block-library/style.css override the clip value (and other properties).

@ravipatel
2 months ago

Radio input focus css added based on ref class

#2 @sabernhardt
2 months ago

Thanks for the patch! Unfortunately, the input uses the screen-reader-text class purposely to avoid display:none or visibility:hidden.

A negative top value might help for a quick fix:

.background-position-control .screen-reader-text:focus {
    top: -9999px;
}

I'd like to try pulling the radio button out of the label tag (using for and id attributes), but that is too much to change comfortably in a minor release.

#3 @sabernhardt
2 months ago

We also could reconsider assigning the ui-helper-hidden-accessible class in the markup (ticket:22058#comment:103), at least for now.

#4 @walbo
2 months ago

As you say @sabernhardt it should probably be refactored in a major release. I did a quick test locally and ui-helper-hidden-accessible will work as a fix for 5.8.1.

Will create a patch tomorrow unless @ravipatel updates his patch.

@ravipatel
2 months ago

@walbo I have created patch with ui-helper-hidden-accessible, now css and class both patch avaiable.

#5 @walbo
2 months ago

Thanks @ravipatel. However it looks like your last patch was meant for a different ticket.

This ticket was mentioned in PR #1523 on WordPress/wordpress-develop by walbo.


2 months ago

  • Keywords has-patch added; needs-patch removed

#7 follow-up: @sabernhardt
2 months ago

  • Focuses accessibility added

On second thought, we need to remove the screen-reader-text:focus styles in Gutenberg instead. These radio buttons might not be the only focusable elements in Core with that class, and users' websites could have something similar on the front end.

PR 30141 apparently added the screen-reader-text styles for the Archives block's optional dropdown label. The focus styles are intended mainly for skip links, not just anything with the class.

#8 @walbo
2 months ago

Thanks @sabernhardt

I'll create a issue upstream in Gutenberg repo.

@ravipatel
2 months ago

@walbo now added a right ticket patch, chanegs class as well in file.

#9 @sabernhardt
2 months ago

Thanks for opening the issue GB33724 and PR 33726

#10 in reply to: ↑ 7 @mukesh27
2 months ago

Replying to sabernhardt:

On second thought, we need to remove the screen-reader-text:focus styles in Gutenberg instead. These radio buttons might not be the only focusable elements in Core with that class, and users' websites could have something similar on the front end.

PR 30141 apparently added the screen-reader-text styles for the Archives block's optional dropdown label. The focus styles are intended mainly for skip links, not just anything with the class.

Agreed with @sabernhardt second thoughts.

Thanks for PR @walbo

#11 @peterwilsoncc
8 weeks ago

  • Keywords commit added

Testing with 53803.2.patch applied:

  • Radio buttons hidden within customizer editor
  • Radio buttons hidden on deprecated wp-admin/themes.php?page=custom-background

I think this is good for the minor release fix.

--

As mentioned elsewhere on this ticket, it probably needs a refactor in a major release. Current best practice is to place the form controls over the replacement control with opacity: 0 to ensure haptic feedback remains on touch devices and it doesn't look like that is the case here.

#12 @peterwilsoncc
6 weeks ago

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

In 51637:

Customize: Hide native control on background position field.

Hide the browser's native radio button on the custom background position selector in the Customizer and the legacy background screen. This fixes an issue causing both to display.

Props mukesh27, ravipatel, sabernhardt, walbo.
Fixes #53803.

#13 @peterwilsoncc
6 weeks ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for merging in to the 5.8 branch.

#14 @desrosj
6 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 51640:

Customize: Hide native control on background position field.

Hide the browser's native radio button on the custom background position selector in the Customizer and the legacy background screen. This fixes an issue causing both to display.

Props mukesh27, ravipatel, sabernhardt, walbo, peterwilsoncc.
Merges [51637] to the 5.8 branch.
Fixes #53803.

Note: See TracTickets for help on using tickets.