Pinned plugin sidebars are visible in iPad portrait, but not clickable

[jasmussen]

GIF:

Note how the Yoast pinned plugin sidebar is visible in the toolbar, but not clickable.

Even the item in the ellipsis menu doesn't invoke the sidebar.

As a reminder, pinned items are not visible on mobile (up to the 600px breakpoint), so all plugin sidebars should have a menu item so you can invoke them from there.

At the iPad breakpoint, the pinned item SHOULD be visible and SHOULD function, even if it opens the sidebar as a fullscreen modal interface.

[jasmussen]

Probably not urgent issue, but could be trivial to fix, probably some responsive rules mismatching. @gziolo any idea?

[gziolo]

Probably not urgent issue, but could be trivial to fix, probably some responsive rules mismatching. @gziolo any idea?

No idea, I'll have a look tomorrow.

[chrisvanpatten]

@jasmussen For what it's worth, I'm unable to reproduce this on my 11" iPad Pro, running iOS 13.1.1. Here's a screen recording: https://www.icloud.com/photos/#0HvrehJ0wTvXA-MciusoZNn_g

I'm using the latest versions of Gutenberg and Yoast SEO.

[jasmussen]

It's reproducible on web. It's between two specific breakpoints that lie between Mobile and iPad Pro. So probably between 600px and 782px.

[gziolo]

I can't reproduce it with Gutenberg 6.5. I will try with the latest master soon.

[jasmussen]

I reproduced it in latest master just now:

Definitely between 600px and 782px.

[gziolo]

Yes, I did it as well. It's a regression introduced in 6.5.

[gziolo]

This is how the state change is reflected in 6.5

It seems like somehow this click doesn't state change now in 6.6.

[gziolo]

I figured out what caused regression here:

gutenberg/packages/edit-post/src/components/editor-initialization/listener-hooks.js

Line 72 in a220eba

closeGeneralSidebar();

This forces the sidebar to get closed on small screens immediately after it gets opened.

Related PR: #15444

cc @nerrad

[nerrad]

Hmm ya (guessing, haven't verified) so it looks like there's a match to this condition:

gutenberg/packages/edit-post/src/components/editor-initialization/listener-hooks.js

Lines 70 to 73 in a220eba

	if ( isSmall && sidebarToReOpenOnExpand ) {
	previousOpenedSidebar.current = sidebarToReOpenOnExpand;
	closeGeneralSidebar();
	} else if ( ! isSmall && previousOpenedSidebar.current ) {

Which means that likely isSmall is triggered and there is an active sidebar on sidebarToReOpenOnExpand.

If true, then this is something we need test coverage for as well. If nobody picks this up today I'll get to it later today or tomorrow.

[gziolo]

I'm on it. I will let you know when I finished the day.

[nerrad]

Hmm... I don't think the editor initialization work was new to GB 6.6 ...

[gziolo]

It probably needs to be further optimized but it solves the issue.

diff --git a/packages/edit-post/src/components/editor-initialization/listener-hooks.js b/packages/edit-post/src/components/editor-initialization/listener-hooks.js
index 56d776127..51b1b2920 100644
--- a/packages/edit-post/src/components/editor-initialization/listener-hooks.js
+++ b/packages/edit-post/src/components/editor-initialization/listener-hooks.js
@@ -54,27 +54,35 @@ export const useBlockSelectionListener = ( postId ) => {
  * @param {number} postId  The current post id.
  */
 export const useAdjustSidebarListener = ( postId ) => {
-	const { isSmall, sidebarToReOpenOnExpand } = useSelect(
+	const { isSmall, activeGeneralSidebarName } = useSelect(
 		( select ) => ( {
 			isSmall: select( 'core/viewport' ).isViewportMatch( '< medium' ),
-			sidebarToReOpenOnExpand: select( STORE_KEY ).getActiveGeneralSidebarName(),
+			activeGeneralSidebarName: select( STORE_KEY ).getActiveGeneralSidebarName(),
 		} ),
 		[ postId ]
 	);
 
 	const { openGeneralSidebar, closeGeneralSidebar } = useDispatch( STORE_KEY );
 
-	const previousOpenedSidebar = useRef( '' );
+	const previousIsSmall = useRef( isSmall );
+	const sidebarToReOpenOnExpand = useRef( null );
 
 	useEffect( () => {
-		if ( isSmall && sidebarToReOpenOnExpand ) {
-			previousOpenedSidebar.current = sidebarToReOpenOnExpand;
-			closeGeneralSidebar();
-		} else if ( ! isSmall && previousOpenedSidebar.current ) {
-			openGeneralSidebar( previousOpenedSidebar.current );
-			previousOpenedSidebar.current = '';
+		if ( previousIsSmall.current === isSmall ) {
+			return;
+		}
+		previousIsSmall.current = isSmall;
+
+		if ( isSmall ) {
+			sidebarToReOpenOnExpand.current = activeGeneralSidebarName;
+			if ( activeGeneralSidebarName ) {
+				closeGeneralSidebar();
+			}
+		} else if ( sidebarToReOpenOnExpand.current && ! activeGeneralSidebarName ) {
+			openGeneralSidebar( sidebarToReOpenOnExpand.current );
+			sidebarToReOpenOnExpand.current = null;
 		}
-	}, [ isSmall, sidebarToReOpenOnExpand ] );
+	}, [ isSmall, activeGeneralSidebarName ] );
 };
 
 /**

The main difference is that we only trigger the logic when isSmall has changed.

[gziolo]

PR opened, let's see what Travis thinks. In the meantime, I will work on the e2e test to cover it.

[nerrad]

Yes, I did it as well. It's a regression introduced in 6.5.

The Editor Initialization work was released as a part of Gutenberg 6.2, so while it looks like you've found a fix that can be implemented in the listener-hooks, I'm wary that we're masking another regression somewhere.

[gziolo]

Yes, I did it as well. It's a regression introduced in 6.5.

The Editor Initialization work was released as a part of Gutenberg 6.2, so while it looks like you've found a fix that can be implemented in the listener-hooks, I'm wary that we're masking another regression somewhere.

It worked properly with WordPress 5.2, so it's indeed a regression introduced in the earlier version of the plugin. I don't know what's the proper fix, to be honest. We can use only the existing e2e tests to find out.

[nerrad]

ahh, so you're not specifically saying it was introduced in Gutenberg 6.5, only that it surfaced in WordPress 5.3 (which includes Gutenberg 6.5). Sorry, I misunderstood :)