google
Spacing on Unified Dashboard when Tooltip Tour is Active
Bug Description
While testing #3947 I noticed that if you scrolled down the page while the tour was active, there's a large space underneath WordPress footer. We didn't have time to fix this and since it's unlikely that the user would scroll that far down the page, we decided to create a new ticket.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
- The background for React Joyride (
.react-joyride__overlay) should always span the full height/width of the page. - When the browser viewport size changes OR the page content/height changes, React Joyride should update. (Optional, but likely useful if this doesn't happen already.)
- There should not be a large gap at the bottom of the dashboard when React Joyride renders.
Implementation Brief
Test Coverage
QA Brief
Changelog entry
I'm able to partially recreate this issue, but it's quite intermittent:
https://user-images.githubusercontent.com/90871/187314425-830662ea-2363-40eb-9687-771d4ff0dcda.mp4
I think the main issue is that when the page height or viewport changes we aren't appropriately recalculating the background for the tour tooltips.
Not knowing exactly how React Joyride is implemented, It strikes me that a good start would be to make the background span the viewport height/width, and possibly refresh its React component when the page size changes.
Oddly it seems there are two Joyride divs on the page:
But the main issue is .react-joyride__overlay setting the wrong (too large) height for the size of the page content, this can be observed in the dev tools.
@aaemnnosttv @tofumatt I spent some time looking into this and I think the issue is the height of the overlay is calculated before the widgets are rendered, the main culprit being the PSO widget. Alternatively i haven't seen anything in the React Joyride docs to update the height which potentially means we'll have to manually set the height. I know there were talks long time ago about triggering an event when widgets are rendered or maybe only the PSI one, but am not really sure where we are with it or i'am just making things up 😅 I suggest we come back to this ticket after #5776 has been merged and see if that improves things?
Let me know what you think.
@asvinb we are addressing the height shift in the PSI widget in #4878 which should address this before 5776 (although they are related).
If PSI is purely responsible here, then we can probably close this as a duplicate.
@aaemnnosttv I'am pretty sure it's PSI widget the culprit here. I would suggest we keep the ticket open until https://github.com/google/site-kit-wp/issues/4878 is merged, just to make sure.
Hi @asvinb 4878 has now been closed. I will remove the 'blocked upstream' label and assume this is ok to pick up again unless you know otherwise?
@aaemnnosttv I had a look and the issue still persists. Basically since most of the widgets are in a loading state when the feature tour is rendered and the height of body changes once they are loaded, there is this small gap (whitespace) which is displayed. This is due to when the feature tour is rendered, the height of the overlay is calculated at this particular time (for e.g widgets are still loading), and when they are loaded, the height of the overlay is not recalculated. I checked and there's no API to redraw the overlay. The only way i found is to trigger a new resize event window.dispatchEvent( new Event( 'resize' ) );. I spent some time investigating to trigger a debounced version of the resize event once widgets are loaded but i was unable to find a generic way to know when all the widgets are loaded on the page and maybe it's out of scope for this ticket. So how about a simpler solution where we just check when the PSI widget is loaded (since it takes more time to render than all widgets) and when it's loaded, we just trigger the resize event. Let me know what you think.
cc @tofumatt
@asvinb can we not simply force a max-height on the element to prevent it from making the window any larger? 🤔
Hi @aaemnnosttv can anyone else pick this up, given Asvin is now o.o.o for 3 weeks? It's a P2 so not urgent, but might be nice to get across the line to EB if we can.
Despite previous assumptions, the spacing issue is not related to the joyride tour.
Instead the spacing is caused by the last widget having a min-height: 80vw applied.
https://github.com/google/site-kit-wp/blob/6551b8be34d798cc2eeb5b03934e53fd5ec325e1/assets/sass/widgets/_widget-context.scss#L25-L27
In reference to the screencast from @tofumatt, the disapearing spacing is indeed caused by changes to the viewport. But not because of a JS recalulation, but because of a window height change, that in turn changed the value for 80vh
So technically, this is not a bug, but the expected behaviour introduced with #4490
Therefore, we have to decide which behavior is more important to us. Should the white space disappear or should the widget still be at the top when using the scroll navigation?
@aaemnnosttv @FlicHollis what is the expected workflow for such a case?
Thanks @derweili for the update. @wpdarren if we do want to resolve this we would need to review the behaviour introduced in https://github.com/google/site-kit-wp/issues/4490 and make a decision. @aaemnnosttv if you agree we can move this back to AC to review?
@derweili it seems the overlay can be either taller or shorter than the content on the page, as it seems the height is only calculated when initially rendered and then on resize (as @asvinb noted). In my attempts to recreate this, I saw the overlay was actually a little shorter than taller.
You're correct that the min-height style of the last widget context could be playing a role there too and in that case, there would be nothing to change here as things would be working as expected.
In reference to the screencast from @tofumatt, the disapearing spacing is indeed caused by changes to the viewport. But not because of a JS recalulation, but because of a window height change, that in turn changed the value for
80vh
In the still image at the end (0:35) you can see that about half of the added height is from the Joyride overlay because the added height from our CSS ends just above where the WP footer begins, indicated by the "Thank you for creating with WordPress" text. If this gap was entirely from our CSS, the WP footer would be all the way at the bottom, as usual.
This can be reproduced by hiding one of the upper widget contexts after the overlay has been rendered (so long as you don't resize in doing so). It will produce a large gap at the bottom of the screen.
It's also worth noting that the PSI widget no longer has a variable height between loading and loaded states (which wasn't the case when this issue was opened) so there should be even less possibility for the dashboard height to change while a tour is active.
IMO this is quite a minor quirk and probably not worth fixing as the focus of feature tours is not at the very bottom of the page so it's unlikely that users will see this anyways. Unless there is a trivial fix for this I would suggest we close this and potentially revisit it later.