diff --git a/app/cypress/e2e/map-themes.cy.js b/app/cypress/e2e/map-themes.cy.js new file mode 100644 index 00000000..ddccf46f --- /dev/null +++ b/app/cypress/e2e/map-themes.cy.js @@ -0,0 +1,44 @@ +/* + * The map basemap has a "flavor" of its own, and MapViewer used to hand protomaps the WROLPi + * theme name. That worked while the only themes were `light` and `dark`, and broke when + * night and amber arrived: protomaps looks the flavor up in a record and dereferences the + * result without checking, so an unknown name throws + * `Cannot read properties of undefined (reading 'background')` before the first tile and the + * viewer renders nothing at all. + * + * Driven through the real page in every theme, because the unit test can only check that the + * mapping returns something valid -- it cannot see whether MapViewer USES it, which is the + * half that was broken. + */ +const THEMES = ['light', 'dark', 'night', 'amber']; + +describe('the map viewer builds a basemap in every theme', () => { + THEMES.forEach((theme) => { + it(`initialises MapLibre in ${theme}`, () => { + const errors = []; + cy.visit('/map', { + onBeforeLoad(win) { + // Set the theme before the app boots, so the viewer builds its style once + // with the theme under test rather than rebuilding after a switch. + win.localStorage.setItem('color-scheme', theme); + // MapViewer reports a failed init through console.error and then renders + // nothing, so the console is where the defect is visible. + const original = win.console.error; + win.console.error = (...args) => { + errors.push(args.map(String).join(' ')); + original(...args); + }; + }, + }); + + // The canvas only exists once MapLibre constructed a style successfully. + cy.get('canvas.maplibregl-canvas', {timeout: 40000}).should('exist'); + + cy.then(() => { + const failures = errors.filter(message => + /MapLibre failed to initialize|reading 'background'/.test(message)); + expect(failures.join('\n')).to.equal(''); + }); + }); + }); +}); diff --git a/app/src/components/MapViewer.js b/app/src/components/MapViewer.js index 9a4765c3..f7ce1970 100644 --- a/app/src/components/MapViewer.js +++ b/app/src/components/MapViewer.js @@ -9,6 +9,7 @@ import {addMapPin, deleteMapPin, getMapFiles, getMapPins, searchMap, setMapDefau import {MAP_VIEWER_URI} from "./Vars"; import {Button, Checkbox, Header, Icon, Panel, TextInput, Toggle} from "./ui"; import {SettingsContext, ThemeContext} from "../contexts/contexts"; +import {mapFlavor} from "../themes/names"; // Terrain DEM file prefix for hillshade and contours. const TERRAIN_PREFIX = "terrain-"; @@ -611,7 +612,7 @@ export default function MapViewer() { try { map = new maplibregl.Map({ container: mapContainer.current, - style: buildStyle(mapSources, theme, hasTerrain, scaleUnit), + style: buildStyle(mapSources, mapFlavor(theme), hasTerrain, scaleUnit), center: [initialLon, initialLat], zoom: initialZoom, attributionControl: true, @@ -757,7 +758,7 @@ export default function MapViewer() { const center = map.getCenter(); const zoom = map.getZoom(); - map.setStyle(buildStyle(activeFilesRef.current, theme, hasTerrainRef.current, scaleUnit)); + map.setStyle(buildStyle(activeFilesRef.current, mapFlavor(theme), hasTerrainRef.current, scaleUnit)); map.once("style.load", () => { map.setCenter(center); map.setZoom(zoom); diff --git a/app/src/themes/map-flavor.test.js b/app/src/themes/map-flavor.test.js new file mode 100644 index 00000000..23cc7454 --- /dev/null +++ b/app/src/themes/map-flavor.test.js @@ -0,0 +1,58 @@ +import fs from 'fs'; +import path from 'path'; +import layers from 'protomaps-themes-base'; +import {isDarkTheme, mapFlavor, themeNames} from './names'; + +/* + * The map viewer draws its basemap with protomaps-themes-base, which takes a "flavor". + * + * MapViewer used to hand it the WROLPi theme name directly. That worked while there were + * two themes called `light` and `dark`, and broke the day night and amber arrived: protomaps + * looks the flavor up in a record, finds nothing, and dereferences it anyway -- + * `TypeError: Cannot read properties of undefined (reading 'background')`, thrown before a + * single tile is drawn, so /map was blank in two of the four themes. + * + * The flavor is also interpolated into a sprite URL, so it has to be a name we ship sprites + * for, which is a smaller set again. Both halves are checked against the real thing -- + * protomaps itself, and the sprites directory on disk -- rather than against a list repeated + * here, because a list repeated here is exactly what would have passed in the first place. + */ + +const SPRITES = path.join(__dirname, '../../../modules/map/static/sprites'); + +/** The sprite sets actually shipped, from disk. */ +const spriteFlavors = () => [...new Set(fs.readdirSync(SPRITES) + .map(file => file.replace(/(@2x)?\.(json|png)$/, '')))].sort(); + +describe('the map basemap flavor', () => { + it('ships sprites for at least two flavors', () => { + // The premise for every case below. If the directory were empty or unreadable, + // `toContain` would fail for the wrong reason and read as a mapping bug. + expect(spriteFlavors().length).toBeGreaterThan(1); + }); + + it.each(themeNames)('maps %s to a flavor protomaps understands', (theme) => { + const flavor = mapFlavor(theme); + + // protomaps itself is the authority, not a list copied into this file. It throws on + // an unknown flavor rather than returning nothing, which is the whole bug. + const built = layers('src', flavor); + expect(Array.isArray(built)).toBe(true); + expect(built.length).toBeGreaterThan(0); + }); + + it.each(themeNames)('maps %s to a flavor we ship sprites for', (theme) => { + // The flavor is interpolated into `/map-assets/sprites/`, so a name protomaps + // accepts is not enough -- `black` and `grayscale` are real flavors with no sprites. + expect(spriteFlavors()).toContain(mapFlavor(theme)); + }); + + it.each(themeNames)('keeps %s on a basemap of its own brightness', (theme) => { + /* + * A dark theme must not get the light basemap. Night and amber tint the canvas with + * an SVG filter, but that only shifts hue -- a white slab filtered to red is a red + * slab, and the point of night mode is not to put one of those on screen. + */ + expect(mapFlavor(theme) === 'light').toBe(!isDarkTheme(theme)); + }); +}); diff --git a/app/src/themes/names.ts b/app/src/themes/names.ts index 7c06e157..32f0a537 100644 --- a/app/src/themes/names.ts +++ b/app/src/themes/names.ts @@ -42,6 +42,28 @@ export const isMonochromeTheme = (theme: ThemeName): boolean => export const isDarkTheme = (theme: ThemeName): boolean => darkThemes.includes(theme); +/* + * The basemap flavor for a theme. + * + * The map viewer draws with protomaps-themes-base, which takes a flavor of its own and is + * NOT a place to pass a WROLPi theme name. It looks the flavor up in a record and then + * dereferences the result without checking, so an unknown name is not a fallback but a + * `TypeError: Cannot read properties of undefined (reading 'background')` thrown before the + * first tile -- which is what /map did in night and amber once those themes existed. + * + * Only `light` and `dark` are available: the flavor is also interpolated into + * `/map-assets/sprites/`, and those are the two sprite sets we ship. protomaps' + * `black` and `grayscale` would load a style and then fail to find any icons. + * + * Night and amber take `dark` and let the media filter tint the canvas from there. Handing + * them `light` would not work even with the filter on: a filter shifts hue, so a white + * basemap becomes a red one, which is the opposite of what a night-vision theme is for. + */ +export type MapFlavor = 'light' | 'dark'; + +export const mapFlavor = (theme: ThemeName): MapFlavor => + isDarkTheme(theme) ? 'dark' : 'light'; + export const isThemeName = (value: unknown): value is ThemeName => themeNames.includes(value as ThemeName);