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 | Owned by: | 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)
Change History (28)
This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.
10 years ago
This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.
10 years ago
#8
@
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?
#9
@
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.
#11
@
9 years ago
- Keywords dev-feedback added
- replace obtrusive JavaScript with unobtrusive JavaScript
- requires browser support for
addEventListener
andquerySelector
, other browsers fall back to?replytocom
links
- 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
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
@
9 years ago
- Milestone changed from Awaiting Review to Future Release
- Owner set to rachelbaker
- Status changed from new to assigned
#13
@
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
@
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?
#16
@
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
#18
@
6 years ago
- Milestone changed from 5.0 to 5.0.1
- Resolution fixed deleted
- Status changed from closed to reopened
Few thoughts on my patch above:
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:
Comment-reply.js config data is created as:
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.