Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#50500 closed defect (bug) (fixed)

Customize: JavaScript error breaks Customizer when loading locked changeset

Reported by: dlh's profile dlh Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.5 Priority: normal
Severity: major Version: 5.3
Component: Customize Keywords: has-patch
Focuses: Cc:

Description

The Customizer includes a locking mechanism to prevent two users from editing the same changeset simultaneously. Currently, when opening a changeset while another user is editing it, a JavaScript error occurs when showing the "changeset is locked" overlay. The overlay does not appear, leaving the Customizer in a broken state.

To replicate, an installation with two administrator accounts is needed:

  1. Open the Customizer while logged in as the first administrator. Make a change and save a draft of the changeset. Leave this window open.
  1. In an incognito window, log in as the second administrator and open the Customizer.

The expected behavior is that an overlay appears notifying the second administrator that another user is customizing the site. Instead, a JavaScript error appears in the console.

I believe the notification was broken in #45066 for WordPress 5.3.

The Customizer's LockedNotification, which notifies the user that the changeset is locked, does not specify a default message, which means it uses the value of null from the base Notification class.

As a result, the message passed to wp.a11y.speak() for the notification created by startLock() is also null: https://github.com/WordPress/wordpress-develop/blob/7b28cdbdee351ddf0c580b8a2d68050ee90583b8/src/js/_enqueues/wp/customize/controls.js#L253

Prior to 5.3, the wp.a11y.speak() JS function forgivingly handled empty non-string input via wp.sanitize.stripTags(): https://github.com/WordPress/wordpress-develop/blob/7b28cdbdee351ddf0c580b8a2d68050ee90583b8/src/js/_enqueues/wp/sanitize.js#L24

The current a11y package does not provide the same fallback: https://github.com/WordPress/gutenberg/blob/969d26f925896dce12a65cdaf06d248722970b38/packages/a11y/src/filter-message.js#L18

The attached patch ensures that the LockedNotification provides an appropriately typed default message parameter for the new package.

Whether these notifications should attempt to speak an empty value is a worthwhile question, but this ticket aims to limit its scope only to getting the Customizer back in action.

Arguably, the default message on the base Notification class should instead be a string instead of null, which would also address the issue.

I have not done so yet in this patch, again for reasons of scope, although I would be open to assurances that the change would be safe.

Attachments (1)

50500.diff (406 bytes) - added by dlh 4 years ago.

Download all attachments as: .zip

Change History (2)

@dlh
4 years ago

#1 @SergeyBiryukov
4 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 48210:

Customize: Ensure the default message in LockedNotification is set to a string instead of null.

This avoids a JS error when passing the message to wp.a11y.speak() when showing the "changeset is locked" overlay to prevent two users from editing the same changeset simultaneously.

Props dlh.
Fixes #50500.

Note: See TracTickets for help on using tickets.