Make WordPress Core

Opened 10 years ago

Closed 6 years ago

#31590 closed defect (bug) (fixed)

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

Reported by: peterwilsoncc's profile peterwilsoncc Owned by: rachelbaker's profile rachelbaker
Milestone: 5.1 Priority: normal
Severity: normal Version: 2.7
Component: Comments Keywords: has-patch
Focuses: javascript, template Cc:

Description

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.

Attachments (5)

31590.patch (1.6 KB) - added by peterwilsoncc 10 years ago.
31590.1.diff (2.7 KB) - added by peterwilsoncc 9 years ago.
obtrusive with expiring method queue
31590.2.diff (10.3 KB) - added by peterwilsoncc 9 years ago.
unobtrusive comment-reply.js
31590.diff (12.4 KB) - added by peterwilsoncc 7 years ago.
31590.3.diff (11.9 KB) - added by peterwilsoncc 7 years ago.

Download all attachments as: .zip

Change History (28)

#1 @DrewAPicture
10 years ago

  • Version changed from trunk to 2.7

#2 @DrewAPicture
10 years ago

  • Keywords has-patch added

#3 @DrewAPicture
10 years ago

  • Component changed from General to Comments
  • Focuses template added

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


10 years ago

#5 @peterwilsoncc
10 years ago

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.

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


10 years ago

#7 @peterwilsoncc
9 years ago

Related #22889 [33038] and [33039].

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

#8 @markjaquith
9 years ago

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
9 years ago

obtrusive with expiring method queue

#9 @peterwilsoncc
9 years ago

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.

#10 @peterwilsoncc
9 years ago

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
9 years ago

unobtrusive comment-reply.js

#11 @peterwilsoncc
9 years ago

  • Keywords dev-feedback added

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.

#12 @rachelbaker
9 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Owner set to rachelbaker
  • Status changed from new to assigned

@peterwilsoncc
7 years ago

#13 @peterwilsoncc
7 years ago

I've refreshed against trunk in 31590.diff.

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

#14 @rachelbaker
7 years ago

@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?

#15 @rachelbaker
7 years ago

  • Keywords needs-refresh added; dev-feedback removed

#16 @peterwilsoncc
7 years ago

  • Keywords needs-refresh removed
  • Milestone changed from Future Release to 5.0

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 plugins to check against.

Additional props required: @bradparbs

Last edited 7 years ago by peterwilsoncc (previous) (diff)

#17 @peterwilsoncc
7 years ago

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

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.

#18 @johnbillion
6 years ago

  • Milestone changed from 5.0 to 5.0.1
  • Resolution fixed deleted
  • Status changed from closed to reopened

#19 @pento
6 years ago

  • Milestone changed from 5.0.1 to 5.0.2

#20 @pento
6 years ago

  • Milestone changed from 5.0.2 to 5.0.3

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


6 years ago

#22 @desrosj
6 years ago

  • Milestone changed from 5.0.3 to 5.1

The since annotations need to be updated here.

#23 @desrosj
6 years ago

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

In 44388:

Docs: Update since tag for comment reply link improvements.

Originally added in [42360].

Fixes #31590.

Note: See TracTickets for help on using tickets.