WP_Comments_List_Table should not show views that have a count of 0

[pbiron]

Other core list tables that have a get_views() method do not output a view if the count for that view is 0, e.g., WP_Posts_List_Table doesn't output "Pending (0)" if there are no posts with $post_status === 'pending').
However, WP_Comments_List_Table does output "Pending (0)" if there are no pending comments.

For consistency's sake, I think WP_Comments_List_Table should skip views with count of 0.

Related: #47495

[pbiron]

This list table is a little different from others in that not only do it's views are displayed not only on the screen that displays the list table but they are also displayed in the "Recent Activity" dashboard widget. I'll have to check whether making the suggested change would have an adverse impact on the dashboard widget.

[SergeyBiryukov]

I think the process of moderating comments is generally different from working with pending posts, so it might be beneficial to always display "Pending (0)" to make sure there are no more comments left to moderate, even if that's inconsistent with the posts table.

[pbiron]

Replying to SergeyBiryukov:

I think the process of moderating comments is generally different from working with pending posts, so it might be beneficial to always display "Pending (0)" to make sure there are no more comments left to moderate, even if that's inconsistent with the posts table.

That's similar to what @garret-eclipse said in #47495 in relation to the privacy requests list table, and I don't see the reasoning.

If the Pending view is not displayed then there are no more comments left to moderate. In fact, I would think it would be easier to know that there were none left to moderate if the view were not displayed when the count is 0 (but I don't do comment moderation on real world sites, so maybe those who do have a different perspective).

[SergeyBiryukov]

The design direction in #47495 was to consistently hide zero-count views on all tables, so this should be good to proceed.

[pbiron]

OK, I try to get a patch up later today.

[pbiron]

@SergeyBiryukov Can you use your trac searching super powers to try to find out the history of class-wp-comments-list-table.php, L305?

It looks like it's been there for 10+ years...but is very wrong! If I'm reading it correctly, if wp_count_comments() doesn't return a count for some specific status (e.g., because it's been filtered by wp_count_comments), then the count for that status is arbitrarily set to 10...shouldn't it be set to 0 in that case?

that line makes it possible that status views that should be hidden (with the patch I'm working on) won't be...for reasons that are incorrect.

[SergeyBiryukov]

Replying to pbiron:

It looks like it's been there for 10+ years...but is very wrong! If I'm reading it correctly, if wp_count_comments() doesn't return a count for some specific status (e.g., because it's been filtered by wp_count_comments), then the count for that status is arbitrarily set to 10...shouldn't it be set to 0 in that case?

Good catch, introduced in [9556]. Somehow it never came up before, but yes, that doesn't look right :)

[chetan200891]

Initial patch.

[pbiron]

Hi @chetan200891, thanx for the patch. I'm working on this right now myself.

