diff --git a/ui/src/album/AlbumList.jsx b/ui/src/album/AlbumList.jsx index 5108bfaa1..cd73fa0c3 100644 --- a/ui/src/album/AlbumList.jsx +++ b/ui/src/album/AlbumList.jsx @@ -1,3 +1,4 @@ +import { cloneElement } from 'react' import { useSelector } from 'react-redux' import { Redirect, useLocation } from 'react-router-dom' import { @@ -23,6 +24,7 @@ import { Title, useAlbumsPerPage, useResourceRefresh, + useScrollRestoration, useSetToggleableFields, } from '../common' import AlbumListActions from './AlbumListActions' @@ -39,6 +41,13 @@ import ExpandInfoDialog from '../dialogs/ExpandInfoDialog' import { humanize } from 'inflection' import { makeStyles } from '@material-ui/core/styles' +// Waits for rows: restoring into an unrendered list leaves the page too short to hold the offset. +const ScrollRestorer = ({ children, ...rest }) => { + const { loaded, total } = useListContext() + useScrollRestoration(loaded && total > 0) + return cloneElement(children, rest) +} + const useStyles = makeStyles({ chip: { margin: 0, @@ -255,11 +264,13 @@ const AlbumList = (props) => { } title={} > - {albumView.grid ? ( - - ) : ( - - )} + + {albumView.grid ? ( + + ) : ( + + )} + } /> diff --git a/ui/src/album/AlbumShow.jsx b/ui/src/album/AlbumShow.jsx index c9e944999..86b5ab1bc 100644 --- a/ui/src/album/AlbumShow.jsx +++ b/ui/src/album/AlbumShow.jsx @@ -10,7 +10,7 @@ import { makeStyles } from '@material-ui/core/styles' import AlbumSongs from './AlbumSongs' import AlbumDetails from './AlbumDetails' import AlbumActions from './AlbumActions' -import { useResourceRefresh, Title } from '../common' +import { useResourceRefresh, useScrollRestoration, Title } from '../common' const useStyles = makeStyles( (theme) => ({ @@ -28,6 +28,7 @@ const AlbumShowLayout = (props) => { const { record } = context const classes = useStyles() useResourceRefresh('album', 'song') + useScrollRestoration(!!record?.id) return ( <> diff --git a/ui/src/artist/ArtistShow.jsx b/ui/src/artist/ArtistShow.jsx index 955a565d6..eef2989d5 100644 --- a/ui/src/artist/ArtistShow.jsx +++ b/ui/src/artist/ArtistShow.jsx @@ -13,7 +13,12 @@ import subsonic from '../subsonic' import AlbumGridView from '../album/AlbumGridView' import MobileArtistDetails from './MobileArtistDetails' import DesktopArtistDetails from './DesktopArtistDetails' -import { useAlbumsPerPage, useResourceRefresh, Title } from '../common/index.js' +import { + useAlbumsPerPage, + useResourceRefresh, + useScrollRestoration, + Title, +} from '../common/index.js' import ArtistActions from './ArtistActions' import { makeStyles } from '@material-ui/core' @@ -85,6 +90,7 @@ const ArtistShowLayout = (props) => { const [, perPageOptions] = useAlbumsPerPage(width) const classes = useStyles() useResourceRefresh('artist', 'album') + useScrollRestoration(!!record?.id) const maxPerPage = 90 let perPage = 0 diff --git a/ui/src/common/ArtistLinkField.jsx b/ui/src/common/ArtistLinkField.jsx index d41b47b06..2a70ded7c 100644 --- a/ui/src/common/ArtistLinkField.jsx +++ b/ui/src/common/ArtistLinkField.jsx @@ -7,7 +7,8 @@ import { intersperse } from '../utils/index.js' import { useDispatch } from 'react-redux' import { closeExtendedInfoDialog } from '../actions/dialogs.js' -const ALink = withWidth()((props) => { +// noSSR: withWidth otherwise renders null until mounted, so the artist line pops in and grows the row. +const ALink = withWidth({ noSSR: true })((props) => { const { artist, width, ...rest } = props const artistLink = useGetHandleArtistClick(width) const dispatch = useDispatch() diff --git a/ui/src/common/index.js b/ui/src/common/index.js index ac8d7f62c..c88ea3e9c 100644 --- a/ui/src/common/index.js +++ b/ui/src/common/index.js @@ -28,6 +28,7 @@ export * from './useAlbumsPerPage' export * from './useGetHandleArtistClick' export * from './useInterval' export * from './useResourceRefresh' +export * from './useScrollRestoration' export * from './useRefreshOnEvents' export * from './useToggleLove' export * from './useTraceUpdate' diff --git a/ui/src/common/useScrollRestoration.jsx b/ui/src/common/useScrollRestoration.jsx new file mode 100644 index 000000000..94e5cb2fd --- /dev/null +++ b/ui/src/common/useScrollRestoration.jsx @@ -0,0 +1,64 @@ +import { useEffect, useLayoutEffect, useRef } from 'react' +import { useHistory, useLocation } from 'react-router-dom' + +// Keyed on the route because hash history assigns no location.key, so every page would +// otherwise share one slot and overwrite the offset we came back for. +const positions = new Map() +const MAX_ENTRIES = 50 +const MAX_FRAMES = 60 + +export const useScrollRestoration = (ready = true) => { + const { pathname, search } = useLocation() + const history = useHistory() + const key = pathname + search + const handled = useRef(null) + const latest = useRef(0) + + useEffect(() => { + latest.current = window.scrollY + const track = () => { + latest.current = window.scrollY + } + window.addEventListener('scroll', track, { passive: true }) + return () => window.removeEventListener('scroll', track) + }, [key]) + + // Layout cleanup, so the offset is banked while this page is torn down. A passive one runs + // after the incoming page tops itself, and this page's listener records that 0 first. + useLayoutEffect( + () => () => { + positions.delete(key) + positions.set(key, latest.current) + if (positions.size > MAX_ENTRIES) { + positions.delete(positions.keys().next().value) + } + }, + [key], + ) + + // Layout effect: a passive one runs after paint, so the list would be painted at the old + // offset first and visibly jump. + useLayoutEffect(() => { + if (!ready || handled.current === key) return + handled.current = key + const saved = positions.get(key) + const top = history.action === 'POP' && saved !== undefined ? saved : 0 + window.scrollTo({ top }) + if (!top || Math.round(window.scrollY) === top) return + + // The page can still be filling in, and until it is tall enough the browser clamps us to + // the top. Keep asking until the offset sticks, yielding if anything else moves us. + let frame + let frames = 0 + let landed = Math.round(window.scrollY) + const retry = () => { + const y = Math.round(window.scrollY) + if (y === top || y !== landed || ++frames > MAX_FRAMES) return + window.scrollTo({ top }) + landed = Math.round(window.scrollY) + frame = requestAnimationFrame(retry) + } + frame = requestAnimationFrame(retry) + return () => cancelAnimationFrame(frame) + }, [ready, key, history.action]) +} diff --git a/ui/src/common/useScrollRestoration.test.jsx b/ui/src/common/useScrollRestoration.test.jsx new file mode 100644 index 000000000..7ca67a724 --- /dev/null +++ b/ui/src/common/useScrollRestoration.test.jsx @@ -0,0 +1,222 @@ +import { renderHook, act } from '@testing-library/react-hooks' +import { render } from '@testing-library/react' +import { useLayoutEffect } from 'react' +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' + +const mockLocation = { pathname: '/album', search: '' } +const mockHistory = { action: 'PUSH' } +vi.mock('react-router-dom', () => ({ + useLocation: () => mockLocation, + useHistory: () => mockHistory, +})) + +import { useScrollRestoration } from './useScrollRestoration' + +describe('useScrollRestoration', () => { + beforeEach(() => { + window.scrollTo = vi.fn() + window.scrollY = 0 + mockLocation.pathname = '/album' + mockLocation.search = '' + mockHistory.action = 'PUSH' + }) + afterEach(() => { + vi.useRealTimers() + vi.clearAllMocks() + }) + + const scrollTo = (y) => { + window.scrollY = y + act(() => { + window.dispatchEvent(new Event('scroll')) + }) + } + + // Returns to a page whose rows have not arrived, so scrollTo still clamps. The returned setter + // grows the page to the height it reaches once they do. + const returnToUnfilledPage = (saved) => { + vi.useFakeTimers() + let maxScroll = saved + window.scrollTo = vi.fn(({ top }) => { + window.scrollY = Math.min(top, maxScroll) + }) + + const first = renderHook(() => useScrollRestoration()) + scrollTo(saved) + first.unmount() + + maxScroll = 0 + window.scrollY = 0 + mockHistory.action = 'POP' + renderHook(() => useScrollRestoration()) + return (height) => (maxScroll = height) + } + + const advanceFrames = () => act(() => vi.advanceTimersByTime(100)) + + it('starts a pushed route at the top', () => { + renderHook(() => useScrollRestoration()) + expect(window.scrollTo).toHaveBeenCalledWith({ top: 0 }) + }) + + it('restores the saved offset when returning to an entry', () => { + const first = renderHook(() => useScrollRestoration()) + scrollTo(640) + first.unmount() + + mockLocation.pathname = '/album/al-1/show' + const second = renderHook(() => useScrollRestoration()) + expect(window.scrollTo).toHaveBeenLastCalledWith({ top: 0 }) + second.unmount() + + mockLocation.pathname = '/album' + mockHistory.action = 'POP' + renderHook(() => useScrollRestoration()) + expect(window.scrollTo).toHaveBeenLastCalledWith({ top: 640 }) + }) + + it('tops a pushed route even if that key was seen before', () => { + const first = renderHook(() => useScrollRestoration()) + scrollTo(500) + first.unmount() + + mockHistory.action = 'PUSH' + renderHook(() => useScrollRestoration()) + expect(window.scrollTo).toHaveBeenLastCalledWith({ top: 0 }) + }) + + // Restoring before the rows exist leaves the document too short: the browser clamps to the + // top and the offset is lost with no error. + it('waits for readiness before touching the scroll position', () => { + const { rerender } = renderHook( + ({ ready }) => useScrollRestoration(ready), + { + initialProps: { ready: false }, + }, + ) + expect(window.scrollTo).not.toHaveBeenCalled() + + rerender({ ready: true }) + expect(window.scrollTo).toHaveBeenCalledWith({ top: 0 }) + }) + + // Hash history assigns no location.key, so an implementation keyed on it would collapse every + // route into one slot and the detail page would erase the list's offset before we returned. + it('keeps a separate offset per route', () => { + const list = renderHook(() => useScrollRestoration()) + scrollTo(900) + list.unmount() + + mockLocation.pathname = '/album/al-1/show' + const detail = renderHook(() => useScrollRestoration()) + scrollTo(0) + detail.unmount() + + mockLocation.pathname = '/album' + mockHistory.action = 'POP' + renderHook(() => useScrollRestoration()) + expect(window.scrollTo).toHaveBeenLastCalledWith({ top: 900 }) + }) + + // Without the per-instance guard this fires again when readiness flickers, fighting a user who + // has already started scrolling. Nothing else in the suite pins it. + it('does not scroll again when readiness flickers on the same route', () => { + const { rerender } = renderHook( + ({ ready }) => useScrollRestoration(ready), + { + initialProps: { ready: true }, + }, + ) + expect(window.scrollTo).toHaveBeenCalledTimes(1) + + rerender({ ready: false }) + rerender({ ready: true }) + expect(window.scrollTo).toHaveBeenCalledTimes(1) + }) + + it('scrolls once per entry, not on every re-render', () => { + const { rerender } = renderHook(() => useScrollRestoration()) + rerender() + rerender() + expect(window.scrollTo).toHaveBeenCalledTimes(1) + }) + + // The artist page reports ready as soon as its header record is cached, while the albums below + // are still loading. A single scrollTo lands on a viewport-tall document and is clamped to 0. + it('keeps trying until the page is tall enough to hold the offset', () => { + const fillPageTo = returnToUnfilledPage(800) + expect(window.scrollY).toBe(0) + + fillPageTo(1200) + advanceFrames() + expect(window.scrollY).toBe(800) + }) + + it('stops retrying once the user scrolls somewhere else', () => { + const fillPageTo = returnToUnfilledPage(800) + + fillPageTo(1200) + window.scrollY = 300 + advanceFrames() + expect(window.scrollY).toBe(300) + }) + + // A trackpad back-swipe keeps firing wheel momentum through the restore, so treating input as + // the yield signal cancels the gesture's own restore. + it('keeps restoring while wheel momentum fires, as a back-swipe does', () => { + const fillPageTo = returnToUnfilledPage(800) + + act(() => { + for (let i = 0; i < 10; i++) { + window.dispatchEvent( + new WheelEvent('wheel', { deltaX: -30, deltaY: 0 }), + ) + } + }) + fillPageTo(1200) + advanceFrames() + expect(window.scrollY).toBe(800) + }) + + // The incoming page tops itself in its own layout effect, while this page's scroll listener is + // still attached. Banking the offset any later than the teardown records that 0 instead. + it('records the offset before the next page scrolls to the top', () => { + const Outgoing = () => { + useScrollRestoration() + return null + } + const Incoming = () => { + useLayoutEffect(() => { + window.scrollY = 0 + window.dispatchEvent(new Event('scroll')) + }, []) + return null + } + + const { rerender } = render() + scrollTo(1400) + + mockLocation.pathname = '/album/al-1/show' + rerender() + + mockLocation.pathname = '/album' + mockHistory.action = 'POP' + renderHook(() => useScrollRestoration()) + expect(window.scrollTo).toHaveBeenLastCalledWith({ top: 1400 }) + }) + + it('keeps saving after the restore, so leaving again remembers the new offset', () => { + const first = renderHook(() => useScrollRestoration()) + scrollTo(200) + first.unmount() + + mockHistory.action = 'POP' + const second = renderHook(() => useScrollRestoration()) + expect(window.scrollTo).toHaveBeenLastCalledWith({ top: 200 }) + scrollTo(900) + second.unmount() + + renderHook(() => useScrollRestoration()) + expect(window.scrollTo).toHaveBeenLastCalledWith({ top: 900 }) + }) +}) diff --git a/ui/src/themes/useCurrentTheme.js b/ui/src/themes/useCurrentTheme.js index 0d986033d..2b5d13d13 100644 --- a/ui/src/themes/useCurrentTheme.js +++ b/ui/src/themes/useCurrentTheme.js @@ -3,10 +3,14 @@ import useMediaQuery from '@material-ui/core/useMediaQuery' import themes from './index' import { AUTO_THEME_ID } from '../consts' import config from '../config' -import { useEffect } from 'react' +import { useEffect, useMemo } from 'react' const useCurrentTheme = () => { - const prefersLightMode = useMediaQuery('(prefers-color-scheme: light)') + // Runs above the ThemeProvider carrying the prop below, so it needs its own noSsr or the + // auto theme renders dark first and flips. + const prefersLightMode = useMediaQuery('(prefers-color-scheme: light)', { + noSsr: true, + }) const theme = useSelector((state) => { if (state.theme === AUTO_THEME_ID) { return prefersLightMode ? themes.LightTheme : themes.DarkTheme @@ -50,7 +54,15 @@ const useCurrentTheme = () => { document.body.style.backgroundColor = bgColor }, [theme]) - return theme + // We never server-render, so let media queries resolve on the first render: the default + // defers them to an effect, which makes every mount paint the wrong breakpoint and reflow. + return useMemo( + () => ({ + ...theme, + props: { ...theme.props, MuiUseMediaQuery: { noSsr: true } }, + }), + [theme], + ) } export default useCurrentTheme