Popover: Fix closest().parentNode null error #22264

sirreal · 2020-05-11T11:10:11Z

Description

Fix an issue in Popover that may throw an error under certain conditions.

Change the .closest( '.popover-slot' ).parentNode to use an optional chain ?.parentNode, which will keep the boundaryElement as undefined if no closest element is found.

#19322 (merged to master via #19344) introduced a chained call that may get parentNode of null, causing an error:

gutenberg/packages/components/src/popover/index.js

Lines 330 to 335 in f6a401b

    
           let boundaryElement; 
        
           if ( __unstableBoundaryParent ) { 
        
           	boundaryElement = containerRef.current.closest( 
        
           		'.popover-slot' 
        
           	).parentNode; 
        
           }

closest may return null if no matching element is found. See the MDN documentation for details.

I'm unfamiliar with the __unstableBoundaryParent feature, so I'd appreciate specific testing in Gutenberg or instructions on how to reproduce a minimal test case. It's simple to confirm the logic behind this fix works as expected in a browser:

<html>
	<body>
		<div class="foo">
			<div class="bar"></div>
		</div>
	</body>
</html>

document.querySelector('.bar').closest('.foo')?.parentNode
// <body></body>
document.querySelector('.bar').closest('.quux')?.parentNode
// undefined

// Without the optional chain, this is an error:
document.querySelector('.bar').closest('.quux').parentNode
// 💥 Uncaught TypeError: Cannot read property 'parentNode' of null

How has this been tested?

Manually modifying code in node_modules fixes error in fix(deps): update wordpress monorepo (take 2) Automattic/wp-calypso#41994
Light browser testing

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

github-actions · 2020-05-11T11:16:50Z

Size Change: +9 B (0%)

Total Size: 824 kB

Filename	Size	Change
`build/components/index.js`	180 kB	+9 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.02 kB	0 B
`build/annotations/index.js`	3.62 kB	0 B
`build/api-fetch/index.js`	4.08 kB	0 B
`build/autop/index.js`	2.82 kB	0 B
`build/blob/index.js`	620 B	0 B
`build/block-directory/index.js`	6.61 kB	0 B
`build/block-directory/style-rtl.css`	760 B	0 B
`build/block-directory/style.css`	761 B	0 B
`build/block-editor/index.js`	102 kB	0 B
`build/block-editor/style-rtl.css`	10.3 kB	0 B
`build/block-editor/style.css`	10.3 kB	0 B
`build/block-library/editor-rtl.css`	7.12 kB	0 B
`build/block-library/editor.css`	7.12 kB	0 B
`build/block-library/index.js`	115 kB	0 B
`build/block-library/style-rtl.css`	7.34 kB	0 B
`build/block-library/style.css`	7.34 kB	0 B
`build/block-library/theme-rtl.css`	683 B	0 B
`build/block-library/theme.css`	685 B	0 B
`build/block-serialization-default-parser/index.js`	1.88 kB	0 B
`build/block-serialization-spec-parser/index.js`	3.1 kB	0 B
`build/blocks/index.js`	48.1 kB	0 B
`build/components/style-rtl.css`	17 kB	0 B
`build/components/style.css`	16.9 kB	0 B
`build/compose/index.js`	6.66 kB	0 B
`build/core-data/index.js`	11.4 kB	0 B
`build/data-controls/index.js`	1.29 kB	0 B
`build/data/index.js`	8.45 kB	0 B
`build/date/index.js`	5.47 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.1 kB	0 B
`build/edit-navigation/index.js`	4.41 kB	0 B
`build/edit-navigation/style-rtl.css`	618 B	0 B
`build/edit-navigation/style.css`	617 B	0 B
`build/edit-post/index.js`	28 kB	0 B
`build/edit-post/style-rtl.css`	12.2 kB	0 B
`build/edit-post/style.css`	12.2 kB	0 B
`build/edit-site/index.js`	12.1 kB	0 B
`build/edit-site/style-rtl.css`	5.22 kB	0 B
`build/edit-site/style.css`	5.22 kB	0 B
`build/edit-widgets/index.js`	8.37 kB	0 B
`build/edit-widgets/style-rtl.css`	4.69 kB	0 B
`build/edit-widgets/style.css`	4.69 kB	0 B
`build/editor/editor-styles-rtl.css`	425 B	0 B
`build/editor/editor-styles.css`	428 B	0 B
`build/editor/index.js`	44.3 kB	0 B
`build/editor/style-rtl.css`	5.07 kB	0 B
`build/editor/style.css`	5.08 kB	0 B
`build/element/index.js`	4.65 kB	0 B
`build/escape-html/index.js`	734 B	0 B
`build/format-library/index.js`	7.63 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`	2.14 kB	0 B
`build/html-entities/index.js`	622 B	0 B
`build/i18n/index.js`	3.56 kB	0 B
`build/is-shallow-equal/index.js`	710 B	0 B
`build/keyboard-shortcuts/index.js`	2.51 kB	0 B
`build/keycodes/index.js`	1.94 kB	0 B
`build/list-reusable-blocks/index.js`	3.12 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`	5.29 kB	0 B
`build/notices/index.js`	1.79 kB	0 B
`build/nux/index.js`	3.4 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.56 kB	0 B
`build/primitives/index.js`	1.5 kB	0 B
`build/priority-queue/index.js`	789 B	0 B
`build/redux-routine/index.js`	2.85 kB	0 B
`build/rich-text/index.js`	14.8 kB	0 B
`build/server-side-render/index.js`	2.68 kB	0 B
`build/shortcode/index.js`	1.7 kB	0 B
`build/token-list/index.js`	1.28 kB	0 B
`build/url/index.js`	4.02 kB	0 B
`build/viewport/index.js`	1.84 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

