WordPress.org

Make WordPress Core

#51863 closed defect (bug) (fixed)

jQuery UI dialog close button style is broken

Reported by: marijnkoopman Owned by: azaozz
Milestone: 5.6 Priority: normal
Severity: normal Version: 5.6
Component: External Libraries Keywords: has-screenshots has-patch commit dev-reviewed
Focuses: ui, css Cc:

Description (last modified by SergeyBiryukov)

In 5.6RC1 and 5.6 Nightly the styling of the Close button from a jQuery UI-dialog is different, giving another view of Dialogs. Instead of showing only a cross, like expected, it shows the cross as well as the text “Close”, see attached screenshot.

In 5.5 this text was wrapped in a span element that was hidden using display: none;. In 5.6 with upgraded jQuery this text is in the button element itself, rendering that solution useless. jQuery UI solves that by putting the text away from the screen by use of a gigantic negative text-indent. In WordPress’ Jquery UI Dialog stylesheet there is no CSS at all for .ui-button-icon-only, as there is on jQueryUI.com. I swiftly tried copying the CSS used by jQueryUI but that doesn’t work in WordPress, since WordPress seems to rely on position: absolute; in the dialog header.

Attachments (2)

Screenshot 2020-11-24 at 13.38.18.png (2.0 MB) - added by marijnkoopman 11 months ago.
UI-dialog comparison screenshot
51863.diff (551 bytes) - added by azaozz 11 months ago.

Change History (14)

@marijnkoopman
11 months ago

UI-dialog comparison screenshot

#1 @SergeyBiryukov
11 months ago

  • Component changed from General to External Libraries
  • Description modified (diff)
  • Milestone changed from Awaiting Review to 5.6

Hi there, welcome to WordPress Trac!

Thanks for the report, this appears to be a result of updating jQuery UI in [49101] / #50564.

Moving to the milestone for visibility.

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


11 months ago

#3 @azaozz
11 months ago

@marijnkoopman yes, can reproduce, thanks for reporting this :)

Unfortunately WP doesn't include the UI CSS. It only includes few bits and pieces that have been added over the years in 4-5 different places/files. Most have been edited to fit/match the rest of the wp-admin CSS. See: [31569], [30343], [23515], etc.

Fortunately the different UI components inherit the wp-admin CSS quite well, in most cases.

As you mention this bug is because the button text is not wrapped in a span in button.js in UI 1.12.1 (and the .ui-button-text class is not used any more). However there is a setting for UI Dialog to specify the text for that particular close button that also allows empty string: https://api.jqueryui.com/dialog/#option-closeText. Ideally plugins would use it.

Don't see a really good way to fix this. The styles from UI 1.12.1 don't work in WP. A possible fix seems to be to add overflow: hidden; as that button is limited in size to 36x36px.

Version 1, edited 11 months ago by azaozz (previous) (next) (diff)

@azaozz
11 months ago

#4 @azaozz
11 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

In 51863.diff: restore previous behavior and hide the button text in the UI Dialog close button.

Please test! Other ideas or patches about how to do this better are welcome.

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


11 months ago

#6 follow-up: @Clorith
11 months ago

I think the overflow fix is the most sensible, given what we have to work with (it also means we don't explicitly hide the text it self, so it would still provide context for accessibility reasons I believe? Although I could be mistaken about how good they are at detecting elements nit on view).

#7 in reply to: ↑ 6 @azaozz
11 months ago

Replying to Clorith:

Thanks for the review. Thinking to commit this to trunk for easier testing.

Although I could be mistaken about how good they are at detecting elements...

As far as I know any element that doesn't have a hidden attribute or display: none is "readable" for screenreaders. Not sure if this can be called an a11y improvement, although it looks that way :)

Last edited 11 months ago by azaozz (previous) (diff)

#8 @azaozz
11 months ago

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

In 49704:

External Libraries: Fix hiding of the text in the jQuery UI dialog close button.

Props marijnkoopman, SergeyBiryukov, Clorith, azaozz.
Fixes #51863.

#9 follow-up: @azaozz
11 months ago

  • Keywords 2nd-opinion commit added; needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for the 5.6 branch.

#10 in reply to: ↑ 9 ; follow-up: @SergeyBiryukov
11 months ago

  • Keywords dev-reviewed added; 2nd-opinion removed

Replying to azaozz:

Reopen for the 5.6 branch.

[49704] looks good to backport.

Hint: If you add the dev-feedback keyword instead of 2nd-opinion, the ticket will show up in "Commit Candidates which need Dev Review" section on Next Major Release, Workflow Oriented report :)

#11 in reply to: ↑ 10 @azaozz
11 months ago

Replying to SergeyBiryukov:

If you add the dev-feedback keyword instead of 2nd-opinion

Right, sorry, guess I'm getting too tired :)

#12 @azaozz
11 months ago

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

In 49707:

External Libraries: Fix hiding of the text in the jQuery UI dialog close button.

Props marijnkoopman, SergeyBiryukov, Clorith, azaozz.
Merges [49704] to the 5.6 branch.
Fixes #51863.

Note: See TracTickets for help on using tickets.