Responsive list tables have no toggle row buttons when there have no row actions

[Chouby]

This makes all non primary columns not visible on mobile devices for list tables having no row actions. This concerns only plugins and although the fix is trivial, there is a risk to break some plugins when WP 4.3 will be released, if the authors are not aware of the issue.

Moving in for review.

What would be a good plugin to test this with?

Replying to obenland:

What would be a good plugin to test this with?

My first test was with Polylang (install it, go in settings->languages, add a language and then go in the strings translation tab to get the table without row actions). However, that's no problem for me since I can update it before the release of WP 4.3.

To know if other plugins are impacted, I just looked to Rewrite rules Inspector and the bulk editor of WordPress SEO (in SEO->Tools) and noticed that the problem is worse for them as no content is rendered at all. But the proposed patch does not fix their issue.

Both Yoast SEO and Rewrite Rule Inspector are overloading the single_row() method that marks a column as primary.

33313.diff​ only hides headings and cells when they have a sibling with .primary-column. That should mitigate custom implementations like Yoast SEO and Rewrite Rule Inspector use.

Since I'm not 100% familiar with how list tables work in that regard, I might be missing something.

33313.patch​ looks good to me.

33313.diff​ is clever, and I like it. Would like some more eyes on testing, as I've got a little near-sightedness at this point. I also don't know that it's really a great result, though that might be okay. Yoast SEO is kind of a strange example because it's specifically small-screen-unfriendly (there's a rather large min-width value in the plugin's CSS), but a good test in that way. The problem with keeping that block layout is that it relies on the data attributes on each cell to show the column name as a label, which of course these very custom implementations don't do. It's not the end of the world, and provides users with data, just with a lot less context.

The funny thing is when in tandem with the other patch, you can end up with a toggle that doesn't do anything, though I think that's preferable to not seeing data. I think plugins that are using list tables like this are going to have to update no matter what, so I'm okay with it being less than elegant. Rewrite Rules Inspector is IMO a particularly egregious use of WP_List_Table (which is rightfully still marked private) - it is a very simple table, no row actions, no sorting, nothing.

Some shoddy screenshots below. Note that the toggle doesn't show for Yoast SEO because of that plugin's min-width value mentioned above.

Before patches:

After patches:

33313.diff​ looks good, but I don't think we have to care about tables with multiple primary columns. 33313.2.diff​ simplifies the selectors.
I've tested this with Yoast SEO and Rewrite rules Inspector and couldn't see a difference with 33313.patch​. Do we still need it?

Replying to ocean90:

I've tested this with Yoast SEO and Rewrite rules Inspector and couldn't see a difference with 33313.patch​. Do we still need it?

33313.patch​ was not initially designed for Yoast SEO or Rewrite rules Inspector but for tables which do not override single_row() and don't have row actions. I noticed that in Polylang when testing it against the RC. Obviously, that's no problem for me as I can patch Polylang before the WordPress release. But that could be a problem for other plugins. I was just trying to find such plugins when I noticed the second issue. I should have open another ticket. Now this one reports two issues and has two patches...

I think we need both, and at this point it's fine to just fix both issues here. I only had one other thing, which was to only show the toggle when it's specified as a primary column so we don't get those non-functional ones. See 33313.3.diff​. With a second review, I think it's good to go.

I was unable to see any breakage in Polylang before applying the patch, so I'm unable to confirm if it is fixed with 33313.3.diff. However, both Yoast SEO & Rewrite Rules Inspector were missing table data. After the patch was applied I'm now see the missing data, though without the toggle button which is expected.

@Chouby How do I break Polylang?

Replying to valendesigns:

@Chouby How do I break Polylang?

The langages table is working good without the patch. To break it, you need to add a language, then go in the strings translations tab (which appears only once a language exists). The strings translations table has no row actions and is broken without the patch. I tested 33313.3.diff​ and that's ok for me.

Replying to Chouby:

The strings translations table has no row actions and is broken without the patch. I tested 33313.3.diff​ and that's ok for me.

Confirmed that the patch fixes the issue, see attached screenshots. (Note: Polylang is hiding the primary colum via CSS for max-width 782px.)

Replying to ocean90:

(Note: Polylang is hiding the primary colum via CSS for max-width 782px.)

That won't be the case in the future as I will change the primary column to adapt the table to this new mobile view.

Tested 33313.3.diff​ and it is working as advertised to fix the problems in both Polylang and wordpress-seo. Approach looks sane in both cases. +1 for commit.

In 33623:

List tables: Yet more primary column fallbacks.

Some custom list tables override enough methods for the column definition fallback to never kick in, so let's ensure that toggling columns only applies when a primary column is defined in some way. We also need to show a toggle button when we can when there are no row actions.

props Chouby, obenland, ocean90.
fixes #33313.

In 33668:

Docs: Clarify the different return conditions in the DocBlock for WP_List_Table->handle_row_actions().

This clarification follows the introduction of primary columns in 4.3. See #33313.

Props morganestes.
Fixes #33436.