Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix block inserter WSOD when an empty reusable block exists #26484

Merged
merged 1 commit into from Oct 28, 2020

Conversation

noahtallen
Copy link
Member

@noahtallen noahtallen commented Oct 27, 2020

Problem: when a user has an empty reusable block (e.g. the post content is an empty string), the block inserter selector passes the content object instead of the content string into the parser. This causes an editor WSOD.

The fix is to check for a string instead of a truthy value. The old truthy check doesn't work for empty strings because empty strings are falsey.

This error happens on any post, even if it does not use reusabel blocks. This is because the inserter always attempts to load the reusable blocks so the user can preview them.

To test:

  1. create a reusable block
  2. go to the reusable block post editor via manage reusable blocks
  3. delete all blocks from the reusable block. It should be completely empty.
  4. Go to any post on your site in Gutenberg.
  5. Click the inserter button.

Previous behavior:
6. The editor crashes to a WSOD.

Fixed behavior:
6. The inserter opens.

closes #26485

@noahtallen noahtallen changed the title Check for string insteadd of truthy Fix block inserter WSOD when an empty reusable block exists Oct 27, 2020
@noahtallen noahtallen self-assigned this Oct 27, 2020
@noahtallen noahtallen added [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended labels Oct 27, 2020
@github-actions
Copy link

github-actions bot commented Oct 27, 2020

Size Change: +4 B (0%)

Total Size: 1.2 MB

Filename Size Change
build/block-editor/index.js 130 kB +4 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.53 kB 0 B
build/api-fetch/index.js 3.34 kB 0 B
build/autop/index.js 2.73 kB 0 B
build/blob/index.js 664 B 0 B
build/block-directory/index.js 8.58 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/style-rtl.css 11 kB 0 B
build/block-editor/style.css 11 kB 0 B
build/block-library/editor-rtl.css 8.93 kB 0 B
build/block-library/editor.css 8.93 kB 0 B
build/block-library/index.js 146 kB 0 B
build/block-library/style-rtl.css 7.75 kB 0 B
build/block-library/style.css 7.75 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 741 B 0 B
build/block-serialization-default-parser/index.js 1.78 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.6 kB 0 B
build/components/index.js 172 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.62 kB 0 B
build/core-data/index.js 12.1 kB 0 B
build/data-controls/index.js 679 B 0 B
build/data/index.js 8.61 kB 0 B
build/date/index.js 31.8 kB 0 B
build/deprecated/index.js 769 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.42 kB 0 B
build/edit-navigation/index.js 10.7 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.37 kB 0 B
build/edit-post/style.css 6.35 kB 0 B
build/edit-site/index.js 22.2 kB 0 B
build/edit-site/style-rtl.css 3.79 kB 0 B
build/edit-site/style.css 3.79 kB 0 B
build/edit-widgets/index.js 26.6 kB 0 B
build/edit-widgets/style-rtl.css 3.09 kB 0 B
build/edit-widgets/style.css 3.09 kB 0 B
build/editor/editor-styles-rtl.css 480 B 0 B
build/editor/editor-styles.css 482 B 0 B
build/editor/index.js 42.6 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.84 kB 0 B
build/element/index.js 4.44 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 7.47 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 1.74 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.55 kB 0 B
build/is-shallow-equal/index.js 712 B 0 B
build/keyboard-shortcuts/index.js 2.38 kB 0 B
build/keycodes/index.js 1.84 kB 0 B
build/list-reusable-blocks/index.js 3.01 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.11 kB 0 B
build/notices/index.js 1.69 kB 0 B
build/nux/index.js 3.26 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.44 kB 0 B
build/primitives/index.js 1.35 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/reusable-blocks/index.js 3.06 kB 0 B
build/rich-text/index.js 13 kB 0 B
build/server-side-render/index.js 2.6 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.24 kB 0 B
build/url/index.js 4.05 kB 0 B
build/viewport/index.js 1.74 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

Copy link
Member

@vindl vindl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified that this fixes the issue. 👍

@noahtallen noahtallen force-pushed the fix/inserter-problem-empty-reusable-block branch from fb126e6 to ba1841b Compare October 27, 2020 00:35
Copy link
Contributor

@jeyip jeyip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can also confirm that opening an inserter after creating an empty reusable block behaves as expected. I double checked the fix in multiple browsers for safe measure (Firefox, Chrome, Safari, IE11, Edge)

@noahtallen noahtallen added [Feature] Inserter The main way to insert blocks using the + button in the editing interface [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) labels Oct 27, 2020
@fullofcaffeine
Copy link
Member

This tests well 👍🏻

@talldan
Copy link
Contributor

talldan commented Oct 27, 2020

Should this fix be backported to WordPress 5.6? (@tellthemachines just making you aware of this one).

I'll label it up and add it the board just to be safe.

@talldan talldan added the Backport to WP Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 27, 2020
@talldan talldan added this to Needs Review in WordPress 5.6.x Must Haves via automation Oct 27, 2020
@talldan
Copy link
Contributor

talldan commented Oct 27, 2020

Potentially also fixes #26381

edit: now confirmed by the reporter, so I've linked this PR as the one that fixes it.

Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can confirm it fixes the issue, and the change makes sense 👍

Rebasing the PR should hopefully get the tests to pass, as they've been a bit more reliable today.

@sirreal sirreal force-pushed the fix/inserter-problem-empty-reusable-block branch from ba1841b to c10c09c Compare October 27, 2020 11:12
@sirreal
Copy link
Member

sirreal commented Oct 27, 2020

I've rebased this PR.

Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm digging in a bit to try to better understand this. The changes happened in #25859.

This changes seems to be an adequate fix although I'd like to understand what's going on. As far as I can tell we'd like to expect content.raw is a string and use that if so, but we don't expect it to be inconsistent for reasons that are unclear to me. CC: @adamziel in case you recall any details.

@talldan
Copy link
Contributor

talldan commented Oct 28, 2020

I tried re-running the tests a couple of times, but couldn't get them to pass. The failing tests are also failing in master, so I'll merge this one.

@talldan talldan merged commit 5a43ca9 into master Oct 28, 2020
WordPress 5.6.x Must Haves automation moved this from Needs Review to Done Oct 28, 2020
@talldan talldan deleted the fix/inserter-problem-empty-reusable-block branch October 28, 2020 02:52
@github-actions github-actions bot added this to the Gutenberg 9.3 milestone Oct 28, 2020
@tellthemachines tellthemachines mentioned this pull request Oct 30, 2020
6 tasks
tellthemachines added a commit that referenced this pull request Nov 2, 2020
* nest content styles in container for higher specificity (#26487)

* Reusable Blocks: Make the number retrieved from the API unlimited (#26486)

* Make the number of reusable blocks returned from the API unlimited

This is to fix #26352. Having no limit seems like it could be risky, but
there are a number of other places in the codebase where all entities
are returned in one call to the API, including categories, which could
return a similar amount of data to this.

* Remove unnecessary quotes to fix lint error.

* Fix block inserter WSOD when an empty reusable block exists (#26484)

* Latest Posts: Bring back classname on post list (#26477)

* [Heading Block] Fix double alignment controls in toolbar (#26492)

* fix heading alignments

* add proper supports in deprecation

* update all previous deprecations

* regenerate fixtures for heading

* change migration call

* remove previous className + revert saves

* Revert "regenerate fixtures for heading"

This reverts commit 27af8c3.

* change fixtures

* create new fixtures + fix deprecation save

* address review feedback

* URLInput: Use debounce() instead of throttle() (#26529)

Wait until the user finishes typing before sending an AJAX request. This
ensures that there isn't an AJAX request sent every 200 ms while the
user is typing.

* Fix single column block display for smaller screens. (#26438)

If there is only one column, don't force a 50% flex-basis for small screens.

* Fix incorrectly pluralised strings (#26565)

* Change block mover label i18n

* Update remove block i18n

* Ensure footer remains position fixed when navigating regions (#26533)

* Update package-lock file to ensure static analysis task passes (#26528)

* Removes extra fullstop (#26586)

Co-authored-by: Addison Stavlo <Stavz01@gmail.com>
Co-authored-by: Paul Bunkham <paul@dobit.co.uk>
Co-authored-by: Noah Allen <noahtallen@gmail.com>
Co-authored-by: Kelly Dwan <ryelle@users.noreply.github.com>
Co-authored-by: Nik Tsekouras <ntsekouras@outlook.com>
Co-authored-by: Robert Anderson <robert@noisysocks.com>
Co-authored-by: Aaron D. Campbell <aaron@AaronDCampbell.com>
Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
Co-authored-by: Tammie Lister <tammie@automattic.com>
@tellthemachines tellthemachines removed the Backport to WP Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Inserter The main way to insert blocks using the + button in the editing interface [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended
Projects
No open projects
7 participants