Make WordPress Core

Opened 8 years ago

Closed 4 years ago

Last modified 3 years ago

#38009 closed defect (bug) (fixed)

#reply-title.comment-reply-title not updating when replying to an individual

Reported by: iguel's profile iguel Owned by: isabel_brison's profile isabel_brison
Milestone: 5.5.1 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch commit fixed-major dev-reviewed
Focuses: Cc:

Description

From what I can tell, in the $defaults array 'title_reply_to' on Line 2227 of wp-includes/comment-template.php is supposed to be called when I click "Reply" to another commenter, but it is not doing so in every theme I have tried, however, it does work in the customizer view. Here's a screenshot of what I found.

Attachments (1)

comment-bug.png (216.1 KB) - added by iguel 8 years ago.

Download all attachments as: .zip

Change History (41)

@iguel
8 years ago

#1 @stopdesign
6 years ago

Adding to this to confirm that I also see this behavior of title_reply_to not changing the title. I have never seen the title change to 'Leave a Reply to %s' as documented when replying to another comment. Even though the form moves and the 'cancel reply' link is made visible -- signs that comment-reply.js seems to have been properly enqueued. This is true not only on my own custom theme that I’m working on now, but on every WordPress site I have found so far, including @matt's own site:

https://ma.tt/2018/05/wordpress-at-15/#comments

and others with the exact same reproducible bug:

https://www.positivityblog.com/22-inspirational-quotes-on-fear/#comments

https://rafaelnadalfans.com/2018/05/31/rafael-nadal-pays-tribute-to-zinedine-zidane/#comments

http://hurrahforgin.com/2017/12/14/adulthood-sucks/#comments

Observed on WordPress v 4.9.6

#2 @obenland
6 years ago

  • Version 4.6.1 deleted

Hi @iguel, welcome to WordPress Trac!

What it comes down to here is context. The comment-reply script moves the comment form right underneath the comment you want to reply to. With that context, I assume, comment-reply was not built to update the respond title.

If that script is not present however (or in case of the Customizer the reply link is followed), the form remains at the bottom of the page, and to create context the respond title gets amended with the original commenter's name.

#3 @stopdesign
6 years ago

Hi @obenland - Thanks for the reply.

I understand the context and how the comment form moves or not in case the script doesn't exist. What I am pointing out (and I think, @iguel too) is that according to the codex for 'comment_form()' the title_reply_to arg exists for the purpose of contextually changing the title to Leave a Reply to %s.

Even if title_reply_to is currently ignored when the script is present, we’re pointing out that title_reply_to should be respected in addition to moving the form, especially when the arg is explicitly included in comment_form() args. If the title is important enough to modify when the form can’t move because of a missing script, it's important enough to modify when the form moves too. The context of where the form appears can still be complemented by a title that responds to the shifted context.

The additional context a modified title helps provide is useful to the user whether the comment form moves or not. It looks broken to me that the title does not update when the context of the reply and form switches, when I know an arg exists for declaring a modified title for replying to another comment.

Thanks for considering.

#4 @chanthaboune
4 years ago

  • Milestone changed from Awaiting Review to 5.4

#5 @audrasjb
4 years ago

  • Milestone changed from 5.4 to Future Release

Hi,

With 5.4 Beta 3 approaching and the Beta period reserved for bugs introduced during the cycle, this is being moved to Future Release. If any maintainer or committer feels this should be included or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

This ticket was mentioned in PR #167 on WordPress/wordpress-develop by tellthemachines.


4 years ago
#6

<!--
Hi there! Thanks for contributing to WordPress!

