Make WordPress Core

Opened 2 years ago

Closed 21 months ago

Last modified 21 months ago

#54990 closed defect (bug) (fixed)

Unload handler for inline replies to comments displays "Are you sure.." alert when nothing has been entered

Reported by: davidbaumwald's profile davidbaumwald Owned by: davidbaumwald's profile davidbaumwald
Milestone: 6.1 Priority: normal
Severity: normal Version: 4.6
Component: Comments Keywords: good-first-bug has-patch
Focuses: Cc:

Description

While investigating #54621, there's a similar(perhaps related) issue happening in wp-admin/edit-comments.php.

To reproduce:

  1. Click "Reply" on an existing comment.
  2. Don't enter anything in the "Reply to Comment" textarea.
  3. Click on "Reply" or "Quick Edit" on any other comment.
  4. Observe the "Are you sure..." alert.

Resetting the originalContent property to an empty string when the action is not edit seems to fix the issue.

Attachments (5)

wp-core-edit-comments-empty-reply-are-you-sure.png (64.4 KB) - added by davidbaumwald 2 years ago.
Screenshot of the alert
edit-comments.min.js (20.9 KB) - added by cu121 2 years ago.
Fix provided for ticket #54990
54990.diff (416 bytes) - added by azouamauriac 2 years ago.
set originalContent to empty when the action is reply
54990.2.diff (498 bytes) - added by cu121 2 years ago.
Fixed #54990, This patch fixed both issues consists with QuickEdit and Reply comment if the reply content is empty.
54990-2.diff (562 bytes) - added by cu121 2 years ago.
This fix is final, it involves solving the issue for quick edit and no reply content. (Quick Edit) If the original content of the quick edit message is not matched with the updated message, then it will show an alert.

Download all attachments as: .zip

Change History (32)

@cu121
2 years ago

Fix provided for ticket #54990

#2 @mkox
2 years ago

I'm not sure if this a fix indeed. It should be fixed in the plain source code an not in the minified js, I guess.

I think a part of the problem is that the replycomment area is used for the edit of comments as well - at least the id.

I will have a look into it.

#3 @azouamauriac
2 years ago

hi @davidbaumwald when i'm trying to upload the fix i get the following error :
svn: E155010: The node 'src/wp-admin/js/edit-comments.js' was not found.

could you please explain me what happen?

[but when i made changes in src/js/_enqueues/admin/edit-comments.js svn show and handle those changes.] thanks.

Last edited 2 years ago by azouamauriac (previous) (diff)

#4 follow-up: @mkox
2 years ago

Hi @azouamauriac,

'/src/wp-admin/js' is ignored by .gitignore file. It will be built from '/src/js/_enqueues/admin/edit-comments.js' during build process instead.

@azouamauriac
2 years ago

set originalContent to empty when the action is reply

#5 in reply to: ↑ 4 @azouamauriac
2 years ago

Replying to mkox:

Hi @azouamauriac,

'/src/wp-admin/js' is ignored by .gitignore file. It will be built from '/src/js/_enqueues/admin/edit-comments.js' during build process instead.

@mkox thank you i've submitted the patch

#6 @mkox
2 years ago

  • Keywords has-patch reporter-feedback added; needs-patch removed

The patch seems to solve the explained problem to me.

Anyway the behavior could be somehow improved for consistent behavior. At the moment even with this fix the check only works, if reply or quick edit are selected on another comment. If a user chooses to (slow) edit another comment, there is no check and the link is just opened. Maybe worth a new ticket.

#7 follow-up: @cu121
2 years ago

@mkox can I upload the patch for this still? I am new here I am not aware of the process for uploading the patch. Could you please inform me, as i can see the patch is uploaded by another user

#8 in reply to: ↑ 7 @azouamauriac
2 years ago

Replying to cu121:

@mkox can I upload the patch for this still? I am new here I am not aware of the process for uploading the patch. Could you please inform me, as i can see the patch is uploaded by another user

hi i'm new here too, to submit a patch read thishttps://make.wordpress.org/core/handbook/tutorials/trac/submitting-a-patch/.

feel free to submit a new patch.

regards

@cu121
2 years ago

Fixed #54990, This patch fixed both issues consists with QuickEdit and Reply comment if the reply content is empty.

