Tracking: fix inserter menu no-results issue

[retrofox]

Changes proposed in this Pull Request

This PR changes the way to track no-results search result from the Inserter Menu, replacing event delegation by using an Slot, through of the, experimental, inserter menu extension.

Also, it implements a fallback implementation for the 7.8.1 core plugin version, since unfortunately, it has a bug. Basically, the hasItems parameter is always true, so we can't rely on it.
So, it checks the plugin version and applies the respective behavior according to this value. We should remove the fallback code once a new version of core-editor ships.

Markable changes

• Remove the event delegation tracker.
• Track same events using the __experimentalInserterMenuExtension component.
• It applies a temporary hacky fix to detect no-results search

Testing instructions

1. Update your testing site with these changes (SB)
2. In your testing site, open the client dev console and set up the debug

> localStorage.setItem( 'debug', 'wpcom-block-editor*' )

A hard refresh is needed.

3. Open the Inserter Menu
4. Open the Inserter Menu selector
5. Generate event typing there
6. Confirm that it behaves as expected. It means when you get results, it should trigger a wpcom_block_picker_search_term event, as the screenshot shows below.

blocks found event

When there aren't blocks there, it also should trigger the wpcom_block_picker_no_results event too:

blocks not found event

You can see what version is currently running in your site inspecting the debug line:

Fixes #41149

[matticbot]

Test live: https://calypso.live/?branch=update/tracking-fix-inserter-menu-no-results-issue

[matticbot]

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

[getdave]

This is a clever PR and a nice use of Slots. I'm mildly concerned that we're relying on experimental features, but I guess you'll have to be conscious of checking the progress of the feature as it moves towards becoming stable in Core.

This PR changes the way to track no-results search result from the Inserter Menu, replacing event delegation by using an Slot, through of the, experimental, inserter menu extension.

I wanted to check - is the purpose of the PR to fix a bug in the original event delegation approach or is this an entirely new approach? If the later, what advantages does this approach provide over the original?

I guess my concern is we may be adding another possible tracking approach, which is halfway between delegation and full Redux which adds further complexity to the tracking system.

I could be wrong...

Testing Results

I'm not entirely clear from the testing instructions whether I need to test against different Gutenberg versions and how to do this. So I decided to go with "stable" and "edge" versions on Dotcom:

I tested on a site stickered with Gutenberg Edge (running Gutenberg 7.9.0-rc.1) and I encountered the following error:

I then tested on a non-stickered site with 7.8.1 and I got the expected results and no errors:

I think there could be a problem with this on rc-1. If that is the case, then I would come back to whether this PR is adding additional complexity. If the delegation solution still works then do we need to add this more complex approach?

Thanks for your work here. Your knowledge of Gutenberg is impressive 👍

[retrofox]

... but I guess you'll have to be conscious of checking the progress of the feature as it moves towards becoming stable in Core.

Yeah, right. This is an experimental feature but I hope it could get stable. Anyway, we need to pay attention to changes in it.

I wanted to check - is the purpose of the PR to fix a bug in the original event delegation approach or is this an entirely new approach? If the later, what advantages does this approach provide over the original?

Both of them. Event delegation wasn't the issue but DOM inspection. In short, we are realying on a DOM select query to detect the no-results event. Since something changed in the DOM tree, this value is always true:

const hasResults = document.querySelectorAll( '.block-editor-inserter__no-results' ).length === 0;

It means that every time that something changes here, it could affect the tracking data.

The slot approach has an API that fits better for our requeriments. The __experimentalInserterMenuExtension provides four arguments: onSelect, onHover, filterValue, hasItems. We don't have to care about DOM tree since everthing that we need is already there, even more.

I guess my concern is we may be adding another possible tracking approach, which is halfway between delegation and full Redux which adds further complexity to the tracking system.

Event delegation should we replaced by a better one as soon as possible, imo. Relying to the DOM tree, that wasn't designed for it, sounds fragile. In fact, it's failing right now in production for this reason.
I think the reason to change the approach is more than enough. My only concern is that this block is experimental, but despite this, I still think it's more stable than the DOM approach.

I could be wrong...

Me too :-)

Testing Results

I'm not entirely clear from the testing instructions whether I need to test against different Gutenberg versions and how to do this. So I decided to go with "stable" and "edge" versions on Dotcom:

Sorry, my bad. Updating testing instructions.

I tested on a site stickered with Gutenberg Edge (running Gutenberg 7.9.0-rc.1) and I encountered the following error:

This is perfect. I've updated the PR adding a debug line to make it easier to test.

I then tested on a non-stickered site with 7.8.1 and I got the expected results and no errors:

I think there could be a problem with this on rc-1.

I don't think so. We need to check the string 7.8.1. any different (higher) version should be ok.

Thanks for your work here. Your knowledge of Gutenberg is impressive 👍

Not enough, tbh. Thanks for your words.

[getdave]

I retested this and for both 7.8.0 (stable) and 7.9.0 ("Gutenberg edge") it worked as expected. No more errors.

I'd say this is a good improvement because we're avoiding reliance on the DOM.

I'd like to understand more about whether using Slots is going to be a method of tracking we can use elsewhere to avoid reliance on event delegation. If it is then would you update the FG entry to ensure that it is documented as a preferred option over and above event delegation but below pure Redux? This will ensure folks are aware of it.

My take is that we won't be able to completely drop event delegation in al circumstances so it should remain "as is" - a last resort after all other options for tracking have been exhausted.

Thanks again for the great work here 👍

[getdave]

Suggested change

	// Skip whether isn't the 7.8.1 version.
	// Skip when isn't Gutenberg version 7.8.1.

[simison]

Shouldn't this be in ./features/tracking and just imported here?

[retrofox]

In that case, should we register the plugin there as well then, right?

[mtias]

@getdave slots are a primary extensibility interface that we encourage as it's more idiomatic and easier to test maintain. We aim for slots to be semantic so that there are no weird couplings. For this use case, it makes sense also because I'd imagine you might also want to render helpful guides for certain queries (like "page" returning something about "here's how you can create a new page").

[retrofox]

@getdave slots are a primary extensibility interface that we encourage as it's more idiomatic and easier to test maintain.

This comment makes me more confident about the approach that we did since my concerning point was just that: how reliable is this feature to considering to put it working on prod.