diff --git a/app/src/App.css b/app/src/App.css index 8f417f52..783dbecf 100644 --- a/app/src/App.css +++ b/app/src/App.css @@ -67,6 +67,46 @@ html .mantine-Notifications-root[data-position^='top'] { background: rgba(0, 0, 0, 0.12); } +/* + * The tab for the section the user is in. + * + * react-router marks it with `active` and `aria-current` on its own, and always did -- the + * mark reached the DOM and nothing here drew it, so every tab looked identical and clicking + * Videos appeared to leave the bar untouched. + * + * The underline is a pseudo-element in `currentColor`, not a border: `currentColor` follows + * the foreground the bar computed for whatever navbar color the user picked, so this works + * on all twelve of them (see `navColorNames`) and in all four themes without naming a color; + * and a pseudo-element adds no height, where a border would jog the whole row as the mark + * moved between tabs. The darker wash is the hover treatment deepened, so the two read as + * the same idea. + */ +.wrolpi-navbar-link.active { + position: relative; + background: rgba(0, 0, 0, 0.2); +} + +.wrolpi-navbar-link.active::after { + content: ''; + position: absolute; + left: 0; + right: 0; + bottom: 0; + /* rem, not px, so the mark grows with the --ui-scale setting like the text above it. */ + height: 0.1875rem; + background: currentColor; +} + +/* + * The same tabs inside the overflow "More" menu and the mobile hamburger, which are the + * only place most of them appear on a phone. Those render in a portal, outside the bar, + * so they take theme tokens rather than the bar's foreground. + */ +.mantine-Menu-item.active { + background: var(--head); + font-weight: 600; +} + /* * Everything in the bar takes the bar's foreground. * diff --git a/app/src/components/Nav.js b/app/src/components/Nav.js index 3ee35d49..8dfce4f4 100644 --- a/app/src/components/Nav.js +++ b/app/src/components/Nav.js @@ -1,5 +1,5 @@ import React from "react"; -import {Link, NavLink} from "react-router"; +import {Link, NavLink, useLocation} from "react-router"; import {IconMenu2, IconPlugOff, IconTemperature, IconTemperaturePlus} from "@tabler/icons-react"; import {Icon, IconButton, Menu, Tooltip} from "./ui"; import {Media, SettingsContext, StatusContext} from "../contexts/contexts"; @@ -61,9 +61,11 @@ const allLinks = [ {text: 'Playlists', to: '/playlists', key: 'playlists'}, {text: 'Zim', to: '/zim', key: 'zim'}, {text: 'Inventory', to: '/inventory', key: 'inventory'}, - {to: '/more/calculators', text: 'Calculators', key: 'calculators', end: true}, + // No `end` on these two: a tab stays current for the pages beneath it, and `end` would + // drop the mark the moment either grows a sub-page. + {to: '/more/calculators', text: 'Calculators', key: 'calculators'}, {text: 'Flasher', to: '/flasher', key: 'flasher'}, - {to: '/more/statistics', text: 'Statistics', key: 'statistics', end: true}, + {to: '/more/statistics', text: 'Statistics', key: 'statistics'}, ]; function MenuLink({link}) { @@ -130,12 +132,42 @@ const NavDropdownTrigger = React.forwardRef(({text, className, ...props}, ref) = ); NavDropdownTrigger.displayName = 'NavDropdownTrigger'; +/** + * Whether `link` covers `pathname`, matching NavLink's own rule: a link owns the pages + * beneath it unless it asks to match its path exactly. + * + * Duplicated from NavLink deliberately. The question here is asked about a link that is + * NOT rendered -- one hidden inside the overflow menu -- so there is no NavLink to ask, and + * the hooks that would answer it cannot be called in a loop. + */ +export const isLinkActive = (pathname, link) => { + // An off-site link (Help) is never the current section. + if (!link.to || /^[a-z]+:\/\//i.test(link.to)) return false; + const to = link.to.length > 1 ? link.to.replace(/\/+$/, '') : link.to; + if (link.end || to === '/') return pathname === to; + return pathname === to || pathname.startsWith(`${to}/`); +} + function DropdownLinks({link}) { // A labelled dropdown trigger: the desktop "More" overflow, or any nested // link group handed to MenuLink. + const {pathname} = useLocation(); + /* + * Mark the trigger when the current section is one of the links folded inside it. + * + * Without this the bar goes blank the moment the window narrows enough to push the + * current tab into "More" -- the mark exists, but only after the user opens the menu + * to look for it, which is precisely when they least need telling. + * + * The class only, not `aria-current`: the real NavLink inside the menu already carries + * that, and a second "current" on the button that opens the menu would announce the + * same page twice. + */ + const holdsCurrent = (link.links || []).some(child => isLinkActive(pathname, child)); + return - + {link.links.map(l => )} diff --git a/app/src/components/Nav.test.js b/app/src/components/Nav.test.js new file mode 100644 index 00000000..82cb7433 --- /dev/null +++ b/app/src/components/Nav.test.js @@ -0,0 +1,233 @@ +import fs from 'fs'; +import path from 'path'; +import React from 'react'; +import {screen} from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import {render} from '../test-utils'; +import {FileWorkerStatusContext} from '../contexts/FileWorkerStatusContext'; +import {NavBar} from './Nav'; + +/* + * Which navbar tab is marked as the current page. + * + * `NavLink` marks itself with `aria-current="page"` and an `active` class, and both need to + * be right: the class is what the bar styles, and the attribute is what tells a screen + * reader which section the user is in. + * + * The rule these tests encode is "a tab stays current for everything underneath it" -- + * /videos/1234 is still Videos. That is `NavLink`'s default, so the tests exist to catch + * an `end` prop added to a link that has pages beneath it, which silently drops the mark on + * every one of those pages. + */ + +/** + * Report a viewport to fresnel, which renders only the breakpoints whose query matches: a + * mock matching nothing renders no navbar at all, and one matching everything renders the + * mobile and desktop bars at once. + */ +const useViewport = (kind) => { + // Fresnel's breakpoints are mobile 0 / tablet 700 / wideTablet 880 / computer 1024. + const matches = kind === 'desktop' + ? (query) => query.includes('1024') + : (query) => !/\d{3,}px/.test(query) || /\(max-width/.test(query); + window.matchMedia = jest.fn().mockImplementation(query => ({ + matches: matches(query), + media: query, + onchange: null, + addListener: jest.fn(), + removeListener: jest.fn(), + addEventListener: jest.fn(), + removeEventListener: jest.fn(), + dispatchEvent: jest.fn(), + })); +}; + +/** + * Give jsdom a bar of `barWidth` holding tabs of 60px each. + * + * Every width is 0 otherwise, so useOverflowNav concludes that nothing fits and sweeps all + * eleven tabs into the closed "More" menu -- the bar renders no tabs at all and every + * assertion about them passes vacuously. A narrow bar is how the overflow path is reached + * on purpose. + */ +const useBarWidth = (barWidth) => { + rectSpy = rectSpy || null; + Object.defineProperty(HTMLElement.prototype, 'clientWidth', {configurable: true, value: barWidth}); + rectSpy = jest.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockImplementation(() => ({ + width: 60, height: 40, top: 0, left: 0, right: 60, bottom: 40, x: 0, y: 0, + toJSON: () => ({}), + })); +}; + +/** + * Hand back the real `getBoundingClientRect`. + * + * The stub above is for the overflow measurement, which the mobile bar never runs -- and + * floating-ui, which positions the menu's portal, walks up to the document element and + * crashes the worker on a rect that is not a real DOMRect. + */ +const useRealRects = () => { + if (rectSpy) { + rectSpy.mockRestore(); + rectSpy = null; + } +}; + +let rectSpy = null; + +beforeEach(() => { + rectSpy = null; + useViewport('desktop'); + useBarWidth(2000); +}); + +// NavBar reads the file-worker status for its indicator icons. That context lives outside +// contexts/contexts.js, so the spec supplies it -- see the note at the top of test-utils. +const fileWorkerStatus = {status: null, error: null, refresh: jest.fn(), setFastPolling: jest.fn()}; + +const renderNav = (route) => render(, { + route, + withMedia: true, + contexts: [[FileWorkerStatusContext, fileWorkerStatus]], +}); + +/** The tab marked as the current page, by its text. */ +const currentTab = () => { + const current = document.querySelectorAll('[aria-current="page"]'); + return [...current].map(el => el.textContent.trim()); +}; + +describe('NavBar current tab', () => { + it.each([ + ['/videos', 'Videos'], + ['/archives', 'Archive'], + ['/docs', 'Docs'], + ['/map', 'Map'], + ['/files', 'Files'], + ['/playlists', 'Playlists'], + ['/zim', 'Zim'], + ['/inventory', 'Inventory'], + ['/flasher', 'Flasher'], + ['/more/calculators', 'Calculators'], + ['/more/statistics', 'Statistics'], + ])('marks %s as %s', (route, text) => { + renderNav(route); + + // Exactly, not `toContain`: a regression leaving Home permanently marked alongside + // the real tab satisfies "contains" on every row here. + expect(currentTab()).toEqual([text]); + }); + + it.each([ + ['/videos/1234', 'Videos'], + ['/videos/channel/5/video', 'Videos'], + ['/archives/domain/example.com', 'Archive'], + ['/files/some/deep/directory', 'Files'], + ['/inventory/1/items', 'Inventory'], + ['/more/calculators/electrical', 'Calculators'], + ])('keeps the tab marked on a page underneath it: %s stays %s', (route, text) => { + /* + * Not where the reported bug was -- Videos never carried `end`, so opening a video + * always marked it in the DOM; what was missing was the CSS that draws the mark. + * These cases guard the other half: that nobody adds `end` to a link with pages + * beneath it, which is what Calculators and Statistics used to have. + */ + renderNav(route); + + expect(currentTab()).toEqual([text]); + }); + + it('marks the home link only at the root', () => { + // `to='/'` matches every path unless it is told not to, which would leave the home + // link permanently marked and two tabs claiming to be the current page at once. + renderNav('/videos'); + + const home = screen.getByRole('link', {name: /WROLPi Home Icon|WROLPi/i}); + expect(home).not.toHaveAttribute('aria-current'); + }); + + it('marks exactly one tab at a time', () => { + renderNav('/videos/1234'); + + expect(currentTab()).toHaveLength(1); + }); + + it('marks the home link at the root', () => { + renderNav('/'); + + expect(currentTab()).toHaveLength(1); + }); + + it('gives the current tab the class the bar styles', () => { + renderNav('/videos'); + + expect(document.querySelector('[aria-current="page"]')).toHaveClass('active'); + }); + + it('marks the current item inside the mobile menu', async () => { + // On a phone the bar is a hamburger, so these menu items are the only place the + // mark can appear -- and `.mantine-Menu-item.active` is the only rule that draws it. + useViewport('mobile'); + useRealRects(); + renderNav('/videos'); + + await userEvent.click(screen.getByRole('button', {name: 'Menu'})); + + const videos = await screen.findByRole('menuitem', {name: 'Videos'}); + expect(videos).toHaveAttribute('aria-current', 'page'); + expect(videos).toHaveClass('active'); + }); + + it('marks the More button when the current section is folded inside it', () => { + /* + * A narrow bar pushes tabs into the overflow menu. Nothing in the bar was marked + * then: the trigger is a plain button, not a NavLink, so the bar went blank exactly + * when the window got too small to show the tab -- and the user had to open the menu + * to find out where they were. + */ + useBarWidth(200); + renderNav('/videos'); + + const more = screen.getByRole('button', {name: /More/}); + expect(more).toHaveClass('active'); + }); + + it('leaves the More button unmarked when the current section is not inside it', () => { + useBarWidth(200); + // Nothing in the bar's link list covers /admin/settings; Admin sits outside it. + renderNav('/admin/settings'); + + expect(screen.getByRole('button', {name: /More/})).not.toHaveClass('active'); + }); + + it('styles that class, so the current tab actually looks current', () => { + /* + * The reported bug in full: react-router was already marking the tab, and the mark + * was already reaching the DOM -- nothing styled it, so every tab looked identical + * and clicking Videos appeared to do nothing to the bar. + * + * jsdom applies no stylesheet, so the rule itself is the thing to guard, and it is + * guarded by its declarations rather than by its selector: a check for the selector + * alone still passes with the body emptied, or with the underline deleted -- and the + * underline is the half that survives on a dark bar, where the wash barely reads. + */ + const css = fs.readFileSync(path.join(__dirname, '..', 'App.css'), 'utf8'); + const ruleBody = (selector) => { + const match = css.match(new RegExp(`${selector}\\s*{([^}]*)}`)); + return match ? match[1] : ''; + }; + + // The wash. + expect(ruleBody('\\.wrolpi-navbar-link\\.active')).toMatch(/background:/); + // The underline: drawn, in the bar's own foreground, with a height to draw. + const underline = ruleBody('\\.wrolpi-navbar-link\\.active::after'); + expect(underline).toMatch(/content:/); + expect(underline).toMatch(/background:\s*currentColor/); + // Parsed, not pattern-matched: `(?!0)` would have called 0.1875rem zero. + const height = underline.match(/height:\s*([\d.]+)\s*(?:rem|em|px)/); + expect(height).not.toBeNull(); + expect(parseFloat(height[1])).toBeGreaterThan(0); + // The menu items, which are the whole of the mark on a phone. + expect(ruleBody('\\.mantine-Menu-item\\.active')).toMatch(/background:|font-weight:/); + }); +});