Make WordPress Core

Opened 8 years ago

Last modified 5 weeks ago

#35833 assigned enhancement

Adding a table with the "widefat" class on post and taxonomy list table screens causes the quick edit UI and bulk edit to fail

Reported by: braad's profile Braad Owned by:
Milestone: 6.6 Priority: normal
Severity: normal Version: 4.4.2
Component: Quick/Bulk Edit Keywords: has-screenshots has-patch changes-requested good-first-bug
Focuses: ui, javascript, administration Cc:

Description

This is a very strange issue. On a fresh WP install running twentysixteen with no plugins activated, including this code causes the quick edit UI to miscalculate its width and end up looking wacky:

add_action( 'load-edit.php', 'xxx_test_table_in_help_tab' );
function xxx_test_table_in_help_tab() {
    $test_table_args = array(
        'id'       => 'test_help_table',
        'title'    => 'Something',
        'content'  => '<table class="widefat"><tr><td>Something</td></tr></table>',
    );
    get_current_screen()->add_help_tab( $test_table_args );
}

I'm attaching a screenshot of what the quick edit UI looks like with this code in place. Removing the class "widefat" restores the quick edit UI to normal. As strange as it sounds, I believe the presence of the widefat class is causing something in the quick edit JS to miscalculate. I haven't found the exact line where the issue is, but it seems to all come down to the presence of the "widefat" class.

Attachments (6)

wp-quick-edit-issue.png (138.1 KB) - added by Braad 8 years ago.
Screenshot of the issue, after toggling open the quick edit
inline-edit-post.diff (2.7 KB) - added by Braad 8 years ago.
First pass at using more specific selectors
wp-tax-quick-edit-issue.png (235.9 KB) - added by Braad 8 years ago.
Screenshot of the issue on taxonomy edit screens, after toggling open the quick edit
test-35833.php (402 bytes) - added by Braad 8 years ago.
A quick plugin for testing this issue, simply adds a table.widefat in the Help tabs
35833.diff (4.3 KB) - added by Braad 8 years ago.
Improved patch that uses table.wp-list-table in place of table.widefat for both post and taxonomy edit screens
35833.2.diff (6.1 KB) - added by oglekler 12 months ago.

Download all attachments as: .zip

Change History (43)

@Braad
8 years ago

Screenshot of the issue, after toggling open the quick edit

#1 @Braad
8 years ago

If anyone wants to test this without running that code, you can even just inspect element in your browser and paste this into one of the Help tabs .help-tab-content divs:

<table class="widefat"><tr><td>Something</td></tr></table>

Then try toggling the quick edit to see the issue. This is happening for me in Chrome.

#2 @Braad
8 years ago

The issue appears to be originating in /wp-admin/js/inline-edit-post.js, where this selector is used throughout that file:

$('table.widefat')

Since these aren't scoped to just the quick edit section a <table> with the class "widefat" inside the Help tabs (or really anywhere else on the page) is getting targeted also.

There is even a selector that is just ".widefat" on line 291, which could potentially cause other side effects when the class is used on inputs and other elements anywhere on the page.

@Braad
8 years ago

First pass at using more specific selectors

#3 @Braad
8 years ago

Just created my first patch that attempts to fix this issue. I'm not sure what the official method is for minifying core JS so I haven't submitted a minified version of the file yet, but trying the updated code in that file works for me.

The changes I made include switching all instances of the generic $( 'table.widefat' ) and $( '.widefat' ) to $( 'table.wp-list-table.widefat' ). I'm hoping this is safe to do, but it's possible that the "widefat" class is no longer needed in the selector at all and we can just do $( 'table.wp-list-table' ).

I don't expect my patch will be the final one but I wanted to get the ball rolling. If anyone can point me in the right direction to minify the file I can submit another patch that includes that, or if there are other issues with my patch I'm happy to make any necessary changes to take this over the finish line.

#4 @Braad
8 years ago

  • Keywords has-screenshots has-patch added

#5 @Braad
8 years ago

  • Summary changed from Including a table with the "widefat" class in Help tabs causes the quick edit UI to fail to Adding a table with the "widefat" class on post and taxonomy list table screens causes the quick edit UI and bulk edit to fail

@Braad
8 years ago

Screenshot of the issue on taxonomy edit screens, after toggling open the quick edit

@Braad
8 years ago

A quick plugin for testing this issue, simply adds a table.widefat in the Help tabs

@Braad
8 years ago

Improved patch that uses table.wp-list-table in place of table.widefat for both post and taxonomy edit screens

#6 @Braad
8 years ago