For this list table, there is JS (in /wp-admin/js/edit-comment.js) that updates the counts for the views when comments are approved/unapproved/marked as spam/etc. And if the views with initial counts of 0 are simply not output, then that JS ends up making the counts seem inconsistent when comments approved/unapproved/etc :-(

I close to having the JS (and 1 change in /wp-admin/css/common.css) updated to work correctly...we'll see if I can get it completely working :-)

[SergeyBiryukov]

50538.2.diff​ is just a simplified version of 50538.diff​, still doesn't touch the JS code.

[pbiron]

50538.3.diff hides views with count of 0, in such a way that /wp-admin/js/edit-comments.js can show them as comments are approved/unapproved/etc.

This solution is very much a kludge, and may not be worth it. Looking for opinions on that.

1. instead of simply not outputting 0 count views in PHP (like other list tables), it adds a hidden class to their li elements, along with CSS to hide them
2. modifies the JS to show/hide the appropriate views when it updates their counts
   • this is complicated by the fact that the "separators" between views are done with the text content in the li elements (instead of via CSS as I think core should do it)
   • because of that, it first wraps the "separators" in a span element, so that it can be hidden for the last "visible" view
   • one consequence of this is that there is a split second when the screen first loads, that the separator for the last "visible" view is displayed...and then it is hidden

I've tested this pretty thoroughly on both the Comments and Dashboard screens, but more testing is certainly warranted.

Also, because of the kludgy nature of the solution, the the selectors (in both the CSS and JS) are more qualified that I would normally do, but I didn't want them to unintentionally affect anything else (e.g., a dashboard widget added by a plugin that just happened to use <ul class="subsubsub">).

[SergeyBiryukov]

Since this isn't as straightforward as it looked at a glance, moving to 5.6 to allow for more testing.

[pbiron]

Replying to SergeyBiryukov:

Since this isn't as straightforward as it looked at a glance, moving to 5.6 to allow for more testing.

Good call. I was going to refresh the patch after [48348], but will hold off on that for now.

[hellofromTonya]

Capturing notes from scrub to see if we can move this ticket forward to land in 5.6:

Per Helen:

I think it’s actually worth discussing again with #design and making sure we’re balancing future maintenance of this code (which is… a lot) with the desire to remove it.

Adding needs-design-feedback.

Per Jorbin

I think this would be good, but I'm not sure this is high enough priority to work on

Per JB

not high priority but still a nice enhancement
maybe it will land in future release, but let’s try to ship it in 5.6

Per @pbiron

since https://core.trac.wordpress.org/changeset/48348 is committed now, I'll try to refresh the patch soon

[garrett-eclipse]

I will revoice my concern with suppressing potential status' as providing them with '0's gives you an idea as to what potential status' to expect.

Refering to comment:4;core#47495:

Thanks @pbiron for the ticket and patch, applying and testing the patch works as outlined.

I am a bit hesitant of moving forward here as I see Requests being more in line with Comments than posts and the Comments list table does display the additional status' for Pending, Approved, Spam and Trash.

In my eyes displaying all status' is helpful to the user experience as it outlines the expected process visually by showing what status buckets the requests will need to pass through.

I'm marking for 2nd-opinion and also flagging to get some design feedback on the UX here. I'm all for consistency so if it's decided to suppress empty status labels then we should also address the Comments list table or potentially vice versa if displaying them is useful for to the user for the interface then resurfacing them on other list tables may be the avenue forward.

That being said following my comment there was design discussion;
Replying to comment:8#47495@mapk:

The Design Team discussed this [in Slack today]​https://wordpress.slack.com/archives/C02S78ZAL/p1593708788256000. We're in agreement that the 0 count views should be hidden.

Maybe @mapk or @pbiron can elaborate on that design decision, or it can be re-raised into a design meeting.

[paaljoachim]

First of all seeing all the sections is helpful if they contain anything or not.

Removing the empty sections or removing the 0 can bring an uncertainty. It is good to have all the sections visible all the time. Showing the valid number be it a 0 or another number. A 0 gives a confirmation to the user that the specific section is empty.

[karmatosed]

I actually could go either way on this but looks like there was a design consensus to remove in a fairly extensive discussion so I would stick with that as a result for now. If I am correct that is also good to suppress on the comments list table.

@paaljoachim whilst I understand your view here, I think at this stage exposing here and not everywhere could also prove troublesome. It's worth deeper exploration considering a discussion previously decided on removing.

[helen]

I am punting this from 5.6 and I would actually recommend closing. There are two main reasons for this:

• The code required to make this work is a textbook example of technical debt - it is unpleasant to look at and it's going to be very unpleasant to maintain into the future.
• I don't agree that this is usably better for comments specifically. I can buy that consistency is good for many other 0-count view filters, but for comments the entire model is pretty different and you have interactions with keyboard shortcuts and mass moderation, etc. so my view on this is that knowing what states are even possible is actually important here.

The combination of those two points and that we're looking at beta 1 tomorrow makes this a punt for now and a recommended closure from me :)

[SergeyBiryukov]

Related: #27831