Comment reply links can cause JS error on slow connections/large pages

[peterwilsoncc]

addComment.moveForm can be called before it is defined, throwing a JavaScript error on the front end.

This is most likely to occur over slow connections or on enormous comment threads.

The attached patch defines a placeholder function in the HTML header if comment-reply is queued to ensure the front-end JavaScript continues to run.

[peterwilsoncc]

Few thoughts on my patch above:

• it works but is sub sub-optimal
• people have strong feelings about inline JavaScript, so lets not.

I've started reworking comment-reply.js to make it unobtrusive. Before going to far, I wanted to check I was on the correct path and fitting with core's preferred techniques, particularly around data attributes. My thoughts are:

Reply link markup becomes:

<a class="comment-reply-link" 
    href="http://example.com/sample-post/?replytocom=1299#respond" 
    data-comment-id="1299" 
    data-comment-reply-to-text="Reply to [commenter name]"
    aria-label="Reply to commenter" >

Comment-reply.js config data is created as:

var _wpCommentReplySettings = {
  "postId": 700,
  "respondId" : "respond",
  "addbelow" : "comment",
  "replyText" : __( 'Reply' ),
  "loginText" : __( 'Log in to Reply' )
};

Additionally, move into window.wp.commentReply name space. The init function would run once the DOM is ready but also be globalised to allow theme authors to re-run if they load the comment thread via JavaScript or similar.

[peterwilsoncc]

Related #22889 [33038] and [33039].

Working on this as a ​plugin, would like to get unobtrusive for 4.4.

[markjaquith]

Is there a certain amount of time in which we should just go through with it, if the code launches? That is, should we hold on to the comment ID of the latest click and if it was less than, say, half a second ago, proceed?

Should we just hide Reply links (via fast inline JS addition of a CSS rule) until they can actually be used?

[peterwilsoncc]

obtrusive with expiring method queue

[peterwilsoncc]

Replying to markjaquith:

Is there a certain amount of time in which we should just go through with it, if the code launches?

31590.1.diff builds on the saving mechanism in the original patch by adding an expiring method queue. 500ms seems as suitable response/expiry time as any. ¯\_(ツ)_/¯

31590.1.diff maintains the obtrusive JavaScript approach. As I mentioned above, it works but is sub-optimal.

-

I would prefer to go down the unobtrusive path as in the plugin. Technically the plugin works as is but it needs a tidy-up, moveForm needs to be refactored. Cutting the mustard check in the plugin makes it IE9+ only at present, it relies on the query-string fallback for older browsers.

Should it be determined that comment replies are an enhancement, then I'd take the approach of an empty span with data attributes to be replaced with a link once comment-reply runs.

[peterwilsoncc]

Committed version 1.0 of the ​plugin on the weekend.

Before generating a patch, there are a few extras I'd like to do on the Github ​issues page. The historyState depends on the outcome of #22889.

[peterwilsoncc]

unobtrusive comment-reply.js

[peterwilsoncc]

In attachment:31590.2.diff​:

• replace obtrusive JavaScript with unobtrusive JavaScript

• requires browser support for addEventListener and querySelector, other browsers fall back to ?replytocomlinks

• checked for compatibility with Jetpack comments (works)

• calls addComment.init() on script load. The init function is exposed so other scripts can call it should the need occur. This is for future proofing, I figure we'll see a bit more lazy loading of comments once the API is committed to core.

• element ids, field ids, and classes are in a config object at the head of the file. This will allow overrides to be set should it be considered useful down the track

I've checked it with:

• Jetpack comments (both browsers which do and don't cut the mustard)
• Disqus
• Intense Debate
• Facebook

Only Jetpack continues to use WPDB.

I have an alternative ​branch which updates the location bar with history.replaceState in Github should there be interest.

[peterwilsoncc]

I've refreshed against trunk in 31590.diff​.

@rachelbaker As discussed, my inclination is to close this as either wontfix or Yolo commit it.

[rachelbaker]

@peterwilsoncc I took a super quick look at [31590.diff] and just from the quick glance I can see it doesn't match the JSDocs standards. Can you take a look? Example: the @ticket lines are not valid, and likely should be @since blocks.

Also, are there any qunit tests that can be added here?

[peterwilsoncc]

In 31590.3.diff​:

• Even more accurate refresh against trunk
• Removed ticket number to replace with since 5.0.0 for committing
• Several docblock and code standards tidy ups

@rachelbaker I chatted to Adam Silverstein about tests, I don't think there is anything meaningful that can be added.

Putting against 5.0 with the intention of getting this in early for the folks at Jetpack, and other commenting dropins to check against.

Additional props required: @bradparbs

[peterwilsoncc]

In 42360:

Comments: Modernise JavaScript for comment reply links.

Update the comment reply JavaScript to be unobtrusive and use events rather than inline onclick attributes.

Along with bringing the code into the 2010s this prevents an edge-case in which addComment.moveForm() could be called before the JavaScript has loaded.

Props peterwilsoncc, bradparbs.
Fixes #31590.

[desrosj]

The since annotations need to be updated here.

[desrosj]

In 44388:

Docs: Update since tag for comment reply link improvements.

Originally added in [42360].

Fixes #31590.