Skip to content

Block Editor: Clear toolbar timeout by ref on unmount#20546

Merged
aduth merged 2 commits into
masterfrom
fix/browser-toolbar-unsubscribe
Mar 2, 2020
Merged

Block Editor: Clear toolbar timeout by ref on unmount#20546
aduth merged 2 commits into
masterfrom
fix/browser-toolbar-unsubscribe

Conversation

@aduth
Copy link
Copy Markdown
Member

@aduth aduth commented Feb 28, 2020 •
edited
Loading

This pull request seeks to resolve a warning which can occur in some circumstances where a block toolbar component is unmounted while its deferred hide/show movers behavior is scheduled. The scheduling was not being cleared when the component unmounts.

Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in %s.%s a useEffect cleanup function 
        in BlockToolbar (created by BlockContextualToolbar)
        in div (created by NavigableContainer)
        in NavigableContainer (created by ForwardRef(NavigableContainer))
        in ForwardRef(NavigableContainer) (created by ForwardRef(NavigableMenu))
        in ForwardRef(NavigableMenu) (created by NavigableToolbar)
        in NavigableToolbar (created by BlockContextualToolbar)
        in div (created by BlockContextualToolbar)
        in BlockContextualToolbar (created by BlockPopover)
        in div (created by Animate)
        in div (created by ForwardRef)
        in ForwardRef (created by Animate)
        in Animate (created by Popover)
        in PopoverDetectOutside (created by WithFocusOutside(PopoverDetectOutside))
        in div (created by WithFocusOutside(PopoverDetectOutside))
        in WithFocusOutside(PopoverDetectOutside) (created by Popover)
        in Fill (created by Fill)
        in Fill (created by Popover)
        in Popover (created by BlockPopover)

Testing Instructions:

I originally observed this when running the meta box end-to-end tests in #20544 when building in development mode (where the React warnings logged in development mode will flag an end-to-end test failure).

You can confirm the following fails in master:

npm run dev
npm run test-e2e packages/e2e-tests/specs/editor/plugins/meta-attribute-block.test.js

Then change to this branch. Running those commands again should show success.

@aduth aduth added [Type] Bug An existing feature does not function as intended [Package] Block editor /packages/block-editor Backport to WP Core labels Feb 28, 2020
@aduth aduth requested a review from youknowriad February 28, 2020 22:01
@aduth aduth requested a review from ellatrix as a code owner February 28, 2020 22:01
@aduth
Copy link
Copy Markdown
Member Author

aduth commented Feb 28, 2020

Related: #14845

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Feb 28, 2020 •
edited
Loading

Size Change: -31 B (0%)

Total Size: 866 kB

Filename Size Change
build/block-editor/index.js 104 kB +8 B (0%)
build/block-editor/style-rtl.css 10.5 kB -6 B (0%)
build/block-editor/style.css 10.5 kB -6 B (0%)
build/edit-post/index.js 90.9 kB -2 B (0%)
build/edit-post/style-rtl.css 8.53 kB -13 B (0%)
build/edit-post/style.css 8.53 kB -12 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-library/editor-rtl.css 7.66 kB 0 B
build/block-library/editor.css 7.66 kB 0 B
build/block-library/index.js 116 kB 0 B
build/block-library/style-rtl.css 7.49 kB 0 B
build/block-library/style.css 7.5 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.6 kB 0 B
build/components/index.js 191 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 5.76 kB 0 B
build/core-data/index.js 10.5 kB 0 B
build/data-controls/index.js 1.03 kB 0 B
build/data/index.js 8.22 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-site/index.js 4.63 kB 0 B
build/edit-site/style-rtl.css 2.51 kB 0 B
build/edit-site/style.css 2.51 kB 0 B
build/edit-widgets/index.js 4.42 kB 0 B
build/edit-widgets/style-rtl.css 2.59 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 325 B 0 B
build/editor/editor-styles.css 327 B 0 B
build/editor/index.js 44.6 kB 0 B
build/editor/style-rtl.css 4.01 kB 0 B
build/editor/style.css 4 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.6 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.92 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.48 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.02 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.54 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@aduth aduth force-pushed the fix/browser-toolbar-unsubscribe branch from 81ac91c to d6024dc Compare February 28, 2020 23:21
youknowriad
youknowriad reviewed Mar 2, 2020
View reviewed changes
Comment thread packages/block-editor/src/components/block-toolbar/utils.js Outdated
[ isFocused ]
);

