WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#15876 closed defect (bug) (fixed)

Problems with comment editing / replying / approving / trashing when editing post

Reported by: garyc40 Owned by: garyc40
Milestone: 3.1 Priority: highest omg bbq
Severity: blocker Version: 3.1
Component: Administration Keywords: has-patch commit
Focuses: Cc:

Description

Problem #1 : Reply to comment

Steps to reproduce:

  1. Edit a post that has some comments
  2. Click "Reply" on a comment

-> The comment form's width is only about a half of that of the comment list

  1. Type the reply and hit "Submit Reply"

-> All the comments' width became about half of the meta box. Only the new comment is displayed correctly

Problem #2 : Approving comment

Steps to reproduce:

Try approving an unapproved comment when editing a post.

-> The row's background color changed, and the "Approve" link changed to "Unapprove", but only for a slight moment. Then it switched back to look like it's still unapproved.

Same thing happen when you unapprove a previously approved comment.

Problem #3: Trashing a comment

Trash a comment. The "moved to the trash" message appears, but the commenter's gravatar appears twice.

Also, it's not a comment river (trashing or spamming comments doesn't seem to refill the list)

Attachments (14)

garyc40-15876.patch (4.3 KB) - added by garyc40 3 years ago.
there's a patch for that
duplicate.png (25.7 KB) - added by garyc40 3 years ago.
garyc40-15876-rev2.patch (6.5 KB) - added by garyc40 3 years ago.
fixed css styling & "show more comments" behavior
before.png (88.5 KB) - added by garyc40 3 years ago.
how it looks BEFORE the patch is applied
after.png (84.4 KB) - added by garyc40 3 years ago.
how it looks AFTER the patch is applied
garyc40-15876-rev3.patch (6.3 KB) - added by garyc40 3 years ago.
removed "console.log"
garyc40-15876-original-style.patch (6.4 KB) - added by garyc40 3 years ago.
patch that solves all of the problems, with original style from 3.0.3
garyc40-15876-original-style-revised.patch (5.7 KB) - added by garyc40 3 years ago.
added back original author column width, and removed duplicate error styles
garyc40-15876-rev4.patch (7.5 KB) - added by garyc40 3 years ago.
removed table headers completely as per scribu's suggestion
garyc40-15876-rev5.patch (7.5 KB) - added by garyc40 3 years ago.
no array_push()
garyc40-15876-rev6.patch (7.5 KB) - added by garyc40 3 years ago.
set checkbox property after get_list_table()
garyc40-15876-rev7.patch (7.4 KB) - added by garyc40 3 years ago.
removed $wp_list_table->checkbox
garyc40-15876-rev8.patch (1.0 KB) - added by garyc40 3 years ago.
fixed colspan problem with reply form
reply problem.png (36.8 KB) - added by garyc40 3 years ago.
Post comment meta box, replying to a comment, Chrome

Download all attachments as: .zip

Change History (45)

comment:1 JohnONolan3 years ago

  • Milestone changed from Awaiting Review to 3.1
  • Priority changed from normal to highest omg bbq

Confirmed - this is completely borked. Additional minor issue of missing padding is also present - http://cl.ly/3gnO

comment:2 duck_3 years ago

All three are regressions (not sure about the comment river note though). Also, in 3.0.3 we didn't show the gravatar and there was also a heading row with titles Author and Comment above the comments.

comment:3 duck_3 years ago

With some quick investigation firebug is showing me that the wpList.ajaxDim function is being triggered twice and so two AJAX requests are being made, could possibly be the cause to the second two problems here?

garyc403 years ago

there's a patch for that

comment:4 garyc403 years ago

The patch fixed 3 problems listed above, however I discovered 2 more problems:

  1. Duplicate comment error message

Try replying to a comment with a test message. Then replying to any comment with the same message. There should be an error message: "Duplicate comment detected; it looks as though you’ve already said that!". However, it doesn't look like it's styled correctly (see screenshot).

  1. If I have more than 10 comments, and click "Show more comments", the exact same 10 comments that have already been listed appear.

I'm working on a fix for both.

garyc403 years ago

comment:5 garyc403 years ago

Patch introduced a bug: comment list table's column headers are visible. Fixing.

garyc403 years ago

fixed css styling & "show more comments" behavior

garyc403 years ago

how it looks BEFORE the patch is applied

garyc403 years ago

how it looks AFTER the patch is applied

comment:6 garyc403 years ago

I modified the css a bit so that the "Comments" meta box looks consistent with "Recent comments" on the Dashboard, as well as with the Comments edit page.

Needs UI feedback.

comment:7 garyc403 years ago

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

garyc403 years ago

removed "console.log"

comment:8 duck_3 years ago

I notice the rev3 patch doing some changes in WP_Post_Comments_List_Table::get_column_info and WP_Post_Comments_List_Table::print_column_headers, but I still don't see the column headers in the screenshot.

comment:9 follow-up: garyc403 years ago

I added the headers but I hide the span inside <th>. This is a trick to set column width while the headers are being hidden (otherwise the widths will be 50% 50%). Don't know if there's any cleaner trick though.

comment:10 in reply to: ↑ 9 duck_3 years ago

Replying to garyc40:

I added the headers but I hide the span inside <th>. This is a trick to set column width while the headers are being hidden (otherwise the widths will be 50% 50%). Don't know if there's any cleaner trick though.