Pull Requests in this GitHub repository must be linked to a ticket in the WordPress Core Trac instance (https://core.trac.wordpress.org), and are only used for code review. No pull requests will be merged on GitHub.

See the WordPress Handbook page on using PRs for Code Review more information: https://make.wordpress.org/core/handbook/contribute/git/github-pull-requests-for-code-review/

If this is your first time contributing, you may also find reviewing these guides first to be helpful:

-->

<!-- Insert a description of your changes here -->

When replying to an existing comment, the comment form is moved to below the existing comment with JS, but the form heading was not being updated. This fixes the issue by introducing a new data-attribute to the reply link with the correct heading string, and applying that string to the heading when the form is moved.

Trac ticket: <!-- insert a link to the WordPress Trac ticket here -->

This addresses https://core.trac.wordpress.org/ticket/38009

---
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See [GitHub Pull Requests for Code Review](https://make.wordpress.org/core/handbook/contribute/git/github-pull-requests-for-code-review/) in the Core Handbook for more details.

#7 @isabel_brison
4 years ago

  • Keywords has-patch added
  • Version set to trunk

This ticket was mentioned in PR #168 on WordPress/wordpress-develop by tellthemachines.


4 years ago
#8

<!-- Insert a description of your changes here -->

Removes most of the boilerplate from the PR description field, so that the synced Trac comment won't be too noisy.
(I have manually deleted the boilerplate from this PR, so see https://core.trac.wordpress.org/ticket/38009 for a sample of what it looks like currently)

Trac ticket: <!-- insert a link to the WordPress Trac ticket here -->
Fixes https://core.trac.wordpress.org/ticket/49508

---
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See [GitHub Pull Requests for Code Review](https://make.wordpress.org/core/handbook/contribute/git/github-pull-requests-for-code-review/) in the Core Handbook for more details.

#9 @isabel_brison
4 years ago

Ugh, the second PR ended up linked to this ticket by mistake, sorry :P

#10 @audrasjb
4 years ago

  • Version trunk deleted

Removing trunk as the bug is not specific to the current development branch :)

peterwilsoncc commented on PR #167:


4 years ago
#12

I spent some time yesterday considering how to avoid adding the new element to the header, without success, I'll continue looking at that today.

Otherwise I think this is good to go.

peterwilsoncc commented on PR #167:


4 years ago
#13

I spent some time yesterday considering how to avoid adding the new element to the header, without success, I'll continue looking at that today.

Otherwise I think this is good to go.

peterwilsoncc commented on PR #167:


4 years ago
#14

Note re PHP linting test failure: checking locally comment-template.php contains only warnings so I am pretty sure the tests will only fail on PRs but pass on master. Probably.

peterwilsoncc commented on PR #167:


4 years ago
#15

I've been working on a couple of approaches to avoid the new element in the comment template, either:

  • document.getElementById( 'reply-title' ).firstNode to get the textNode (which can then be replaced. This may need some extra work to get the first textNode to account for sites that have modified the form.
  • Cloning the reply title and removing anything that isn't a text nodes document.getElementById( 'reply-title' ).querySelectorAll( '*' )

There are options available.

#16 @noisysocks
4 years ago

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

#17 @noisysocks
4 years ago

  • Keywords commit added

PR #167 is approved.

#18 @noisysocks
4 years ago

  • Milestone changed from Future Release to 5.5
  • Resolution set to fixed
  • Status changed from assigned to closed

This was fixed by r47506. Not sure why Trac didn't pick up the ticket mention :)

#19 follow-up: @mailnew2ster
4 years ago

This change breaks my theme. See:
https://i.imgur.com/3eWwbYw.png

#20 @mailnew2ster
4 years ago

The solution would be to change:

var initialHeadingText = ( 'undefined' !== typeof replyElement ) ? replyElement.firstChild.textContent : '';

to:

var initialHeadingText = ( null !== replyElement ) ? replyElement.firstChild.textContent : '';

or, to be consistent with other similar usage:

var initialHeadingText = replyElement ? replyElement.firstChild.textContent : '';

here:

https://github.com/WordPress/WordPress/commit/b4e1e8f0d144df6337f1b8978c8f3f79023aa88f#diff-705579fc6109ea62afe07661843ba138R411

#22 @noisysocks
4 years ago

  • Milestone changed from 5.5 to Awaiting Review
  • Resolution fixed deleted
  • Status changed from closed to reopened

#23 in reply to: ↑ 19 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.5.1

Replying to mailnew2ster:

This change breaks my theme. See:
https://i.imgur.com/3eWwbYw.png

Thanks for the report and the PR!

Moving to 5.5.1, as this appears to be a regression in [47506].

In the future though, instead of reopening the ticket closed on a completed milestone, please create a new one for any follow-up issues. Thanks again!

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


4 years ago

#25 @davidbaumwald
4 years ago

  • Keywords needs-testing added; commit removed

Per the bug scrub today, removing commit and adding needs-testing for the new PR.

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


4 years ago

#27 @johnbillion
4 years ago

  • Milestone changed from 5.5.1 to 5.5.2

#28 @sarahricker
4 years ago

  • Keywords needs-testing removed

@johnbillion @SergeyBiryukov I wasn't able to reproduce the bug on 5.5, but the patch looks good to me. Tested with a few core themes. No regressions, no console errors.

#29 @mailnew2ster
4 years ago

Well, I use a custom theme which doesn't have the relevant element. To reproduce, you can edit any theme to remove that element as well.

#30 @SergeyBiryukov
4 years ago

  • Milestone changed from 5.5.2 to 5.5.1

#31 @SergeyBiryukov
4 years ago

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

In 48876:

Comments: Correct the check for reply element existence in comment-reply.js.

document.getElementById() returns null if no matching element was found, so the previous comparison didn't work as expected.

Follow-up to [47506].

Props mailnew2ster, sarahricker.
Fixes #38009.

#32 @SergeyBiryukov
4 years ago

In 48877:

Comments: Correct the check for reply element existence in comment-reply.js.

document.getElementById() returns null if no matching element was found, so the previous comparison didn't work as expected.

Follow-up to [47506].

Props mailnew2ster, sarahricker.
Merges [48876] to the 5.5 branch.
Fixes #38009.

#33 @johannadevos
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I tried testing [48876] in the 5.5.1 RC.
In comments.php in my theme (Twenty Twenty), I replaced:
title_reply_before' => '<h2 id="reply-title" class="comment-reply-title">', with:
'title_reply_before' => '<h2>',.

In other words, I removed the "reply-title" id attribute, so that in comment-reply.js the commentReplyTitleId would be undefined.

To test, I opened the console and replied to a comment on a post. That leads to the following error:
Uncaught TypeError: Cannot read property 'nodeType' of null on line 340 of comment-reply.js (concerning replyHeadingTextNode.nodeType).

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#34 @mailnew2ster
4 years ago

@johannadevos I pushed a fix for this.

#35 @SergeyBiryukov
4 years ago

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

In 48904:

Comments: Check if reply heading text node exists before accessing its property in comment-reply.js.

Follow-up to [47506], [48876].

Props johannadevos, mailnew2ster.
Fixes #38009.

#36 @SergeyBiryukov
4 years ago

  • Keywords commit fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for the 5.5 branch.

#37 @SergeyBiryukov
4 years ago

  • Keywords dev-feedback added

#38 @desrosj
4 years ago

  • Keywords dev-reviewed added; dev-feedback removed

👍🏼 for backport.

#39 @desrosj
4 years ago

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

In 48917:

Comments: Check if reply heading text node exists before accessing its property in comment-reply.js.

Follow-up to [47506], [48876].

Props johannadevos, mailnew2ster.
Merges [48904] to the 5.5 branch.
Fixes #38009.

Note: See TracTickets for help on using tickets.