Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#46280 closed defect (bug) (fixed)

Uncaught javascript error on comment-reply.js

Reported by: xknown's profile xknown Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 5.1.1 Priority: normal
Severity: normal Version: 5.1
Component: Comments Keywords: has-patch fixed-major
Focuses: Cc:

Description

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

Attachments (6)

46280.diff (2.1 KB) - added by pento 5 years ago.
46280.2.diff (1.8 KB) - added by peterwilsoncc 5 years ago.
46280.3.diff (1.8 KB) - added by peterwilsoncc 5 years ago.
hotfix.46280.diff (14.0 KB) - added by peterwilsoncc 5 years ago.
hotfix.46280.2.diff (15.8 KB) - added by peterwilsoncc 5 years ago.
46280.4.diff (1.8 KB) - added by peterwilsoncc 5 years ago.
Updated ready() docblock

Download all attachments as: .zip

Change History (31)

#1 @jorbin
5 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.1

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

#2 @peterwilsoncc
5 years ago

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

#3 @pento
5 years ago

  • Keywords has-patch needs-refresh needs-testing added; needs-patch removed

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.

#4 @peterwilsoncc
5 years ago

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.

#5 @pento
5 years ago

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

#6 @peterwilsoncc
5 years ago

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

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

#7 @jorbin
5 years ago

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.

#8 follow-up: @pento
5 years ago

  • Milestone changed from 5.1 to 5.1.1

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.

#9 in reply to: ↑ 8 @peterwilsoncc
5 years ago

  • Keywords needs-refresh removed
  • Owner set to peterwilsoncc
  • Status changed from new to assigned

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.

#10 @peterwilsoncc
5 years ago

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

#11 @peterwilsoncc
5 years ago

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

#12 @pento
5 years ago

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!

#13 @peterwilsoncc
5 years ago

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

#14 @pento
5 years ago

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

#15 @Otto42
5 years ago

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.

This ticket was mentioned in Slack in #forums by otto42. View the logs.


5 years ago

This ticket was mentioned in Slack in #forums by jorbin. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-comments by lukecarbis. View the logs.


5 years ago

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


5 years ago

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


5 years ago

#21 @peterwilsoncc
5 years ago

  • Keywords needs-testing removed

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

Updated ready() docblock

#22 @peterwilsoncc
5 years ago

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

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.

#23 @peterwilsoncc
5 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for merge to 5.1 branch.

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


5 years ago

#25 @peterwilsoncc
5 years ago

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

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.

Note: See TracTickets for help on using tickets.