#9 @hasanuzzamanshamim
2 years ago

I tested this issue with the provided patch; this will logically work for this case only:

  1. click on "reply"
  2. click on "quick edit".

it works perfectly cause this time the reply content is empty.
But if somehow I trigger the "reply" again without changing any value this will throw the alert. cause the value we checked is not empty now.

$( '#replycontent', editRow ).val() // not empty when quick edit

I think this hardcoded empty string check will not solve this issue for edit reply.


        discardCommentChanges: function() {
                var editRow = $( '#replyrow' );
                
                if  ( '' === $( '#replycontent', editRow ).val() ) {
                        return true;
                }

                return window.confirm( __( 'Are you sure you want to do this?\nThe comment changes you made will be lost.' ) );
        }

#10 follow-up: @cu121
2 years ago

@hasanuzzamanshamim edit comment is synchronous action, based on the issue explained this fix was provided specifically for asynchronous actions like reply, quick edit, etc. I guess creating a separate ticket for that will be awesome as well :)

#12 in reply to: ↑ 10 ; follow-up: @hasanuzzamanshamim
2 years ago

Replying to cu121:

@hasanuzzamanshamim edit comment is synchronous action, based on the issue explained this fix was provided specifically for asynchronous actions like reply, quick edit, etc. I guess creating a separate ticket for that will be awesome as well :)

I think you missed something, see this test case you will get the issue.
https://drive.google.com/file/d/1EHXYs5REL8llvs4hGktzi1w_ittIS8v3/view?usp=sharing

#13 in reply to: ↑ 12 @cu121
2 years ago

@hasanuzzamanshamim since the field content is not empty, it will definitely show a "Are you sure?" alert when clicked on another comment 'Quick Edit'. Because you are editing the existing comment, which was not empty. But in the case of a reply, the field was empty, so on another comment, if the reply is clicked then it was showing 'Are you sure?' alert was a bug because the reply content was empty. Based on the issue reported, this was the specific fix provided.

Additionally, the id of the reply field and edit field are the same.

Last edited 2 years ago by cu121 (previous) (diff)

@cu121
2 years ago

This fix is final, it involves solving the issue for quick edit and no reply content. (Quick Edit) If the original content of the quick edit message is not matched with the updated message, then it will show an alert.

#14 @cu121
2 years ago

@hasanuzzamanshamim thank you for pointing out the issue with your specific test case :).

This ticket was mentioned in PR #2299 on WordPress/wordpress-develop by rafiahmedd.


2 years ago
#15

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

I remove t.originalContent in line 970 because it's overwriting the value so I think we don't need this.

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


2 years ago

#17 @hellofromTonya
2 years ago

  • Milestone changed from 6.0 to 6.1

6.0 RC1 is tomorrow and today is the last beta before RC starts. The patch needs testing and review. As this bug was not introduced in the 6.0 cycle, moving the ticket to 6.1.

This ticket was mentioned in PR #2762 on WordPress/wordpress-develop by cuchakma.


2 years ago
#18

---Code Explanation---
Added fix of solving the issue for quick edit and no reply content. (Quick Edit) If the original content of the quick edit message is not matched with the updated message, then it will show an alert. Code Modified On-Line Number 1228

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

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


22 months ago

#20 @desrosj
22 months ago

  • Keywords needs-testing added; reporter-feedback removed

#22 @davidbaumwald
21 months ago

  • Owner set to davidbaumwald
  • Status changed from new to reviewing

#23 @JeffPaul
21 months ago

  • Keywords needs-testing removed

@faisal03 thanks for the testing, @davidbaumwald I'll leave this to you if you want to try and get in before 6.1 Beta 1 (or leave for during the beta cycles).

dream-encode commented on PR #2299:


21 months ago
#24

Thanks for the PR! This was merged into core with https://core.trac.wordpress.org/changeset/54334.

dream-encode commented on PR #2762:


21 months ago
#25

Thanks for the PR! This was merged into core with https://core.trac.wordpress.org/changeset/54334.

#26 @davidbaumwald
21 months ago

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

Not sure why Trac didn't capture the commit, but this was resolved in r54334.

cuchakma commented on PR #2762:


21 months ago
#27

@dream-encode Thank you for accepting my PR.

Note: See TracTickets for help on using tickets.