It seems like it should be a relatively safe change, and I confirmed that boundaryElement is used, it's expected that it could be falsey (source). It's not exactly clear to me the precise circumstances under which this could occur, but the result of closest() could certainly produce an error, so the extra guarding is sensible.

cc @youknowriad @ellatrix who are likely more familiar with the implementation of __unstableBoundaryParent.

jorgefilipecosta

I verified boundaryElement can be falsy inside computePopoverPosition where it is used. I also did some smoke tests with popovers and everything seemed ok.

sirreal · 2020-05-11T15:09:02Z

Thanks for the quick reviews, folks 🙇‍♂️

sirreal added 2 commits May 11, 2020 12:53

Optional chain possibly null closest() result

70527c7

Add CHANGELOG entry

a021b05

sirreal marked this pull request as ready for review May 11, 2020 11:27

sirreal requested review from ajitbohra, chrisvanpatten, jorgefilipecosta and youknowriad as code owners May 11, 2020 11:27

sirreal self-assigned this May 11, 2020

sirreal added [Package] Components /packages/components [Type] Bug An existing feature does not function as intended labels May 11, 2020

sirreal requested a review from ellatrix May 11, 2020 11:28

sirreal mentioned this pull request May 11, 2020

fix(deps): update wordpress monorepo (take 2) Automattic/wp-calypso#41994

Closed

aduth approved these changes May 11, 2020

View reviewed changes

jorgefilipecosta approved these changes May 11, 2020

View reviewed changes

sirreal merged commit f7ed8c7 into master May 11, 2020

sirreal deleted the fix/null-popover-closest branch May 11, 2020 15:08

github-actions bot added this to the Gutenberg 8.1 milestone May 11, 2020

This was referenced May 13, 2020

"TypeError: Cannot read property 'parentNode' of null" in popover when using block-editor standalone #22013

Closed

Update block editor docs to incorporate the block-toolbar Popover Slot #22308

Merged

sirreal mentioned this pull request May 14, 2020

fix(deps): update wordpress monorepo Automattic/wp-calypso#42031

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Popover: Fix closest().parentNode null error #22264

Popover: Fix closest().parentNode null error #22264

sirreal commented May 11, 2020 •

edited

Loading

github-actions bot commented May 11, 2020 •

edited

Loading

aduth left a comment

jorgefilipecosta left a comment

sirreal commented May 11, 2020

	let boundaryElement;
	if ( __unstableBoundaryParent ) {
	boundaryElement = containerRef.current.closest(
	'.popover-slot'
	).parentNode;
	}

Popover: Fix closest().parentNode null error #22264

Popover: Fix closest().parentNode null error #22264

Conversation

sirreal commented May 11, 2020 • edited Loading

Description

How has this been tested?

Types of changes

Checklist:

github-actions bot commented May 11, 2020 • edited Loading

aduth left a comment

Choose a reason for hiding this comment

jorgefilipecosta left a comment

Choose a reason for hiding this comment

sirreal commented May 11, 2020

sirreal commented May 11, 2020 •

edited

Loading

github-actions bot commented May 11, 2020 •

edited

Loading