WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 3 months ago

Last modified 3 months ago

#46260 closed defect (bug) (fixed)

5.1 Backwards Compatibility Issue: comment-reply.js

Reported by: iCaleb Owned by: peterwilsoncc
Milestone: 5.1 Priority: normal
Severity: normal Version: 5.1
Component: Comments Keywords: has-patch commit fixed-major
Focuses: Cc:

Description (last modified by SergeyBiryukov)

tl;dr -> The changes made to comment-reply.js for the 5.1 release cause BC issues when comments are loaded in after the initial page load (via ajax for example). The reply link does not work as expected anymore, and can break commenting completely depending on the implementation.

Issue Description

In WP 5.0 and before, the comment reply links were hardcoded to the onclick event, seen here: https://github.com/WordPress/WordPress/commit/f617a2d6d932e658449cef1b7864fb77080ecd09#diff-3db08a6b98155b349c93a220bc903dceL1670

But in 5.1+ (since #31590), comment-reply.js initializes by binding these links to the event handler. The problem is though, these links won't exist yet when the script is first loaded since they come in later via ajax.

Replicating / Solutions

I first replicated on a site using a custom ajax implementation, but you can get a similar result using a plugin like https://wordpress.org/plugins/ajax-comment-loading/. Test on 5.0 versus 5.1 and you can see the difference.

To maintain backwards compatibility, the comment reply script should take into consideration that the links won't always be on the page right away when the script is first loaded.

As a fix for individual sites/plugins though (in case somebody stumbles across this ticket), you can call window.addComment.init(); at the end of your ajax callback to ensure the links get bound to the needed event handlers.

Attachments (4)

46260.diff (4.5 KB) - added by pento 3 months ago.
46260.2.diff (1.4 KB) - added by pento 3 months ago.
46260.3.diff (1.6 KB) - added by pento 3 months ago.
46260.4.diff (2.1 KB) - added by peterwilsoncc 3 months ago.

Download all attachments as: .zip

Change History (17)

#1 @desrosj
3 months ago

  • Milestone changed from Awaiting Review to 5.1

Adding to 5.1 to be investigated properly.

#2 @peterwilsoncc
3 months ago

Researched but untested :)

I think using MutationObserver with a fallback to DOMNodeInserted for checking for inserted content and re-initializing will fix this back to IE 9.

@pento
3 months ago

#3 follow-up: @pento
3 months ago

  • Keywords has-patch 2nd-opinion added

46260.diff fixes this by event bubbling, rather than MutationObserver.

I have no strong preference either way, this is just the approach I took first.

Looking at the code and discussion on #31590, it appears intentional that Ajax comment loaders will need to call window.addComment.init(), but I couldn't find a devnote to match. @peterwilsoncc, I don't suppose you recall any additional context on this decision?

@pento
3 months ago

#4 in reply to: ↑ 3 @peterwilsoncc
3 months ago

  • Owner set to peterwilsoncc
  • Status changed from new to accepted

I was in the same two minds for bubbling vs mutation observing. I'll test out the performance of each.

Replying to pento:

@peterwilsoncc, I don't suppose you recall any additional context on this decision?

I don't, I am afraid. I suspect the context was that four years ago I was more willing to break back-compat for many, many sites than I am now and I missed it when I came back to the ticket recently.

@pento
3 months ago

#5 @pento
3 months ago

46260.3.diff is the alternative approach, using MutationObserver. I haven't implemented a DOMNodeInserted fallback, I'm fine with IE<11 falling back to the replytocom comment form.

#6 @peterwilsoncc
3 months ago

46260.4.diff is almost identical to 46260.3.diff:

  • Since tag on observeChanges()
  • Moved MutationObserver declaration to avoid re-defintion
  • Trailing comma removed on observerOptions
  • Minor golfing change to init().

I've tested it with the plugin highlighted but needed to debug the plugin first as it uses some long-lost jQuery attribute selectors.

I considered including MutationObserver in the cuts the mustard check but ultimately left it out for a little wider browser support for sites loading comments in the traditional WordPress fashion.

#7 @peterwilsoncc
3 months ago

  • Keywords commit added; 2nd-opinion removed

#8 @peterwilsoncc
3 months ago

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

In 44748:

Comments: Fix backward compatibility regressions in comment reply JavaScript.

Adds a MutationObserver to comment-reply.js to allow for lazy-loaded comments to continue working without the need to re-initialize the comment form.

Props Pento.
Fixes #46260.

#9 @peterwilsoncc
3 months ago

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

Re-opening for back-port consideration.

#10 @SergeyBiryukov
3 months ago

  • Description modified (diff)

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


3 months ago

#12 @pento
3 months ago

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

In 44751:

Comments: Fix backward compatibility regressions in comment reply JavaScript.

Adds a MutationObserver to comment-reply.js to allow for lazy-loaded comments to continue working without the need to re-initialize the comment form.

Merges [44748] to the 5.1 branch.

Fixes #46260.

#13 @MegaZ
3 months ago

This fix takes care of the problem in Chrome, but Safari still does not work. Replying to a comment just scrolls down the page to the comments section.

Note: See TracTickets for help on using tickets.