Make WordPress Core

Opened 10 years ago

Closed 18 months ago

#27697 closed defect (bug) (fixed)

wp-admin.css class="hidden" bug renders screen options menu in admin blank

Reported by: grabmedia's profile grabmedia Owned by: audrasjb's profile audrasjb
Milestone: 6.1 Priority: normal
Severity: normal Version: 3.8.1
Component: General Keywords: has-patch has-testing-info commit
Focuses: ui, javascript, administration Cc:

Description

One of the publishers who uses our GrabPress plugin brought a serious issue to our attention. Initially they thought it was a bug with our plugin, but after looking further into the issue, it's clear that the issue is within the WordPress admin files.

The Screen Options menu at the top of the WordPress admin dashboard becomes unusable, and the form elements appear hidden. Reproducing this issue was difficult to track, mainly because it just depends on when other CSS files conflict with it (which can be many). It leaves the '.hidden' class out there to potentially conflict against any other CSS file in the environment that uses the 'visibility:hidden' property instead of or in addition to 'display:none'

One of our developers took a closer look and found the following.

In the PHP file where the original state of the HTML for the screen options UI that is experiencing the issue is formed you'll see that the class="hidden" is present.
wp-admin/includes/screen.php (line 917)

<div id="screen-options-wrap" class="hidden" tabindex="-1" aria-label="<?php esc_attr_e('Screen Options Tab'); ?>">

They have this class tied up to the following CSS: wp-admin/css/wp-admin.css (line 234 thru 243)

.hidden,
.js .closed .inside,
.js .hide-if-js,
.no-js .hide-if-no-js,
.js.wp-core-ui .hide-if-js,
.js .wp-core-ui .hide-if-js,
.no-js.wp-core-ui .hide-if-no-js,
.no-js .wp-core-ui .hide-if-no-js {
	display: none;
}

So you can see that the '.hidden' class (along with several others) has the property 'display: none;'. This is perfectly fine and normal if they plan to toggle on and off the '.hidden' class from the element. However they instead make use of jQuery's 'show()' and 'hide()' methods and leave the 'hidden' class.
wp-admin/js/common.js (lines 99 thru 143)

