Block Editor Tracking: Remove "wpcom_block_editor_close_click" from W button #53864

david-szabo97 · 2021-06-21T12:08:21Z

Changes proposed in this Pull Request

This is a companion PR to #53500

wpcom_block_editor_close_click is used to track when the editor is closed. A few months ago a navigation sidebar was implemented for the post editor, that time the W button's behaviour changed. It opened the navigation sidebar rather than closing the editor. To make sure we are tracking the right behaviour, this PR changes the following:

Remove wpcom_block_editor_close_click from the WP button when it's supposed to open a navigation sidebar.

Testing instructions

Follow steps to enable tracking debugging at PCYsg-nrf-p2
Open post editor
Click on the W button to open the navigation sidebar.
Confirm "wpcom_block_editor_close_click" event is NOT triggered

E2E test will fail for Site Editor.

Related to #53500 #53410

matticbot · 2021-06-21T12:11:19Z

Link to Calypso live: https://calypso.live?image=registry.a8c.com/calypso/app:build-9241
Link to Jetpack Cloud live: https://calypso.live?image=registry.a8c.com/calypso/app:build-9241&env=jetpack

matticbot · 2021-06-21T12:15:17Z

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.

matticbot · 2021-06-21T13:44:36Z

This PR modifies the release build for wpcom-block-editor

To test your changes on WordPress.com, run install-plugin.sh wpcom-block-editor update/wpcom-nav-sidebar-tracking on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-l4k-p2

Addison-Stavlo

This is working well in my testing!

test/e2e/lib/gutenberg/tracking/general-tests.js

Addison-Stavlo · 2021-06-21T13:47:34Z

test/e2e/lib/gutenberg/tracking/general-tests.js

