Fix unified-sidebar scrolling #46689

cpapazoglou · 2020-10-22T17:14:03Z

After a lot of try & fail, I eventually:

used vanilla JS no without passing styles through react to the sidebar component. This way we avoid rerendering the component while scrolling.
had to manipulate the secondary area and not the sidebar itself as it is encapsulated over there

Changes proposed in this Pull Request

Adds ...props pass through to Sidebar component.
Adds window.addEventListener( 'scroll') in MySitesSidebarUnified component so that we reposition (scroll the sidebar) like wp-admin behaves.

Testing instructions

load calypso with ?flags=nav-unification
test that the sidebar behaves as in wp-admin, especially when the window height is shorter than the sidebar
test that my sites, reader, me work as expected
test the mobile view (sidebar full width)

Relates #46643

matticbot · 2020-10-22T17:14:05Z

Test live: https://calypso.live/?branch=fix/unified-sidebar-scrolling

matticbot · 2020-10-22T17:14:48Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~461 bytes added 📈 [gzipped])

name        parsed_size           gzip_size
entry-main      +1422 B  (+0.1%)     +461 B  (+0.1%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~83 bytes added 📈 [gzipped])

name                   parsed_size           gzip_size
woocommerce                  +41 B  (+0.0%)       +9 B  (+0.0%)
site-blocks                  +41 B  (+0.0%)       +9 B  (+0.0%)
security                     +41 B  (+0.0%)       +9 B  (+0.0%)
purchases                    +41 B  (+0.0%)       +4 B  (+0.0%)
privacy                      +41 B  (+0.0%)       +9 B  (+0.0%)
notification-settings        +41 B  (+0.0%)       +9 B  (+0.0%)
me                           +41 B  (+0.0%)       +9 B  (+0.0%)
help                         +41 B  (+0.0%)       +9 B  (+0.0%)
happychat                    +41 B  (+0.0%)       +9 B  (+0.0%)
devdocs                      +41 B  (+0.0%)      -11 B  (-0.0%)
account-close                +41 B  (+0.0%)       +9 B  (+0.0%)
account                      +41 B  (+0.0%)       +9 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~52 bytes added 📈 [gzipped])

