From eaa8b83e54ffb2d3091fcdf5defcc6588bc471ff Mon Sep 17 00:00:00 2001 From: lrnselfreliance <33263708+lrnselfreliance@users.noreply.github.com> Date: Sat, 8 Aug 2026 17:10:53 -0600 Subject: [PATCH] ui: the navbar shows which section you are in (#649) * ui: the navbar shows which section you are in Clicking Videos opened the videos page and left the bar looking untouched. react-router was doing its part the whole time -- the tab carried `active` and `aria-current="page"` on every one of those pages. Nothing in the stylesheet drew that class, so all eleven tabs rendered identically and the mark was visible only to a screen reader. The current tab now takes a deepened version of the hover wash plus an underline. The underline is a pseudo-element in `currentColor` rather than a border, for two reasons: `currentColor` is the foreground the bar already computed for whichever of the fourteen navbar colors the user picked, so one rule covers every color in every theme without naming one; and a pseudo-element adds no height, where a border would jog the whole row as the mark moved between tabs. The overflow "More" menu and the mobile hamburger mark their copy of the tab too -- on a phone that menu is where most tabs live. Calculators and Statistics also lose `end: true`. A tab is meant to stay current for the pages beneath it, and `end` restricts the match to the exact path, so either one would have dropped its mark the moment it grew a sub-page. The other nine links never had it, which is why /videos/1234 already kept Videos marked. The test renders the real bar, which needed two pieces of browser jsdom does not have: fresnel renders no navbar at all unless a breakpoint query matches, and every width is 0, which convinces the overflow logic that nothing fits and sweeps all eleven tabs into the closed "More" menu -- where every assertion about them would have passed vacuously. Both are stubbed, and the styling itself is asserted against the stylesheet, since jsdom applies none. * review: the mark follows the tab into the overflow menu Six review points, no bugs among them. The substantive one: when the window narrows enough to fold the current tab into "More", the bar went blank again -- the trigger is a plain button, not a NavLink, so nothing was marked until the user opened the menu to look, which is exactly when they least need telling. "More" now takes the mark when it holds the current section. That needs the matching rule outside a NavLink, where the hooks that answer it cannot be called in a loop, so `isLinkActive` states it once: a link owns the pages beneath it unless it asks for an exact match. The trigger gets the class only -- the real NavLink inside the menu already carries `aria-current`, and a second one on the button that opens the menu announces the same page twice. The other five, all fair: * The mobile hamburger and the overflow menu had no test at all, so `.mantine-Menu-item.active` -- the whole of the mark on a phone -- was never exercised. Both now are, which meant splitting the viewport and the bar width out of the fixture so a test can ask for a narrow bar or a phone. The mobile case also needs the real getBoundingClientRect back: floating-ui positions the menu's portal and crashes the worker on a stub DOMRect. * The CSS guard matched the selector as a substring, which still passed with the rule body emptied or the underline deleted -- and the underline is the half that survives on a dark bar. It now reads the declarations. * The table cases used `toContain`, which a regression leaving Home permanently marked would satisfy on every row. `toEqual([text])`. * A comment claimed the tab "only matched its own exact path", which is the root cause the PR description explicitly rules out -- Videos never carried `end`. Corrected to what those cases actually guard. * The navbar offers twelve colors, not fourteen. I had carried over Semantic's palette count, which includes black and white. Writing the tightened height assertion I reached for `(?!0)`, which calls 0.1875rem zero. It reads the number instead. --- app/src/App.css | 40 ++++++ app/src/components/Nav.js | 40 +++++- app/src/components/Nav.test.js | 233 +++++++++++++++++++++++++++++++++ 3 files changed, 309 insertions(+), 4 deletions(-) create mode 100644 app/src/components/Nav.test.js 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:/); + }); +});