Ensure that openref is defined before accessing to .current #62508

gigitux · 2024-06-12T12:45:33Z

What?

This PR improves a check to ensure that JS errors aren't triggered. You can see more details at: woocommerce/woocommerce#48308

Why?

By default, InsertionPointOpenRef is defined as an empty context in Gutenberg. You can find the context definition here.

When Gutenberg is used as a framework, the BlockTools component (that defined the context) may not be utilized in some projects (e.g., Customize Your Store project). In such cases, JavaScript will throw an error at the following line: use-in-between-inserter.js#L43.

To prevent this error, we need to update the check for InsertionPointOpenRef to handle scenarios where the context isn't defined. This PR will ensure that the context is defined, avoiding runtime errors.

github-actions · 2024-06-12T12:45:51Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: gigitux <[email protected]>
Co-authored-by: cbravobernal <[email protected]>
Co-authored-by: vcanales <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

gigitux · 2024-06-12T12:50:17Z

If this PR makes sense, it would be great if it could be backported to WordPress 6.6 🙇

cbravobernal · 2024-06-12T12:56:34Z

packages/block-editor/src/components/block-list/use-in-between-inserter.js

@@ -40,7 +40,7 @@ export function useInBetweenInserter() {
 			}

 			function onMouseMove( event ) {
-				if ( openRef.current ) {
+				if ( openRef === undefined || openRef.current ) {


I think this would work too.

Suggested change

if ( openRef === undefined || openRef.current ) {

if ( openRef?.current ) {

It was my first commit :D

However, with this code, the function will not return at line 44, but will continue with the logic.

BTW, I don't have a strong opinion! What do you think?

These are different checks:

The proposed logic in the PR is bail on openRef === undefined OR if openRef.current is truthy, which I'm not sure is correct.

@cbravobernal's suggestion bails on openRef truthy, but openRef === undefined keeps going.

@gigitux Could you explain why the handler should bail on openRef === undefined?

It was my first commit :D

However, with this code, the function will not return at line 44, but will continue with the logic.

BTW, I don't have a strong opinion! What do you think?

Agh, we made the same mistake then 😆

Return means not showing the in-between inserter. openRef seems to be a reference of where the next block should be insterted.

So, if the reference of the next position to be insterted does not exist (is undefined), the indicator should be hidden.

PD: I cannot get openRef.current to return true while testing on a post editor.

@vcanales Good point! Thanks for clarifying this! My first intuition was the right one. Sometimes overthinking isn't a good idea 😅

Oh now I understand, thanks for the explanation @cbravobernal
@gigitux I think openRef === undefined || openRef.current is the correct check, since we don't want to show the inserter when the next ref is undefined.

This is from where I understood that returning is hiding the inserter btw:

gutenberg/packages/block-editor/src/components/block-list/use-in-between-inserter.js

Line 132 in 2d34e2a

// Don't show the inserter when hovering above (conflicts with

vcanales

Could I ask you to also add a comment to the handler explaining its flow a little bit?

@cbravobernal's comment explains part of why we're returning early, but I still find it hard to grasp at a glance by just looking at the code

Return means not showing the in-between inserter. openRef seems to be a reference of where the next block should be insterted.

So, if the reference of the next position to be insterted does not exist (is undefined), the indicator should be hidden.

gigitux · 2024-06-12T13:50:35Z

Some clarifications: it looks like this handler is used to render the inserter between blocks.

This is the default behavior:

Screen.Capture.on.2024-06-12.at.15-35-01.mp4

This is the behavior if I define the openRef as true. The inserter between blocks isn't rendered:

Screen.Capture.on.2024-06-12.at.15-35-27.mp4

Now, this hook works only if the component is inside the InsertionPointOpenRef.Provider hierarchy. For this reason, I think that it doesn't make sense to continue with the flow in case openRef is undefined.

I added a comment with this commit 2ea0e3e, but I'm open to feedback.

Thanks for the review and help! 🙇

vcanales

Looks good. Thanks for illustrating and documenting too!

cbravobernal

Thanks for the clarification @gigitux !

…s#62508) * Ensure that openref is defined before accessing to .current * add comment Co-authored-by: gigitux <[email protected]> Co-authored-by: cbravobernal <[email protected]> Co-authored-by: vcanales <[email protected]>

* Ensure that openref is defined before accessing to .current * add comment Co-authored-by: gigitux <[email protected]> Co-authored-by: cbravobernal <[email protected]> Co-authored-by: vcanales <[email protected]>

ellatrix · 2024-06-18T07:30:49Z

I just cherry-picked this PR to the wp/6.6-beta-3 branch to get it included in the next release: 49cc18e

* Ensure that openref is defined before accessing to .current * add comment Co-authored-by: gigitux <[email protected]> Co-authored-by: cbravobernal <[email protected]> Co-authored-by: vcanales <[email protected]>

gigitux requested a review from ellatrix as a code owner June 12, 2024 12:45

Ensure that openref is defined before accessing to .current

d99d934

gigitux force-pushed the fix/use-in-between-inserter-undefined branch from c069148 to d99d934 Compare June 12, 2024 12:46

gigitux mentioned this pull request Jun 12, 2024

[CYS - Full Composability]: Cannot read properties of undefined (reading 'current') woocommerce/woocommerce#48308

Closed

cbravobernal reviewed Jun 12, 2024

View reviewed changes

cbravobernal added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Jun 12, 2024

gigitux requested a review from cbravobernal June 12, 2024 13:03

vcanales reviewed Jun 12, 2024

View reviewed changes

gigitux force-pushed the fix/use-in-between-inserter-undefined branch from 8dd228f to d99d934 Compare June 12, 2024 13:20

vcanales self-requested a review June 12, 2024 13:24

add comment

2ea0e3e

vcanales approved these changes Jun 12, 2024

View reviewed changes

cbravobernal approved these changes Jun 12, 2024

View reviewed changes

cbravobernal enabled auto-merge (squash) June 12, 2024 14:06

cbravobernal merged commit 3bb138e into WordPress:trunk Jun 12, 2024
62 checks passed

github-actions bot added this to the Gutenberg 18.6 milestone Jun 12, 2024

ellatrix added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Jun 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that openref is defined before accessing to .current #62508

Ensure that openref is defined before accessing to .current #62508

gigitux commented Jun 12, 2024

github-actions bot commented Jun 12, 2024 •

edited

Loading

gigitux commented Jun 12, 2024

cbravobernal Jun 12, 2024

gigitux Jun 12, 2024 •

edited

Loading

vcanales Jun 12, 2024 •

edited

Loading

cbravobernal Jun 12, 2024 •

edited

Loading

gigitux Jun 12, 2024

vcanales Jun 12, 2024

cbravobernal Jun 12, 2024

vcanales left a comment •

edited

Loading

gigitux commented Jun 12, 2024

vcanales left a comment

cbravobernal left a comment

ellatrix commented Jun 18, 2024

	if ( openRef === undefined \|\| openRef.current ) {
	if ( openRef?.current ) {

Ensure that openref is defined before accessing to .current #62508

Ensure that openref is defined before accessing to .current #62508

Conversation

gigitux commented Jun 12, 2024

What?

Why?

github-actions bot commented Jun 12, 2024 • edited Loading

gigitux commented Jun 12, 2024

cbravobernal Jun 12, 2024

Choose a reason for hiding this comment

gigitux Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

vcanales Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

cbravobernal Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

gigitux Jun 12, 2024

Choose a reason for hiding this comment

vcanales Jun 12, 2024

Choose a reason for hiding this comment

cbravobernal Jun 12, 2024

Choose a reason for hiding this comment

vcanales left a comment • edited Loading

Choose a reason for hiding this comment

gigitux commented Jun 12, 2024

vcanales left a comment

Choose a reason for hiding this comment

cbravobernal left a comment

Choose a reason for hiding this comment

ellatrix commented Jun 18, 2024

github-actions bot commented Jun 12, 2024 •

edited

Loading

gigitux Jun 12, 2024 •

edited

Loading

vcanales Jun 12, 2024 •

edited

Loading

cbravobernal Jun 12, 2024 •

edited

Loading

vcanales left a comment •

edited

Loading