Prevent layout shifts when navigating from links in PSI and other setup notifications on the dashboard

[techanvil]

Feature Description

Following on from the work in https://github.com/google/site-kit-wp/issues/4641, ensure that the lazy-loaded dashboard sections are not loaded while scrolling past them after clicking on the links in the PSI and Idea Hub setup notifications.

This will involve a bit of refactoring of the solution to #4641 in order to make it applicable to the PSI/Idea Hub links in SetupSuccessBannerNotification.

https://github.com/google/site-kit-wp/blob/f584517e6810170f17f47c6ac3477ae676c653cb/assets/js/components/notifications/SetupSuccessBannerNotification.js#L181-L195

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• If a user clicks a link that scrolls them to different content on the page (via an ID/#hash link), they haven't moved away from that content by manually scrolling the page, and a layout shift in the content should not shift the page and move them away from the content.

Implementation Brief

See: https://github.com/google/site-kit-wp/issues/4880#issuecomment-1813040905 for a link to a proof-of-concept branch with a fix.

Test Coverage

QA Brief

Changelog entry

[aaemnnosttv]

@techanvil is this still a problem? The useInViewSelects shouldn't be triggered if the hash doesn't match the widget area's ID or is there a detail here because the jump links don't match a section?

[techanvil]

Hi @aaemnnosttv, this is still an issue, yup, at least it is for PSI as evident clicking on the link in the PSI setup notification. Notice how it navigates to an offset position on the first click of it and the widgets in the Content section are already loaded. Then I scroll up to click on the link again to show the intended behaviour of cleanly navigating to the Speed section.

https://user-images.githubusercontent.com/18395600/191971584-2f29960e-e6aa-4ade-99ab-ceaf23e93995.mp4

Compare this to clicking on the Speed chip, which navigates correctly to the section, while the Content section is not yet loaded:

https://user-images.githubusercontent.com/18395600/191973654-40a6209f-4ba7-41a2-851a-83c7ccd9accf.mp4

This is because clicking on a chip in Navigation invokes this handler, setting up the necessary bits of state - isJumpingTo, ACTIVE_CONTEXT_ID etc., whereas the PSI link is just a regular href link to #speed.

https://github.com/google/site-kit-wp/blob/98bd026ca0f95f9db25536c0a99eeb7bf00b97cc/assets/js/components/DashboardNavigation/Navigation.js#L181-L204

The behaviour for the Idea Hub case is a bit different though. The lazy loading is not an issue in practice here because the Traffic section preceding Content is already loaded. I am not sure if there's any scenario where the lazy loading could be an issue here. However, the link is an href to #googlesitekit-idea-hub-widget which results in a slightly off-looking navigation. Maybe this should be treated as a separate issue. Here's a recording of the Idea Hub scenario:

https://user-images.githubusercontent.com/18395600/191977236-c3edf3ca-3cd1-4b2c-9686-312762ef8aa4.mp4

[aaemnnosttv]

This one could use a refresh since IH is no longer a module. We've also had some changes related to the inView state so, let's confirm this is still an issue for PSI and see about how to address it. Also there are some related issues due to layout shifts which have crept in to some of the widgets like All Traffic which should be addressed as separate issues I think. It might even be worth introducing some shared infra for testing components that the height doesn't change between loading and loaded states.

[tofumatt]

This is still sort of an issue, though worth noting that jump links specifically for PageSpeed aren't very relevant since it's enabled by default now and thus most users won't see that notice:

https://github.com/google/site-kit-wp/assets/90871/65b6b6bb-e463-450f-a633-bacfe9730c43

But in general, scrolling through the dashboard to the bottom of the page where content can shift will move it around. If you click at exactly the right time it will scroll to a slightly "off" place but it was tough to get the timing and it's rarer.

---

That said, there's a variable-height container of pages from Analytics above the PageSpeed widget in that case, so it will always present the possibility of layout shifts.

While we could slightly improve the consistency of loading heights on the page, there's actually little we can do to stop layout shifts.

If it's a priority, I think, the one solution would be to—for any click a user makes to an ID we scroll to on the page like this:

1. store the ID/hash the user has navigated to in the CORE_UI store
2. detect/listen for scroll events/keyboard events/any interaction—if the user interacts with the page at all, clear the data from step 1
3. anytime a useInViewSelect returns new data call a function that re-scrolls the user to the ID in question if it hasn't been removed in step 3

It's kind of heavy-handed and complex but it'd work, I think. It would scale as we add more things, as well. What do you think @aaemnnosttv/@techanvil?

[aaemnnosttv]

It's kind of heavy-handed and complex but it'd work, I think.

@tofumatt the current AC seems rather complicated indeed. It also seems like this would be solved by making the link in the CTA the same action as the dashboard navigation, since that is not affected by this even though the navigation targets are the same, no? 🤔 If so, can we make the issue about updating the way hash changes work so that any that reference a dashboard section work the same way perhaps?

[tofumatt]

making the link in the CTA the same action as the dashboard navigation, since that is not affected by this even though the navigation targets are the same, no?

Do you mean the chip links at the top? They definitely are affected, but I'd guess it might be that they're less affected because they use a smooth scroll.

We could modify all same-page hash links to use a smooth scroll and it would minimise the shifting. It's probably a bit less robust, but much less complicated. I'll write up new ACs and move it to IB 👍🏻

[techanvil]

Hey @tofumatt, it turns out the PSI notification link is already using behaviour: smooth for the scrolling, so these AC need to be revised.

I think that as per the feature description, we need to extract the relevant part of the the Navigation scrolling logic for reuse, and use it in the PSI notification click handler (and subsequently anywhere else that we want to scroll to a context area). I've created a PoC that works OK - it looks a bit complicated at first glance, but it's just lifting code from Navigation into hooks for the most part, with a React context for sharing state (maybe we could avoid this context by using Redux, but I just wanted a quick lift 'n shift of the useState() call).

[tofumatt]

@techanvil I've written up an AC with less implementation details, let me know what you think 🙂

[techanvil]

Thanks @tofumatt. It works in the context of the conversation we've had, but I think it could be a bit ambiguous to a fresh pair of eyes.

It would be good to be a bit clearer that we're only talking about links to context areas on the Dashboard, i.e. the lazy-loaded bits, and there's actually only one specific case at present which is the PSI setup notification. The way it's currently worded suggests we're looking for a solution to avoid layout shifts for any ID link target, which is a different problem to solve - but also one we don't need to at the moment, AFAIK, as I don't think we actually have any internal ID links beyond the PSI one.

It would also be good to make it clear that the intent is for the lazy-loaded content not to load in the areas which are scrolled over, thus preventing a layout shift from occurring.

[ivonac4]

@tofumatt Are you still planning to work on this? If not, could you please unassign yourself so someone else can pick it up? Thank you!