After digging a bit deeper I discovered that the table.widefat and .widefat selectors were also being used on taxonomy edit screens in /wp-admin/js/inline-edit-tax.js, and the issue with the quick edit UI was also happening there.

I also discovered that on the posts list table screens the bulk edit was broken (checking box for one or more posts, setting "bulk edit actions" dropdown to "edit" and clicking apply does nothing) in addition to the issue with the quick edit UI happening.

All of these issues happen when there is a second table with the class "widefat" anywhere on the post and taxonomy list table screens, not just in the Help tabs.

I've uploaded a screenshot of the quick edit issue happening on taxonomy edit screens, and I've uploaded test-35833.php as a quick test plugin you can activate to see the issue happening (activate and try to use the quick edit on posts, pages, categories, or tags to see the issue).

I'm also uploading a second patch that fixes a typo in the first patch, switches to consistently using table.wp-list-table as the selector (getting away from selecting by .widefat completely), and updates inline-edit-tax.js also, solving this problem for both post and taxonomy list table screens.

Looking at similar tickets it doesn't seem that people are submitting minified versions of JS files in patches, so I think this new patch has everything it needs. I did grab the develop branch and get the grunt tasks working and all qunit tests still pass.

If there is anything else this patch needs, please let me know. I'm happy to keep working on it. This would be my first core contribution if it gets merged, and I'm ready to do anything necessary to take it over the finish line :)

Last edited 8 years ago by Braad (previous) (diff)

#7 @Braad
8 years ago

  • Focuses ui javascript administration added
  • Keywords dev-feedback added

#8 @oglekler
14 months ago

Hi @Braad thank you for the ticket and the patch,

I agree that .widefat doesn't look like the right selector for JS DOM manipulations and should be used for styling purposed only. But this change will not bring any improvements other than the ability to use this class for styling purposes elsewhere. I understand that your code is just an example and there was a reason to place a table into the Help tab, but the same wide result can be achieved with inline styles for not to make things too complicated and not to add a new style file into the admin purely for this purpose. On the other hand, if this patch to be applied, someone will need to test inline post editing script for an unexpected regression even if it looks like nothing can be broken.

I suggest leaving jQuery selector as is and return to this case if the inline editing script will be reworked for another reason.

#9 @webcommsat
14 months ago

  • Keywords needs-testing added

Component review today
The patch needs testing. @oglekler has left some notes above.
This is one of the tickets to be tested at WCAsia Contributor Day. This could be an easier ticket to test, and if this works, then to fix. Note from @oglekler: someone will need to test inline post editing script for an unexpected regression even if it appears that nothing is broken.

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


14 months ago

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


13 months ago

#12 @SergeyBiryukov
13 months ago

  • Milestone set to 6.3

This ticket was mentioned in Slack in #core-test by oglekler. View the logs.


13 months ago

@oglekler
12 months ago

#14 @oglekler
12 months ago

Patch (35833.2.diff) is updated with regard to new files location, plus to remove '.widefat' selector from JS files completely. With this selector was another compatability issue in src/js/_enqueues/admin/inline-edit-tax.js, see: https://core.trac.wordpress.org/ticket/35664#comment:13

For testing:
src/js/_enqueues/admin/edit-comments.js - Comments list
src/js/_enqueues/admin/inline-edit-post.js - Posts list
src/js/_enqueues/admin/inline-edit-tax.js - Taxonomies lists
src/js/_enqueues/admin/post.js - Comments list under the Classic editor

This ticket was mentioned in PR #4226 on WordPress/wordpress-develop by @oglekler.


12 months ago
#15

PR is based on the previous PR, patch is updated with regard to new files location,
.widefat selector is removed from JS files and replaced by classes which reference DOM elements and not how they should look.

Trac ticket: https://core.trac.wordpress.org/ticket/35833

#16 @oglekler
12 months ago

  • Keywords dev-feedback removed

#17 @oksankaa
12 months ago

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/4226