screenMeta = {
	element: null, // #screen-meta
	toggles: null, // .screen-meta-toggle
	page:    null, // #wpcontent

	init: function() {
		this.element = $('#screen-meta');
		this.toggles = $('.screen-meta-toggle a');
		this.page    = $('#wpcontent');

		this.toggles.click( this.toggleEvent );
	},

	toggleEvent: function( e ) {
		var panel = $( this.href.replace(/.+#/, '#') );
		e.preventDefault();

		if ( !panel.length )
			return;

		if ( panel.is(':visible') )
			screenMeta.close( panel, $(this) );
		else
			screenMeta.open( panel, $(this) );
	},

	open: function( panel, link ) {

		$('.screen-meta-toggle').not( link.parent() ).css('visibility', 'hidden');

		panel.parent().show();
		panel.slideDown( 'fast', function() {
			panel.focus();
			link.addClass('screen-meta-active').attr('aria-expanded', true);
		});
	},

	close: function( panel, link ) {
		panel.slideUp( 'fast', function() {
			link.removeClass('screen-meta-active').attr('aria-expanded', false);
			$('.screen-meta-toggle').css('visibility', '');
			panel.parent().hide();
		});
	}
};

The use of these methods is also okay. The 'show()' and 'hide()' methods toggle the element's display value between 'display:none' and 'display:block'. The issue here is that WP used the '.hidden' class to set the default display state and then use the Jquery toggle methods to adjust on the fly. However, this leaves the '.hidden' class out there to snag itself in conflict against any CSS at play that uses the 'visibility:hidden' property instead of or in addition to 'display:none'. Twitter Bootstrap for example, which GrabPress uses and is widely used across the web has the following declaration for '.hidden':

.hidden {
  display: none;
  visibility: hidden;
}

So my recommendation to WP would be to have their JavaScript toggle on/off the class 'hidden' from the element rather than leaving it there. To have 'hidden' on a unhidden element just makes no sense, even semantically speaking. It might also be good for them to toggle both the 'display' property and the 'visibility' property of the element.

Attachments (7)

GBPS-263-screenoptions-missing.png (128.1 KB) - added by grabmedia 10 years ago.
screen.php (27.7 KB) - added by grabmedia 10 years ago.
the changes attached here to screen.php would likely resolve this issue
27697.diff (770 bytes) - added by sabernhardt 2 years ago.
27697.1.diff (740 bytes) - added by sabernhardt 2 years ago.
removeClass needed to be inside slideDown function
27697.2.diff (744 bytes) - added by sabernhardt 22 months ago.
adding spaces for consistency and readability
ab3716026ba06f4832f5c0478673278f.gif (871.5 KB) - added by audrasjb 18 months ago.
Before patch, using @costdev test plugin
44b1892c233fc31c67b69d3c3aaa4d0b.gif (916.1 KB) - added by audrasjb 18 months ago.
After patch: works fine!

Download all attachments as: .zip

Change History (20)

@grabmedia
10 years ago

the changes attached here to screen.php would likely resolve this issue

This ticket was mentioned in IRC in #wordpress-dev by Clorith. View the logs.


10 years ago

#2 @SergeyBiryukov
10 years ago

  • Component changed from Menus to Administration
  • Focuses ui added

Could you please attach an SVN patch rather than a PHP file?

See http://make.wordpress.org/core/handbook/submitting-a-patch/.

#3 @SergeyBiryukov
10 years ago

  • Keywords needs-patch added

I saw this issue before, caused by .hidden class usage in some theme framework. Would be nice to fix this.

#4 @helen
9 years ago

  • Component changed from Administration to General
  • Focuses javascript administration added

I agree that we should be toggling classes more often than setting CSS directly with JS (which is effectively what show() and hide() do). Would like to see a patch here. If we are worried about unintended side effects, we can add an .is-hidden class.

However, I do want to note that on the whole I'm not really concerned about working around problems that arise from loading another admin framework (like Bootstrap) within the WordPress admin. I know core doesn't always provide all the UI tools needed, but there's enough generically named stuff in wp-admin.css that loading up another framework inside is almost definitely going to have side effects.

#5 @sabernhardt
3 years ago

  • Milestone set to Future Release

#6 @sabernhardt
2 years ago

#54165 was marked as a duplicate.

@sabernhardt
2 years ago

#7 @sabernhardt
2 years ago

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

I expect that the script could be better, but 27697.diff simply removes the hidden class when the panel element is not hidden (and adds the class again when the panel closes).

@sabernhardt
2 years ago

removeClass needed to be inside slideDown function

@sabernhardt
22 months ago

adding spaces for consistency and readability

#8 @sabernhardt
22 months ago

  • Milestone changed from Future Release to 6.1

I would welcome a script change that replaces the show() and hide() with classes, plus I would prefer replacing slideUp() and slideDown() with animation that respects reduced motion settings. However, I only updated the current patch because anything with a hidden class should not be expected to display.

For testing 27697.2.diff: Add .hidden {display:none !important;} to a stylesheet, either in a custom plugin or in a browser extension such as Stylus. (I could not find a plugin that always hides elements with the hidden class, and the browser extension mentioned on #54165 apparently fixed their implementation.)

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


19 months ago

#10 @costdev
19 months ago

  • Keywords has-testing-info added

This ticket was reviewed in the recent bug scrub.

The ticket has testing info for 27697.2.diff. However, this could be explained a little more for testers. Adding has-testing-info and I'll post more detailed testing instructions below.

Additional props: @chaion07, @mukesh, @hilayt24

Last edited 19 months ago by costdev (previous) (diff)

#11 @costdev
19 months ago

Test Report

Patch tested: 27697.2.diff

Steps to Reproduce or Test

  1. In wp-content/plugins, create a new file called hidden-test.php with the following contents:
    <?php
    
    /**
     * Plugin Name: Hidden Test
     * Description: Add <code>.hidden</code> styling to admin area.
     * Author:      WordPress Core Contributors
     * Author URI:  https://make.wordpress.org/core
     * License:     GPLv2 or later
     * Version:     1.0.0
     */
    
    add_action( 'admin_head', 'wpcore_add_hidden_class_to_admin' );
    function wpcore_add_hidden_class_to_admin() {
            echo '<style>.hidden{display:none;visibility:hidden}</style>';
    }
    
  2. In the admin area, navigate to Plugins > Installed Plugins and activate Hidden Test.
  3. 🐞 To the top right, click Screen Options.
  4. When testing the patch:
    • Run grunt patch:27697 and select 27697.2.diff.
    • Run sudo npm run build:dev.
    • Refresh the admin page and to the top right, click Screen Options.

Expected Results

When reproducing a bug:

  • ❌ The Screen Options tab opens, but the content is hidden.

When testing a patch to validate it works as expected:

  • ✅ The Screen Options tab opens, and the content is visible.

Environment

  • Server: Apache (Linux)
  • WordPress: 6.1-alpha-53344-src
  • Browser: Chrome 104.0.0.0
  • OS: Windows 10
  • Theme: Twenty Twenty-One
  • Plugins:
    • Hidden Test 1.0.0

Actual Results

When reproducing a bug/defect:

  • ❌ The Screen Options tab opens, but the content is hidden. Issue reproduced.

When testing the bugfix patch:

  • ✅ The Screen Options tab opens, and the content is visible. Issue resolved with patch.

Test Report Icons:
🐞 <= Indicates where issue ("bug") occurs.
✅ <= Behavior is expected.
❌ <= Behavior is NOT expected.

@audrasjb
18 months ago

Before patch, using @costdev test plugin

@audrasjb
18 months ago

After patch: works fine!

#12 @audrasjb
18 months ago

  • Keywords commit added; needs-testing removed
  • Owner set to audrasjb
  • Status changed from new to accepted

Good to go 👍

#13 @audrasjb
18 months ago

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

In 54177:

Help/About: Remove .hidden class when the Help Tab panel is displayed.

It is obviously more logical and semantic to remove the hidden class when the panel is displayed. Plus, it prevents from weird behavior occurring when plugins also use the hidden class to hide stuff.

Props grabmedia, SergeyBiryukov, helen, sabernhardt, costdev, audrasjb.
Fixes #27697.

Note: See TracTickets for help on using tickets.