I meant that the column headers actually showed up in 3.0.3 (link to the screenshot you posted in IRC).

comment:11 garyc403 years ago

Before I patched this, the column headers were already hidden (or rather, not being output at all), see http://core.trac.wordpress.org/attachment/ticket/15876/before.png .

So should it display the header? Recent Comments on Dashboard doesn't have the headers though.

garyc403 years ago

patch that solves all of the problems, with original style from 3.0.3

comment:12 garyc403 years ago

Added another patch, basically does the same thing as rev3. However, the style of the comment meta box is the same as in 3.0.3 (no gravatar, column headers visible).

How it looks in 3.0.3: http://awesomescreenshot.com/0894v1t2c

garyc403 years ago

added back original author column width, and removed duplicate error styles

comment:13 scribu3 years ago

So which are the two final patches we should look at?

comment:14 garyc403 years ago

Two final patches:

New style: garyc40-15876-rev3.patch

Original style: garyc40-15876-original-style-revised.patch

Sorry for the confusion.

comment:15 scribu3 years ago

Both patches seem to work fine.

I obviously like the new style more.

comment:16 nacin3 years ago

The new styling is fine, CSS isn't the issue.

But I see a lot of code changes here. We need to be careful.

comment:17 scribu3 years ago

If you don't show the column headers at all, why do you overwrite print_column_headers() in WP_Post_Comments_List_Table?

comment:18 garyc403 years ago

It's because without column headers (<th> elements specifically), I can't find any way to adjust the width of the columns. Applying width:30% for example to the td.column-xxx didn't work. Even if I output the headers, then display:none, widths are not adjusted as well. They just automatically reset to 50% / 50%. That's why I have to introduce <span> inside <th> .

Anyone knows if there's a way to adjust columns' width without <th>? let me know and I'll update the patch :)

Another way is to use <colgroup>, but we have to print it before <thead>.

garyc403 years ago

removed table headers completely as per scribu's suggestion

comment:19 garyc403 years ago

Latest patch removed table headers completely by overriding WP_List_Table::display(). Obviously a cleaner solution than the previous patch.

garyc403 years ago

no array_push()

comment:20 scribu3 years ago

$wp_list_table->checkbox = ( isset($_POST['checkbox']) && true == $_POST['checkbox'] ) ? 1 : 0; 
$wp_list_table = get_list_table('WP_Comments_List_Table');

Should be reversed, no?

garyc403 years ago

set checkbox property after get_list_table()

comment:21 follow-up: westi3 years ago

Do we need to enumerate the list table class there - I thought the class name was in the ajax request anyway?

comment:22 scribu3 years ago

set checkbox property after get_list_table()

Actually, I don't think we need the checkbox property at all. That is a relic from before WP_Post_Comments_List_Table.

comment:23 in reply to: ↑ 21 ; follow-up: garyc403 years ago

Replying to westi:

Do we need to enumerate the list table class there - I thought the class name was in the ajax request anyway?

I didn't see any class name in the ajax request. I just basically ported the table in post_comment_meta_box() into WP_Post_Comments_List_Table::display(), and keep the classes and id intact, in case it might break some javascript.

Replying to scribu:

Actually, I don't think we need the checkbox property at all. That is a relic from before WP_Post_Comments_List_Table.

Sure, I'll remove it in the final patch after the current patch has been scrutinized.

comment:24 in reply to: ↑ 23 westi3 years ago

Replying to garyc40:

Replying to westi:

Do we need to enumerate the list table class there - I thought the class name was in the ajax request anyway?

I didn't see any class name in the ajax request. I just basically ported the table in post_comment_meta_box() into WP_Post_Comments_List_Table::display(), and keep the classes and id intact, in case it might break some javascript.

I guess we aren't using the generic list table js then here.

garyc403 years ago

removed $wp_list_table->checkbox

comment:25 scribu3 years ago

  • Keywords commit added; needs-testing ui-feedback removed

Per IRC bug scrub today, this should be commited, but it seems nobody got around to it yet.

comment:26 ryan3 years ago

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

(In [17113]) Fix comment manipulation on edit post screen. Props garyc40. fixes #15876

comment:27 follow-up: garyc403 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

When replying to comments, the Reply form is still wrapped in one column (colspan = 0) instead of 2.

Last edited 3 years ago by garyc40 (previous) (diff)

garyc403 years ago

fixed colspan problem with reply form

comment:28 in reply to: ↑ 27 scribu3 years ago

Replying to garyc40:

When replying to comments, the Reply form is still wrapped in one column (colspan = 0) instead of 2.

Can't reproduce the problem. Tried on the dashboard box, the post metabox and the list table, with Firefox and Chrome.

garyc403 years ago

Post comment meta box, replying to a comment, Chrome

comment:29 garyc403 years ago

Screenshot attached.

Nothing went wrong with dashboard box or the comment list table, only the post metabox.

The main reason is because the table headers are removed, so when the reply form is opened, line 341 in edit-comments.dev.js sets colspan to 0.

comment:30 scribu3 years ago

Was able to reproduce eventually. Patch is good to go.

comment:31 ryan3 years ago

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

(In [17124]) Fix comment reply colspan. Props garyc40. fixes #15876

Note: See TracTickets for help on using tickets.