WordPress.org

Make WordPress Core

Opened 5 months ago

Last modified 4 months ago

#47306 reopened enhancement

comment reply event listener: need ability to run custom js function

Reported by: jnorell Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 5.1
Component: Comments Keywords:
Focuses: Cc:
PR Number:

Description

I wrote the comment-tweaks plugin, which adds the tinymce editor to the comments field. When threaded comments are enabled, clicking on a Reply link moves #comment within the dom, and that breaks tinymce - I need to be able to run a bit of js when a Reply link is clicked to remove the editor, then move the element, then add the editor back.

Prior to 5.1, the comment reply link had an onclick attribute calling moveForm(), and I simply overwrote the single onclick handler with a custom one. This is/was a bit of a hack, but worked fine (as long as no other plugin was hoping to also take over onclick).

Recent changes (#46260) to comment replying changed how event listeners are handled for the comment Reply and Cancel reply buttons, which now uses a MutationObserver to (re)add click/touchstart listeners every time the document body changes. I can no longer overwrite the onclick handler, as it will be re-added in subsequent document changes.

I could probably add my own MutationObserver, handle the race of which fires first, and continue to overwrite the core click/touchstart event handlers, but this is just another hack and I would rather fix it correctly, where other plugins could utilize event handlers on those links as well.

Note that I need more than just the ability to add my own event listeners, as they fire in the order they were added, and I need to remove the editor prior to moving #comment (addComment.clickEvent() firing), and add it again afterwards.

A javascript "action hook" that fires in clickEvent() ahead of moveForm() and another which fires afterwards may be a viable solution. (And another pair of "action hooks" in cancelEvent().) Probably could make the entire click/cancel event handlers just call the action hook and add the current clickEvent/cancelEvent code in functions called via those same hooks, so they could be overridden if needed? (I don't need to overwrite them myself if I can order a call before and after them, but perhaps other plugins would.)

Attachments (2)

addComment_more_references.patch (398 bytes) - added by jnorell 5 months ago.
addComment: expose references to functions and config
addComment_more_references-2.patch (1.2 KB) - added by jnorell 5 months ago.

Download all attachments as: .zip

Change History (9)

#1 follow-up: @peterwilsoncc
5 months ago

Hi @jnorell and welcome to trac!

Am I correct to understand you had a custom implementation of addComment.moveForm() to replace WP Core's version?

If so, you still replace it by settings your plugin's JavaScript to rely on Core's comment-reply when enqueueing it override it by creating a function addComment.moveForm(). Jetpack takes a similar approach for Jetpack comments.

See https://gist.github.com/peterwilsoncc/e463ba477e81e8c946478d3b3c2154b4

--

If I am misunderstanding your needs, please let me know and I will keep this ticket open until I hear back from you.

#2 in reply to: ↑ 1 ; follow-up: @jnorell
5 months ago

Replying to peterwilsoncc:

Am I correct to understand you had a custom implementation of addComment.moveForm() to replace WP Core's version?

No, that would have been a better design :), I simply created a new click handler, overwriting the one which previously called moveForm(): https://github.com/jnorell/comment-tweaks/blob/master/public/js/comment-tweaks-public.js#L102

If so, you still replace it by settings your plugin's JavaScript to rely on Core's comment-reply when enqueueing it override it by creating a function addComment.moveForm().

That's a better approach, I'll rework things along those lines, thanks!

Jetpack takes a similar approach for Jetpack comments.

See https://gist.github.com/peterwilsoncc/e463ba477e81e8c946478d3b3c2154b4

Thanks for the pointers.


If I am misunderstanding your needs, please let me know and I will keep this ticket open until I hear back from you.

I'll give that a go and reply here with the outcome.

Thanks again!

#3 @peterwilsoncc
5 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

Thanks,

For now I will close of the ticket and mark it invalid, which is trac's unfriendly term indicating no action needs to be taken.

If modifying your plugin to override moveForm() doesn't work, please reopen it when you provide an update.

#4 in reply to: ↑ 2 @jnorell
5 months ago

Replying to jnorell:

Replying to peterwilsoncc:

If I am misunderstanding your needs, please let me know and I will keep this ticket open until I hear back from you.

I'll give that a go and reply here with the outcome.

I did use this approach (overwriting moveForm()) which works for clicking on a Reply link, but a problem still remains in handling the cancel reply link - addComment.cancelEvent() moves #comment as well, so ideally it would be nice to be able to overwrite it as well, which is not currently possible.

As a workaround, I rename the wp-temp-form-div element so cancelEvent exits without doing anything, and I attach an inline onclick handler for the cancel link to do the real work. That works so that comment-tweaks can add the editor with 5.1+ again.

I'll submit a simple patch (if I can to a closed issue) to allow overwriting clickEvent and cancelEvent functions, maybe consider it.

Thanks much!

@jnorell
5 months ago

addComment: expose references to functions and config

#5 @peterwilsoncc
5 months ago

  • Milestone set to Future Release
  • Resolution invalid deleted
  • Status changed from closed to reopened

Thanks for the patch, I've reopened the ticket.

To allow for the handler to be overridden, the cancel event will need to call window.addComment.cancelEvent (similar to how moveForm is called in the click handler.

Otherwise the function within scope will be called even if a plugin replaces it.

#6 @jnorell
5 months ago

ok, patch is amended

#7 @jnorell
4 months ago

@peterwilsoncc, when you have a moment can you please check the updated patch and see if the changes are what you had in mind?

thanks!

Note: See TracTickets for help on using tickets.