Uncaught javascript error on comment-reply.js

[xknown]

In r42360, there was a change that adds the following code

var supportsDataset = !! document.body.dataset;

While this seems to work in most cases, some (legacy) themes use XHTML doctype declarations like the classic p2 (​https://themes.trac.wordpress.org/browser/p2/1.5.8/header.php#L8). Using this theme along with WordPress 5.1, there's the following error being shown in the JS console

Uncaught TypeError: Cannot read property 'dataset' of null

[jorbin]

As this is a bug introduced in 5.1, moving to the milestone for consideration.

[peterwilsoncc]

Looking at the ​P2 demo, the theme is printing comment-reply in the HTML header rather than the HTML footer as it's listed as a ​dependency of another script.

init could be moved to the DOMContentLoaded event to account for this but I am in two minds as to whether this is a core or theme bug given part of the reason for #31590 was the script could throw an error on large comment threads.

[pento]

46280.diff​ fixes this particular error.

While testing, I noticed that moveForm() is called twice, because P2 adds its own onclick attribute to the Reply button. I'm thinking the clickEvent() should only be attached to the Reply button if that element has all of the necessary data attributes. Otherwise, assume that the comment was generated by a custom callback sent to wp_list_comments(), as P2 does.

I'm leaning towards targeting this fix for WordPress 5.1.1, as we're only a day out from WordPress 5.1 being released.

[peterwilsoncc]

46280.2.diff​ builds upon the initial patch:

• moves the dataset support check to the HTML element document.documentElement.dataset as the HTML element is available when the script is included in the HTML header. This allows the fix to touch fewer lines
• Uses document.readyState !== 'loading' to check if the page has loaded, in the initial patch document.body can be defined before the form is loaded.
• adds a check for required attributes to clickEvent() before calling moveForm() to allow for P2 style custom callbacks.

[pento]

I've had a look through the theme directory for themes that might have this problem.

To find all themes that use an XHTML doctype:

ag 'http://www.w3.org/TR/xhtml1' -l --php | awk '{split($0, flds, "/");print flds[1];}' | uniq > xhtml-themes.txt

To find all themes that print the comment-reply script in a custom location (assuming they print it in the head):

cat xhtml-themes.txt | xargs ag --php "wp_print_scripts\(\s*['\"]comment-reply['\"]"

To find all themes that put comment-reply in an array, which could be passed to wp_print_scripts(), wp_register_script(), or wp_enqueue_script():

cat xhtml-themes.txt | xargs ag --php "array\(([^)]|\n)*['\"]comment-reply['\"]"

After manually checking for false positives, this is the list of themes I've found:

bizway: 2000+ installs
blackbird: 2000+ installs
p2: 1000+ installs
p3: 70+ installs
p2-categories: 20+ installs

[peterwilsoncc]

Chatting to @xknown this morning, we realized the doctype wasn't relevant, causing this to affect many more sites.

• wp_print_scripts (​wpdirectory search): 17.5k installs approx
• comment-reply within array (​wp directory search): mostly false positives from hybrid and forked themes, 3-4k installs.

See this ​code bin for a POC with the HTML doctype.

[jorbin]

I don't think this needs to block release if we are going to do a quick 5.1.1 as proposed in ​https://make.wordpress.org/core/2019/02/20/wordpress-5-1-1/

Getting this patch in trunk and making it available as a part of ​https://wordpress.org/plugins/rapid-comment-reply/ could help limit the fallout for the themes it will affect.

[pento]

Thanks for the feedback, y'all. Of those 20k potentially affected sites, ~4k are running WordPress 5.0, so these are the sites that are likely to upgrade to 5.1 as soon as its released.

For the themes that use wp_print_scripts(), most of them check if comment threading is turned off before printing it, so a reasonable workaround is to turn that off.

I like the idea of releasing a hotfix for this: we could use the ​existing hotfix plugin for it, too. @peterwilsoncc: I can add you to that plugin, if you can wrangle adding the fix to it.

Given the impact, severity, and workarounds for this bug, I'm punting it to 5.1.1.

[peterwilsoncc]

Replying to pento:

@peterwilsoncc: I can add you to that plugin, if you can wrangle adding the fix to it.

Thanks, I'm happy to add the fix to the plugin.

[peterwilsoncc]

46280.3.diff​ Minor formatting fix on previous patch and includes the cutting the mustard check alongside document.readyState !== 'loading'.

[peterwilsoncc]

@pento as discussed via Slack, here's a copy of the patch for the hotfix plugin (without readme, etc).

[pento]

Thanks, @peterwilsoncc! A few small things:

• Line 61: Looks like overwriting $hotfixes is some leftover debugging code?
• Hot fix functions should be at the end of the file, they're ordered in ascending fashion.
• wp_hotfix_510_enqueue_comment_reply_js() has some bonus indenting.

Otherwise, looks good!

[peterwilsoncc]

Thanks @pento, fixes, readme and php header changes in hotfix.46280.2.diff​.

[pento]

👍🏻 to hotfix.46280.2.diff​, let's ship it.

[Otto42]

You'll probably want to update the Tested up to: line in both of these files as well:

​https://plugins.svn.wordpress.org/hotfix/trunk/readme.txt
​https://plugins.svn.wordpress.org/hotfix/tags/1.2/readme.txt

That will make the plugin more easily discoverable in the search engine.

[peterwilsoncc]

Tested in the following using either real browsers or via BrowserStack.

I'm happy with putting this in as it works as advertised in recent browsers, in older browsers it can use the fall back if a theme or plugin modifies the script in an unexpected fashion, ie by forcing it to the HTML header.

Browser tests

pass: script runs and moves comment reply form
FB pass: fall back pass using replytocom link, no console error

AJAX loaded, script in header:

• IE 11 - pass
• iOS 11 - pass
• iOS 12 - pass
• Firefox 65 - pass
• Chrome 72 - pass
• Safari 12 - pass
• Safari 11 - pass
• Android 8 Chrome - pass
• Android 7 Chrome - pass
• Android 6 chrome - pass
• IE 10 - FB pass
• IE 9 - FB pass
• IE 8 - FB pass
• Safari 5.1 - FB pass

normally loaded, script in header:

• IE 11 - pass
• iOS 11 - pass
• iOS 12 - pass
• Firefox 65 - pass
• Chrome 72 - pass
• Safari 12 - pass
• Safari 11 - pass
• Android 8 Chrome - pass
• Android 7 Chrome - pass
• Android 6 chrome - pass
• IE 10 - FB Pass (readyState browser bug)
• IE 9 - FB Pass (readyState browser bug)
• IE 8 - FB pass (readyState incomplete support)
• Safari 5.1 - pass

Script in footer, IE as expected

• As above with IE 9 and 10 passing.

[peterwilsoncc]

Updated ready() docblock

[peterwilsoncc]

In 44794:

Comments: Allow for comment-reply.js to be loaded in the HTML header.

Allows for themes or plugins setting the comment-reply JavaScript as a dependency of an HTML header script. This in turn causes comment-reply.js to be loaded early, requiring execution to be delayed.

Props pento, peterwilsoncc, jorbin for feedback.
Fixes #46280.

[peterwilsoncc]

Reopening for merge to 5.1 branch.

[peterwilsoncc]

In 44795:

Comments: Allow for comment-reply.js to be loaded in the HTML header.

Allows for themes or plugins setting the comment-reply JavaScript as a dependency of an HTML header script. This in turn causes comment-reply.js to be loaded early, requiring execution to be delayed.

Props pento, peterwilsoncc, jorbin for feedback.
Merges [44794] to the 5.1 branch.
Fixes #46280.