useEffect( () => () => clearTimeout( timeoutRef.current ) );
Copy link
Copy Markdown
Contributor

@youknowriad youknowriad Mar 2, 2020

Choose a reason for hiding this comment

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

If you want to uun this on unmount, you need to add [] as a second argument of useEffect otherwise it runs on each render.

Copy link
Copy Markdown
Member

@gziolo gziolo Mar 2, 2020

Choose a reason for hiding this comment

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

Good catch Riad, I see it explained in React docs:

The default behavior for effects is to fire the effect after every completed render. That way an effect is always recreated if one of its dependencies changes.

If you want to run an effect and clean it up only once (on mount and unmount), you can pass an empty array ([]) as a second argument. This tells React that your effect doesn’t depend on any values from props or state, so it never needs to re-run. This isn’t handled as a special case — it follows directly from how the dependencies array always works.

https://reactjs.org/docs/hooks-reference.html#conditionally-firing-an-effect

Copy link
Copy Markdown
Member

@gziolo gziolo Mar 2, 2020

Choose a reason for hiding this comment

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

Fixed in 02eb218.

Copy link
Copy Markdown
Member Author

@aduth aduth Mar 2, 2020

Choose a reason for hiding this comment

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

Good catch, thanks for spotting @youknowriad, and for the fix @gziolo .

@gziolo gziolo requested a review from jorgefilipecosta March 2, 2020 09:51
gziolo
gziolo reviewed Mar 2, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@gziolo gziolo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I still could see a similar error but triggered from a different place:

Screen Shot 2020-03-02 at 12 52 32

The only way I could reproduce is by doing the following:

  1. Add a few Paragraph blocks.
  2. Save the draft and refresh the page.
  3. Remove the block and observe JS console in Dev Tools.

It only gets triggered for the first block removed. It works perfectly fine after that when removing any blocks.

gziolo
gziolo approved these changes Mar 2, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I found a similar but apparently a different issue. Let's merge this fix for now 🚢

@aduth aduth merged commit 72eca2a into master Mar 2, 2020
@aduth aduth deleted the fix/browser-toolbar-unsubscribe branch March 2, 2020 12:59
@github-actions github-actions Bot added this to the Gutenberg 7.7 milestone Mar 2, 2020
@aduth
Copy link
Copy Markdown
Member Author

aduth commented Mar 2, 2020

I removed 5.4-related project and issue assignment. As far as I can tell, this was only introduced as of #19344, so it doesn't need to be backported. Let me know if I'm wrong.

@gziolo
Copy link
Copy Markdown
Member

gziolo commented Mar 2, 2020

I removed 5.4-related project and issue assignment. As far as I can tell, this was only introduced as of #19344, so it doesn't need to be backported. Let me know if I'm wrong.

I know that issue existed forever, but it might another one I shared as well 🤷‍♂ I guess, it's fine to wait for WP 5.5 rather than work on two independent fixes. Unless you think it might have some really negative consequences.

@aduth
Copy link
Copy Markdown
Member Author

aduth commented Mar 2, 2020

Well, the specific file changed was only introduced as of #19344. If there's a related issue, I expect it might need its own patch, since this one won't apply cleanly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowriad youknowriad youknowriad left review comments

@gziolo gziolo gziolo approved these changes

@ellatrix ellatrix Awaiting requested review from ellatrix

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

Assignees

No one assigned

Labels

[Package] Block editor /packages/block-editor [Type] Bug An existing feature does not function as intended

Projects

None yet

Milestone

Gutenberg 7.7

Development

Successfully merging this pull request may close these issues.

4 participants

@aduth @gziolo @youknowriad @jorgefilipecosta