+		const editorCloseClickNotFired =
+			eventsStack.filter( ( [ eventName ] ) => eventName === 'wpcom_block_editor_close_click' )
+				.length === 0;
+		assert.strictEqual(


Should we add a second test to ensure this fires as expected on < All Pages / < All Posts / < Dashboard ?

Any idea how to tackle this? I've the tests written (locally) but clicking on the button closes the editor. Which removes the E2E events stack.

Ohh good point. Im not quite sure the best way to handle that. Maybe a simple way would be to define the selector for this tracks event as a constant, use that import in both the test and the tracks event, and have the test just verify that selector is present where we would expect it to be? It wouldn't technically test that the event is firing, but it would at least notify us if the selector that event is tied to is no longer present 🤔 .

@WunderBart Any idea? Is there a way to disable/prevent navigations? Googled it but no luck 😞

Not sure I understand the issue here, could you elaborate please?

Actually, should we even test it here? Isn't that something we should do in ETK?

Addison-Stavlo · 2021-06-21T13:52:51Z

test/e2e/lib/gutenberg/gutenberg-editor-component.js

+		} else {
+			await driverHelper.clickWhenClickable(
+				this.driver,
+				By.css( '.wpcom-block-editor-nav-sidebar-toggle-sidebar-button__button' )


I think this selector works for toggling regardless of open/close state.

It doesn't work. The button is not visible so Selenium can't click on it. That's why we need to use the button in the sidebar.

I think if you let CSS select the first available element it should toggle the sidebar alright:

const dismissSidebarButtonSelector = '.wpcom-block-editor-nav-sidebar-nav-sidebar__dismiss-sidebar-button'; const toggleSidebarButtonSelector = '.wpcom-block-editor-nav-sidebar-toggle-sidebar-button__button'; async toggleNavigationSidebar() { return await driverHelper.clickWhenClickable( this.driver, By.css( toggleSidebarButtonSelector + ', ' + dismissSidebarButtonSelector ); ); }

...assuming those selectors are correct 😉

Just checked those out in Gutenberg, and it seems we can use aria attributes as selectors. It's generally a better practice to use user-facing attributes as selectors as they're less likely to change and break the test. Notice also the rename to toggleBlockEditorSidebar as it seems to be the UI component name. Here's the updated version of the above toggle method:

async toggleBlockEditorSidebar() { const dismissSidebarButtonSelector = 'button[aria-label="Block editor sidebar"][aria-expanded="false"]'; const toggleSidebarButtonSelector = 'button[aria-label="Close block editor sidebar"]'; return await driverHelper.clickWhenClickable( this.driver, By.css( toggleSidebarButtonSelector + ', ' + dismissSidebarButtonSelector ); ); }

Addison-Stavlo

Im noticing there are calypso_editor_sidebar_* events set up in the editing toolkit for the opening the panel and closing the editor via the dotcom sidebar version. It seems like some of this in the post editor might be redundant.

Addison-Stavlo · 2021-06-21T14:07:32Z

apps/wpcom-block-editor/src/wpcom/features/tracking/wpcom-block-editor-nav-sidebar-open.js

+export default () => ( {
+	id: 'wpcom-block-editor-nav-sidebar-open',
+	selector:
+		'.edit-post-header .wpcom-block-editor-nav-sidebar-toggle-sidebar-button__button[aria-expanded="false"]',
+	type: 'click',
+	capture: true,
+	handler: () => tracksRecordEvent( 'wpcom_block_editor_nav_sidebar_open' ),
+} );


Looking more, I don't think we need to add this for the post-editor. It is already tracking calypso_editor_sidebar_open:

wp-calypso/apps/editing-toolkit/editing-toolkit-plugin/wpcom-block-editor-nav-sidebar/src/components/toggle-sidebar-button/index.tsx

Line 30 in f2af217

recordTracksEvent( `calypso_editor_sidebar_open` );

We don't easily see it in our logs since that comes from editing-toolkit and not wpcom-block-editor, but looking at t.gif under network requests should show it happening.

Addison-Stavlo · 2021-06-21T14:12:19Z

apps/wpcom-block-editor/src/wpcom/features/tracking/wpcom-block-editor-close-click.js

 export default () => ( {
 	id: 'wpcom-block-editor-close-click',
-	selector: '.edit-post-header .edit-post-fullscreen-mode-close',
+	selector:
+		'.edit-post-header .edit-post-fullscreen-mode-close:not(.wpcom-block-editor-nav-sidebar-toggle-sidebar-button__button), .wpcom-block-editor-nav-sidebar-nav-sidebar__home-button',
 	type: 'click',


Im starting to second guess our decision on this now. Looking further there is also a calypso_editor_sidebar_editor_close set up:

wp-calypso/apps/editing-toolkit/editing-toolkit-plugin/wpcom-block-editor-nav-sidebar/src/components/nav-sidebar/index.tsx

Line 131 in f2af217

recordTracksEvent( 'calypso_editor_sidebar_editor_close' );

Although, I am not seeing this one firing when I use this close button for some reason. 🤔 I see it in recent tracks history though, there must be some inconsistency with the conditional, maybe it needs to record the event regardless of that condition.

david-szabo97 · 2021-06-22T09:02:11Z

That's a nice find @Addison-Stavlo ! Since the ETK is tracking open, dismiss and editor close, I guess we can just scope this down to removing the wpcom_block_editor_close_click event.

WunderBart · 2021-06-22T15:33:47Z

test/e2e/lib/gutenberg/gutenberg-editor-component.js

+	async toggleNavigationSidebar() {
+		const isOpen = await driverHelper.isElementLocated(
+			this.driver,
+			By.css( '.wpcom-block-editor-nav-sidebar-nav-sidebar__click-guard' )


Is this nav-sidebar-nav-sidebar part intended here?

WunderBart · 2021-06-22T15:40:54Z

test/e2e/lib/gutenberg/gutenberg-editor-component.js

+		} else {
+			await driverHelper.clickWhenClickable(
+				this.driver,
+				By.css( '.wpcom-block-editor-nav-sidebar-toggle-sidebar-button__button' )


I think if you let CSS select the first available element it should toggle the sidebar alright:

const dismissSidebarButtonSelector = '.wpcom-block-editor-nav-sidebar-nav-sidebar__dismiss-sidebar-button'; const toggleSidebarButtonSelector = '.wpcom-block-editor-nav-sidebar-toggle-sidebar-button__button'; async toggleNavigationSidebar() { return await driverHelper.clickWhenClickable( this.driver, By.css( toggleSidebarButtonSelector + ', ' + dismissSidebarButtonSelector ); ); }

...assuming those selectors are correct 😉

WunderBart · 2021-06-22T15:41:27Z

test/e2e/lib/gutenberg/tracking/general-tests.js

+		const editor = await EditorComponent.Expect( this.driver, gutenbergEditorType );
+
+		await editor.toggleNavigationSidebar();
+		await editor.toggleNavigationSidebar();


Why a double call here?

To make sure we don't leave it open for the next test. And it's also to make sure the event isn't triggered for both open and close action.

Ah, gotcha. A short explanation comment would be nice then! 🙏

WunderBart · 2021-06-22T15:42:59Z

test/e2e/lib/gutenberg/tracking/general-tests.js

+		assert.strictEqual(
+			editorCloseClickNotFired,
+			true,
+			'"wpcom_block_editor_close_click" editor tracking event fired'
+		);


IMO this would be a cleaner way for this kind of assertions:

Suggested change

assert.strictEqual(

editorCloseClickNotFired,

true,

'"wpcom_block_editor_close_click" editor tracking event fired'

);

assert( editorCloseClickNotFired, '"wpcom_block_editor_close_click" editor tracking event fired' );

WunderBart · 2021-06-22T15:44:53Z

test/e2e/lib/gutenberg/tracking/general-tests.js

+			eventsStack.filter( ( [ eventName ] ) => eventName === 'wpcom_block_editor_close_click' )
+				.length === 0;


Suggested change

eventsStack.filter( ( [ eventName ] ) => eventName === 'wpcom_block_editor_close_click' )

.length === 0;

! eventsStack.some( ( [ eventName ] ) => eventName === 'wpcom_block_editor_close_click' );

matticbot · 2021-06-23T10:40:29Z

This PR modifies the release build for editing-toolkit

To test your changes on WordPress.com, run install-plugin.sh editing-toolkit update/wpcom-nav-sidebar-tracking on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-mMA-p2

david-szabo97 · 2021-06-23T12:29:29Z

Thanks @WunderBart ! Made some changes according to your suggestions.

WunderBart · 2021-06-24T12:47:57Z

test/e2e/lib/components/site-editor-component.js

+	/**
+	 * Alias for `toggleNavigationSidebar`.
+	 * We need to have the same function names in both
+	 * this and gutenberg editor component to
+	 * make sure general tests are working as expected.
+	 */


tip: If you're using VSCode, there's a neat extension that wraps the comments automatically for you: https://marketplace.visualstudio.com/items?itemName=stkb.rewrap. I've set it up to wrap at 100th column as it's what we've set for Prettier.

I just installed this per your suggestion and its great!

WunderBart · 2021-06-24T12:52:35Z

test/e2e/lib/gutenberg/tracking/general-tests.js

@@ -158,6 +158,22 @@ export function createGeneralTests( { it, editorType, postType } ) {
 		);
 	} );

+	it( `W button should not trigger the "wpcom_block_editor_close_click" event when it's connected to a sidebar`, async function () {


Could you explain how this test ensures that the W button is not triggering this event? I don't see that button being pressed anywhere... 🤔

Ah, scratch that. I thought is's about a W key, like in WSAD 😅. Anyway, what that W button actually is on the page? 😄

Right! We don't really have an offical name for the button. Either W Button, WP Button or Site Icon Button

Ah, makes sense! Although we did name it in the toggleBlockEditorSidebar as the sidebarButton, no? 😄 I think we can keep the "Block editor sidebar button" name for clarity, as it's what is displayed as the element's title:

matticbot · 2021-06-24T13:59:05Z

This PR modifies the release build for notifications

To test your changes on WordPress.com, run install-plugin.sh notifications update/wpcom-nav-sidebar-tracking on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-elI-p2

Addison-Stavlo

This looks good and works well on my end.

Track sidebar open and editor close propertly

83fdb34

david-szabo97 requested review from kurt213, Copons, ianstewart and Addison-Stavlo June 21, 2021 12:08

david-szabo97 self-assigned this Jun 21, 2021

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 21, 2021

david-szabo97 added 2 commits June 21, 2021 15:40

Only track opens

b613149

Add E2E test

82a7856

david-szabo97 requested review from scinos, WunderBart and a team as code owners June 21, 2021 13:40

david-szabo97 mentioned this pull request Jun 21, 2021

Full Site Editing: Stats (Tracking Issue) #53410

Closed

24 tasks

david-szabo97 mentioned this pull request Jun 21, 2021

Site Editor Tracking: Track browsing sidebar actions #53500

Merged

Addison-Stavlo reviewed Jun 21, 2021

View reviewed changes

Scope down

a9af9b9

david-szabo97 changed the title ~~Block Editor Tracking: Track "sidebar open" and "editor close" properly~~ Block Editor Tracking: Remove "wpcom_block_editor_close_click" from W button Jun 22, 2021

WunderBart reviewed Jun 22, 2021

View reviewed changes

david-szabo97 added 2 commits June 23, 2021 12:24

Simplify toggleNavigationSidebar

3a09e33

Use aria attributes

5397ae1

david-szabo97 added 3 commits June 23, 2021 12:48

Simplify assert

5c66597

Rename toggleNavigationSidebar

e25cec1

Simplify alias

48e036a

WunderBart reviewed Jun 24, 2021

View reviewed changes

Wrap comment

bc3a415

david-szabo97 added 2 commits June 25, 2021 15:38

Rename event

d1f00f7

Add explanation for two function calls

6b6b059

Addison-Stavlo approved these changes Jun 25, 2021

View reviewed changes

WunderBart approved these changes Jun 28, 2021

View reviewed changes

david-szabo97 added 3 commits June 30, 2021 18:08

Merge branch 'trunk' into update/wpcom-nav-sidebar-tracking

9c919f1

Increase wait time

a3916ee

Fix typo

c71161e

david-szabo97 merged commit ca585df into trunk Jun 30, 2021

david-szabo97 deleted the update/wpcom-nav-sidebar-tracking branch June 30, 2021 17:44

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 30, 2021

		eventsStack.filter( ( [ eventName ] ) => eventName === 'wpcom_block_editor_close_click' )
		.length === 0;

	eventsStack.filter( ( [ eventName ] ) => eventName === 'wpcom_block_editor_close_click' )
	.length === 0;
	! eventsStack.some( ( [ eventName ] ) => eventName === 'wpcom_block_editor_close_click' );

Block Editor Tracking: Remove "wpcom_block_editor_close_click" from W button #53864

Block Editor Tracking: Remove "wpcom_block_editor_close_click" from W button #53864

Conversation

david-szabo97 commented Jun 21, 2021 • edited Loading

Changes proposed in this Pull Request

Testing instructions

matticbot commented Jun 21, 2021 • edited Loading

matticbot commented Jun 21, 2021

matticbot commented Jun 21, 2021

Addison-Stavlo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WunderBart Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Addison-Stavlo left a comment

Choose a reason for hiding this comment

Addison-Stavlo Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-szabo97 commented Jun 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WunderBart Jun 22, 2021 • edited Loading

Choose a reason for hiding this comment

WunderBart Jun 22, 2021 • edited Loading

Choose a reason for hiding this comment

matticbot commented Jun 23, 2021

david-szabo97 commented Jun 23, 2021

Choose a reason for hiding this comment

Addison-Stavlo Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matticbot commented Jun 24, 2021

Addison-Stavlo left a comment

Choose a reason for hiding this comment

david-szabo97 commented Jun 21, 2021 •

edited

Loading

matticbot commented Jun 21, 2021 •

edited

Loading

WunderBart Jun 23, 2021 •

edited

Loading

Addison-Stavlo Jun 21, 2021 •

edited

Loading

WunderBart Jun 22, 2021 •

edited

Loading

WunderBart Jun 22, 2021 •

edited

Loading

Addison-Stavlo Jun 24, 2021 •

edited

Loading