name                                           parsed_size           gzip_size
async-load-calypso-reader-sidebar                    +41 B  (+0.1%)       +6 B  (+0.0%)
async-load-calypso-my-sites-sidebar-unified          +41 B  (+0.1%)       +8 B  (+0.1%)
async-load-calypso-my-sites-sidebar                  +41 B  (+0.0%)      +19 B  (+0.0%)
async-load-calypso-components-jetpack-sidebar        +41 B  (+0.2%)      +19 B  (+0.3%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

mreishus · 2020-10-27T22:23:42Z

I don't think the scroll handler is being cleaned up properly.

To test, apply diff:

diff --git a/client/my-sites/sidebar-unified/index.jsx b/client/my-sites/sidebar-unified/index.jsx
index 6763389a39..6296943c20 100644
--- a/client/my-sites/sidebar-unified/index.jsx
+++ b/client/my-sites/sidebar-unified/index.jsx
@@ -39,8 +39,10 @@ export const MySitesSidebarUnified = ( { path } ) => {
        const sidebarIsCollapsed = useSelector( getSidebarIsCollapsed );
 
        useEffect( () => {
+               console.log( 'Registered event listener' );
                window.addEventListener( 'scroll', () => handleScroll() );
                return () => {
+                       console.log( 'Cleared event listener' );
                        window.removeEventListener( 'scroll', () => handleScroll() );
                };
        } );
diff --git a/client/my-sites/sidebar-unified/utils.jsx b/client/my-sites/sidebar-unified/utils.jsx
index 22c82acb7b..bdf43d71f8 100644
--- a/client/my-sites/sidebar-unified/utils.jsx
+++ b/client/my-sites/sidebar-unified/utils.jsx
@@ -6,6 +6,7 @@ let pinnedSidebarBottom = false;
 let ticking = false; // Used for Scroll event throttling.
 
 export const handleScroll = () => {
+       console.log( 'handlescroll' );
        const windowHeight = window?.innerHeight;
        const secondaryElHeight = secondaryEl?.scrollHeight;
        const masterbarHeight = document.getElementById( 'header' ).getBoundingClientRect().height;

Visit http://calypso.localhost:3000/home/testsite5mmr.wordpress.com?flags=nav-unification
Open console, see registered, cleared, registered, cleared, registered
Click "Switch Site", "All my Sites", see "Select a site" with no sidebar
See 'Cleared event listener' in console
Scroll mouse wheel
Expect to see: No handlescroll messages Actually see: Handlescroll messages in console

I think because A and B are different anonymous functions, it can't match them up:

	useEffect( () => {
		window.addEventListener( 'scroll', () => handleScroll() ); // A
		return () => {
			window.removeEventListener( 'scroll', () => handleScroll() ); // B
		};
	} );

I'm about to head out, so I haven't tested it, but I think const x = useCallback(() => handleScroll()); then using x in both addEventListener and removeEventListener should fix.

frontdevde · 2020-10-28T10:40:30Z

From a purely functional perspective this seems to be working fine for me locally until I interact with the My Sites switcher. When scrolling while the site list is showing, it starts detaching the sidebar. Sent you a screen-recording via DM.

cpapazoglou · 2020-10-28T17:45:28Z

I don't think the scroll handler is being cleaned up properly.

To test, apply diff:

diff --git a/client/my-sites/sidebar-unified/index.jsx b/client/my-sites/sidebar-unified/index.jsx
index 6763389a39..6296943c20 100644
--- a/client/my-sites/sidebar-unified/index.jsx
+++ b/client/my-sites/sidebar-unified/index.jsx
@@ -39,8 +39,10 @@ export const MySitesSidebarUnified = ( { path } ) => {
        const sidebarIsCollapsed = useSelector( getSidebarIsCollapsed );
 
        useEffect( () => {
+               console.log( 'Registered event listener' );
                window.addEventListener( 'scroll', () => handleScroll() );
                return () => {
+                       console.log( 'Cleared event listener' );
                        window.removeEventListener( 'scroll', () => handleScroll() );
                };
        } );
diff --git a/client/my-sites/sidebar-unified/utils.jsx b/client/my-sites/sidebar-unified/utils.jsx
index 22c82acb7b..bdf43d71f8 100644
--- a/client/my-sites/sidebar-unified/utils.jsx
+++ b/client/my-sites/sidebar-unified/utils.jsx
@@ -6,6 +6,7 @@ let pinnedSidebarBottom = false;
 let ticking = false; // Used for Scroll event throttling.
 
 export const handleScroll = () => {
+       console.log( 'handlescroll' );
        const windowHeight = window?.innerHeight;
        const secondaryElHeight = secondaryEl?.scrollHeight;
        const masterbarHeight = document.getElementById( 'header' ).getBoundingClientRect().height;

Visit http://calypso.localhost:3000/home/testsite5mmr.wordpress.com?flags=nav-unification
Open console, see registered, cleared, registered, cleared, registered
Click "Switch Site", "All my Sites", see "Select a site" with no sidebar
See 'Cleared event listener' in console
Scroll mouse wheel
Expect to see: No handlescroll messages Actually see: Handlescroll messages in console

I think because A and B are different anonymous functions, it can't match them up:

	useEffect( () => {
		window.addEventListener( 'scroll', () => handleScroll() ); // A
		return () => {
			window.removeEventListener( 'scroll', () => handleScroll() ); // B
		};
	} );

I'm about to head out, so I haven't tested it, but I think const x = useCallback(() => handleScroll()); then using x in both addEventListener and removeEventListener should fix.

Great finding @mreishus , thanks! Since the callback has no dependencies, declaring
const scrollCallback = () => handleScroll(); outside of the component and using scrollCallback in the event listener is enough bccd0ff

cpapazoglou · 2020-10-28T18:23:01Z

From a purely functional perspective this seems to be working fine for me locally until I interact with the My Sites switcher. When scrolling while the site list is showing, it starts detaching the sidebar. Sent you a screen-recording via DM.

Thanks @frontdevde for uncovering this! Fixed here 8a74b4c

cpapazoglou · 2020-10-28T19:16:28Z

Since the changes are regarding the secondary and not the sidebar, I moved the code to wp-calypso/client/layout/index.jsx and also rebased.

This change also helps reader and me behave the same way.

obenland · 2020-10-28T20:11:27Z

client/layout/index.jsx

@@ -78,6 +80,15 @@ class Layout extends Component {
 					.classList.add( `is-${ this.props.colorSchemePreference }` );
 			}
 		}
+		if ( config.isEnabled( 'nav-unification' ) ) {
+			window.addEventListener( 'scroll', scrollCallback );


Could this be simplified to the following?

Suggested change

window.addEventListener( 'scroll', scrollCallback );

window.addEventListener( 'scroll', handleScroll );

This was done so that we can effectively clear the event listener as discussed here #46689 (comment). I have already tried using the exported function handleScroll directly in vain. I may be doing something wrong though..

client/layout/utils.ts

frontdevde · 2020-10-29T10:14:05Z

I'm loving the direction we're going in here. This already feels way better than current Calypso. Scrolling the sidebar by scrolling the page is so much more intuitive than having to scroll inside the sidebar. 👏

Have we tested this on an iPad (in landscape and switching between orientations)?

One thing I noticed is that it's still possible to recreate a detached state when using the dev tools. We might need to factor in resize in this as well.

frontdevde · 2020-10-29T10:19:08Z

Suggestion: Let's remove the Fixes #46643 keyword from the description (or create a separate issue for the second part of the issue).

This PR fixes the first part of the issue, which is great! We should definitely limit the scope of this PR to that issue. That said, we'll still need to address the my site switcher overflow being visible during the transition when a site is selected. This will probably involve dynamically hiding the overflow for the duration of the transition.

cpapazoglou · 2020-10-29T12:08:41Z

I have added the resize event which hopefully will fix any problems on orientation change. Lets merge it so I can test it on a real device easier.

client/layout/utils.ts

frontdevde

After some more testing I can confirm that the resize is now handled well and the previously reported issue is gone.

Let's remove the rouge console.log and then I'd be fine with merging it so we can a) get it in in time for demo and b) you can properly test this on the iPad. Let's keep iterating! 👍

cpapazoglou changed the title ~~fix/unified-sidebar-scrolling~~ Fix unified-sidebar scrolling Oct 22, 2020

cpapazoglou requested a review from frontdevde October 26, 2020 23:59

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

cpapazoglou added the [Feature] Calypso & wp-admin Navigation All navigation in Calypso and wp-admin, and the unified transitions between the two. label Oct 26, 2020

cpap added 10 commits October 28, 2020 20:53

Adds forward Ref to the sidebar

c540b37

First iteration of moving sidebar on scroll

7df1f73

Removes forward ref from sidebar

07ff1d3

Uses plain JS for sidebar positioning

da59daa

Removes stale sidebar ref

c80027a

Makes sidebar scroll same as wp-admin

04c5ce4

Removes sidebar id

074f857

Properly clears the callback

c81895f

Fixes sidebar detaching

58f4db5

Transfer code from sidebar to layout

493b60c

cpapazoglou force-pushed the fix/unified-sidebar-scrolling branch from 8a74b4c to 493b60c Compare October 28, 2020 19:14

obenland reviewed Oct 28, 2020

View reviewed changes

client/layout/utils.ts Show resolved Hide resolved

cpapazoglou requested a review from obenland October 29, 2020 09:12

Fixes TS warnings

a2f4db6

Fixes sidebar height / position on resize

c44518d

frontdevde reviewed Oct 29, 2020

View reviewed changes

client/layout/utils.ts Outdated Show resolved Hide resolved

frontdevde approved these changes Oct 29, 2020

View reviewed changes

Removes console log

eb3d162

cpapazoglou merged commit b8233e1 into master Oct 29, 2020

cpapazoglou deleted the fix/unified-sidebar-scrolling branch October 29, 2020 13:16

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 29, 2020

This was referenced Oct 29, 2020

Add React.UIEvent type to handleScroll function #46926

Merged

handleScroll() doesn't run at all if waiting on requestAnimationFrame #46929

Merged

This was referenced Nov 2, 2020

Nav unification - allow sidebar to be scrolled (while maintaining flyout menus) #46643

Closed

Transition from the site-switcher back to the menu animation #47019

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unified-sidebar scrolling #46689

Fix unified-sidebar scrolling #46689

cpapazoglou commented Oct 22, 2020 •

edited

Loading

matticbot commented Oct 22, 2020

matticbot commented Oct 22, 2020 •

edited

Loading

mreishus commented Oct 27, 2020

frontdevde commented Oct 28, 2020

cpapazoglou commented Oct 28, 2020

cpapazoglou commented Oct 28, 2020

cpapazoglou commented Oct 28, 2020

obenland Oct 28, 2020

cpapazoglou Oct 29, 2020

frontdevde commented Oct 29, 2020

frontdevde commented Oct 29, 2020

cpapazoglou commented Oct 29, 2020

frontdevde left a comment

	window.addEventListener( 'scroll', scrollCallback );
	window.addEventListener( 'scroll', handleScroll );

Fix unified-sidebar scrolling #46689

Fix unified-sidebar scrolling #46689

Conversation

cpapazoglou commented Oct 22, 2020 • edited Loading

Changes proposed in this Pull Request

Testing instructions

matticbot commented Oct 22, 2020

matticbot commented Oct 22, 2020 • edited Loading

mreishus commented Oct 27, 2020

frontdevde commented Oct 28, 2020

cpapazoglou commented Oct 28, 2020

cpapazoglou commented Oct 28, 2020

cpapazoglou commented Oct 28, 2020

obenland Oct 28, 2020

Choose a reason for hiding this comment

cpapazoglou Oct 29, 2020

Choose a reason for hiding this comment

frontdevde commented Oct 29, 2020

frontdevde commented Oct 29, 2020

cpapazoglou commented Oct 29, 2020

frontdevde left a comment

Choose a reason for hiding this comment

cpapazoglou commented Oct 22, 2020 •

edited

Loading

matticbot commented Oct 22, 2020 •

edited

Loading