Environment

  • OS: macOS 12.2.1
  • Web Server: Nginx
  • PHP: 7.4.33
  • WordPress: 6.3-alpha-55505-src
  • Browser: Chrome 111.0.5563.146
  • Theme: Twenty Twenty-One (but it doesn't matter)
  • Active Plugins:
    • Classic Editor 1.6.3
    • Query Monitor 3.12.1
    • Test plugin by Braad

Actual Results

  • Issue resolved with the patch.

The changes are applied in these files:

  • src/js/_enqueues/admin/edit-comments.js
  • src/js/_enqueues/admin/inline-edit-post.js
  • src/js/_enqueues/admin/inline-edit-tax.js
  • src/js/_enqueues/admin/post.js

Then the npm run build:dev command was run.

Tested the issue is no longer reproduced in these areas:

  • ✅ Comments list
  • ✅ Taxonomies lists
  • ✅ Single post editing and its Comments list in the Classic editor
  • ✅ Comments list for posts/pages under the Classic editor
  • ✅ Comments list for media items under the Edit Media page
Last edited 12 months ago by oksankaa (previous) (diff)

#18 @oglekler
12 months ago

Hi @oksankaa,
Did you run npm run build:dev?
These changes are in source files and actual scripts need to be built.

#19 @oksankaa
12 months ago

@oglekler Oh, you are right! I didn't know I needed to run that command, thanks for pointing this out.

I've updated the test report above (to avoid unnecessary duplication), plus added yet another place I checked.

#20 @oglekler
9 months ago

From my point of view, this is ready to go into 6.3. But the final patch is mine, so, please take a look.

#21 @oglekler
9 months ago

  • Owner set to oglekler
  • Status changed from new to assigned

#22 follow-up: @oglekler
9 months ago

  • Keywords commit added

I've re-checked the patch and marking this for review to commit.

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


9 months ago

#24 @mukesh27
9 months ago

This ticket was discussed during the bug scrub.

@SergeyBiryukov could you please take look?

#25 in reply to: ↑ 22 @azaozz
8 months ago

Replying to oglekler:

Sure, the patch makes sense here and seems to work well. Just wondering if the same problem may happen because some other plugin is adding tables with a wp-list-table class on these screens. Seems possible as there are quite a few matches in a WP Directory search. Perhaps a new class name should be added to these list tables specifically to target them from the WP JS.

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

#26 follow-up: @oglekler
8 months ago

@azaozz it may happen if some of these plugins are adding similar content into the same pages, for example 'inline-edit-tax' will be only enqueued on the edit-tags.php page. I think it is unlikely, but I also believe that the right approach to separate classes for CSS and JS and don't mix up them. What is your suggestion, change it now?

#27 in reply to: ↑ 26 @azaozz
8 months ago

Replying to oglekler:

Yes, you're right. Best would be to change it. Yea, it is "best practice" to use separate classes that are used only in JS.
(In theory that won't stop some plugin from using it for something else, but that can happen even with the best intentions.)

#28 @oglekler
8 months ago

  • Keywords changes-requested added; needs-testing commit removed
  • Milestone changed from 6.3 to 6.4
  • Type changed from defect (bug) to enhancement

I had no time to work on this further, so I am moving it into 6.4, and now it looks more like enhancement that bug fix.

#29 @oglekler
7 months ago

  • Keywords good-first-bug added

The scope of the ticket is clear now, and there is already a patch with which work can be continued, so, I am marking this as good-first-bug.

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


7 months ago

#31 @oglekler
6 months ago

  • Milestone changed from 6.4 to 6.5

Because I didn't have time to work on this patch and no one picked up this so far, I am moving this ticket into 6.5

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


4 months ago

#33 follow-up: @oglekler
4 months ago

  • Owner oglekler deleted

This ticket was discussed during bug scrub.

This is a straightforward patch, it most likely will need rebase due to other js patch we are working on, but the change is simple enough — to add dedicated class to list tables and to replace it in the scripts. JS files that heed changes are there: https://github.com/WordPress/wordpress-develop/pull/4226/files

@azaozz Do we have some naming convention for this JS used classes?

I am removing myself from owners for not to confuse new contributors that this ticket "is taken".

Add props to @jorbin

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


3 months ago

#35 @webcommsat
3 months ago

Highlighted at today's bug scrub (December 19, 2023). There is some advice above on updating a patch. If you can give time to help move this forward, please comment on the ticket.

@Braad thanks for highlighting and the work you have done to progress this ticket. There are some comments above on the latest patch if you would are able to help move this forward to 6.5.

@azaozz grateful if you could look at the question in the above comment from Olga on naming conventions.

#36 in reply to: ↑ 33 @azaozz
3 months ago

Replying to oglekler:

Do we have some naming convention for this JS used classes?

Don't think so. As with any other names in WP, it would be quite nice to make them self-documenting as much as possible, and even add an inline comment about the expected use.

This is a straightforward patch...

Right. Still thinking this will be much better if a specific classmane is used instead of .wp-list-table. Perhaps something like inline-edit-list-table or maybe wp-inline-edit?

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

#37 @swissspidy
5 weeks ago

  • Milestone changed from 6.5 to 6.6

Moving out of the milestone as there is no consensus or traction and 6.5 Beta is approaching.

Note: See TracTickets for help on using tickets.