Make WordPress Core

Opened 10 years ago

Last modified 6 years ago

#29030 new defect (bug)

Screen Options Poor Update/Rendering Causes Many things to Break

Reported by: codecandid's profile codecandid Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.9.1
Component: Administration Keywords: dev-feedback has-patch
Focuses: ui, javascript, administration Cc:

Description (last modified by SergeyBiryukov)

Screen options dont work properly in many different situations. I noticed the first issue when trying to create a sticky header plugin for the wp_list_table. When scrolling down the page the headers stick to the top by cloning the header with javascript and hiding the other original at the same time. However, If screen options are updated the tables break completly even after the plugin is disabled. Wordpress checks the current table headers to determine which ones are hidden and should be added to the manageedit-{$post_type}columnshidden field in the user_meta table. So since the cloned table header the plugin created is hidden visually while scrolling up, wordpress thinks that all columns aredisabled and adds all the columns to manageedit-{$post_type}columnshidden.

http://i.stack.imgur.com/wrYin.png

This is poor practice because it doesn't seperate presentation well enough from the logic used to render screen options. Any user who has access to wp-admin/edit.phpcan completly break their tables if any html/css visually hides the <thead> or a column-header perhaps by a plugin, or maybe the browser doesn't load a certain script, or perhaps they are just messing with the dev-tools. Beginers that don't know how to properly [remove columns][5], could run into this issue if they ever try to use css instead. manageedit-{$post_type}column should not rely on the visibility of <thead> and only the actual checked input fields. Also cb and title should not be allowed to be added to the manageedit-{$post_type}column. They should only be able to be removed with unset.


To recreate this issue:

  1. open up firebug/chrome dev tools/etc. on http://www.example.com/wp-admin/edit.php
  2. add thead {display: none;} to the style editor
  3. On the page screen options uncheck at least one column ( this is to ensure manageedit-{$post_type}columnshidden is a database field for the current user and if not it creates it )
  4. Hit apply to refresh the page

*The tables will now be broken....*


To chck the columns I used the get_user_meta(); function to print the array of hiddencolumns on each post types edit.php admin screen notices:

    <?php
    function get_current_post_type() {
            global $post, $typenow, $current_screen;
            if ($post && $post->post_type)
                return $post->post_type;
             elseif ($typenow)
                return $typenow;
             elseif ($current_screen && $current_screen->post_type)
                return $current_screen->post_type;
             elseif (isset($_REQUEST['post_type']))
                return sanitize_key($_REQUEST['post_type']);
            return null;
    }
    
    function get_current_user_manageedit_pagecolumnshidden() {
    	$current_ptype = get_current_post_type();
    	$user_id = get_current_user_id();
    	$key = 'manageedit-'.$current_ptype.'columnshidden';
    	$single = true;
    	if(get_user_meta($user_id, $key, $single))
    		return get_user_meta($user_id, $key, $single);
    }

     function echo_current_user_manageedit_pagecolumnshidden() {
        	global $pagenow;
        	if ( $pagenow !== 'edit.php' )
        		return;
        	$columnshidden= get_current_user_manageedit_pagecolumnshidden();
        	echo '<pre>'; print_r( $columnshidden ); echo '</pre>';
        }
        add_action('all_admin_notices', 'echo_current_user_manageedit_pagecolumnshidden');


Output for the broken tables :

    Array
    (
        [0] => cb
        [1] => title
        [2] => 
        [3] => 
    )


After determining that cb & title were in fact added to the $meta_valueyou need to fix the table. This will do the trick:


   function delete_current_user_manageedit_pagecolumnshidden() {
    	$user_id = get_current_user_id();
    	$meta_key = 'manageedit-pagecolumnshidden';
    	if( get_user_meta($user_id, $meta_key) )
    		delete_user_meta( $user_id, $meta_key );
    }
    add_action ('admin_init', 'delete_current_user_manageedit_pagecolumnshidden');

Side-Notes:
*columnshidden appears [wp_ajax_hidden_columns()][1] & [get_hidden_columns()][2]
*client-side functionality appears to be here in [common.js][3] which checks for the [hidden table headers][4]


