WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 months ago

#38009 closed defect (bug) (fixed)

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

Reported by: iguel Owned by: isabel_brison
Milestone: 5.5 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch commit
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 4 years ago.

Download all attachments as: .zip

Change History (19)

@iguel
4 years ago

#1 @stopdesign
2 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
2 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
2 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
8 months ago

  • Milestone changed from Awaiting Review to 5.4

#5 @audrasjb
6 months 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.


5 months ago

<!--
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
5 months ago

  • Keywords has-patch added
  • Version set to trunk

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


5 months ago

<!-- 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
5 months ago

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

#10 @audrasjb
5 months ago

  • Version trunk deleted

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

#12 @prbot
5 months ago

peterwilsoncc commented on PR #167:

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.

#13 @prbot
5 months ago

peterwilsoncc commented on PR #167:

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.

#14 @prbot
5 months ago

peterwilsoncc commented on PR #167:

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.

#15 @prbot
5 months ago

peterwilsoncc commented on PR #167:

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
5 months ago

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

#17 @noisysocks
4 months ago

  • Keywords commit added

PR #167 is approved.

#18 @noisysocks
4 months 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 :)

Note: See TracTickets for help on using tickets.