Similar issues with the screen options can be recreated for different situations that have nothing to do with the tables.

Recreate similar issue on nav-menus.php

  1. Go to http://example.com/wp-admin/nav-menus.php
  2. Uncheck all the fields in the *"Show advanced menu properties"* Screen-Options tab
  3. Add the screen options filter to hide them from display: add_filter('screen_options_show_screen', 'remove_screen_options_tab');
  4. Reload http://example.com/wp-admin/nav-menus.php

All of the hidden advanced menu properties will now be broken and are all visible even though they were unchecked. I'm not sure if this is the same issue, but it appears that overall screen options have a high change of not working properly



Other-Notes
These issues of broken tables might also have to do with the same functionality problem of how screen options update/render:

http://wordpress.stackexchange.com/questions/31154/wp-list-table-custom-quick-edit-box-post-meta-data-missing-and-columns-change

http://wordpress.stackexchange.com/questions/123182/custom-admin-column-disappearing-when-using-quick-edit?lq=1

http://wordpress.stackexchange.com/questions/144361/wordpress-admin-wp-table-list-show-incorrectly

#21016

[1]: https://github.com/WordPress/WordPress/blob/448275cce483138f53ccfa586b2d28b7fe8b0785/wp-admin/includes/screen.php#L55
[2]: https://github.com/WordPress/WordPress/blob/270a57075c290736387b6551670fde34fb3f1851/wp-admin/includes/ajax-actions.php#L1307
[3]: https://github.com/WordPress/WordPress/blob/448275cce483138f53ccfa586b2d28b7fe8b0785/wp-admin/js/common.js#L29
[4]: https://github.com/WordPress/WordPress/blob/448275cce483138f53ccfa586b2d28b7fe8b0785/wp-admin/includes/screen.php#L17
[5]: http://codex.wordpress.org/Plugin_API/Filter_Reference/manage_$post_type_posts_columns

Attachments (1)

29030.diff (2.7 KB) - added by shariqkhan2012 6 years ago.

Download all attachments as: .zip

Change History (8)

#1 @codecandid
10 years ago

By the way here is the original plugin that I was trying to make that I first noticed the problem.

https://github.com/bryanwillis/sticky-admin-table-headers

#2 @SergeyBiryukov
10 years ago

  • Description modified (diff)
  • Focuses performance removed

Confirmed.

#3 @codecandid
10 years ago

  • Keywords dev-feedback added

Checking my hunch, I do believe this is in fact a javascript issue:

Ajax handler looks for "hidden columns here:https://github.com/WordPress/WordPress/blob/448275cce483138f53ccfa586b2d28b7fe8b0785/wp-admin/js/common.js#L29

.manage-column css selector of each <th> uses :hidden to check hidden table header's:

hidden : function() {
		return $('.manage-column').filter(':hidden').map(function() { return this.id; }).get().join(',');
	},

manageedit-{$post_type}columnshidden ajax should not check :hidden table columns but only checked input fields in the screen options form to better seperate presentation and data handling. This way cb and title should also never get added to manageedit-{$post_type}columnshidden.

I will run some tests to make sure this is in fact it and try to write a patch later this weekend. My jquery is average at best, so if anyone else can figure this out be my guest!

#4 @michalzuber
10 years ago

Reproduced and the column state stayed in the messed up state http://i.imgur.com/alSoT3l.png
I had to search the core for finding solution to restore the checkbox and title column :D
Removing manageedit-postcolumnshidden row from wp_usermeta database table helped.
Screencast: https://youtu.be/lZKjUpFx_yc

#5 @chriscct7
9 years ago

  • Keywords needs-patch added

@shariqkhan2012
6 years ago

#6 @shariqkhan2012
6 years ago

  • Keywords has-patch added; needs-patch removed

Based on @codecandid 's findings, I added a patch.
In addition to the code fix in common.js, I made a few more changes. I thought it is better to handle the logic to hide the columns at server-side in addition to client-side.

From what I have tested, this patch seems to fix the issue.

Last edited 6 years ago by shariqkhan2012 (previous) (diff)

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


6 years ago

Note: See TracTickets for help on using tickets.