diff --git a/client/src/components/Map/MapView.test.tsx b/client/src/components/Map/MapView.test.tsx index d44923dbe..3a35c46d3 100644 --- a/client/src/components/Map/MapView.test.tsx +++ b/client/src/components/Map/MapView.test.tsx @@ -1180,6 +1180,31 @@ describe('MapView photo thumbnails', () => { expect(thumbCallbacks.size).toBe(0) expect(vi.mocked(photoService.fetchPhoto)).not.toHaveBeenCalled() }) + + it('FE-COMP-MAPVIEW-080: an uploaded photo fills its marker whatever its proportions', () => { + // Leaflet's marker pane sets `width: auto` on every img in it, so a photo sized + // by attributes was drawn at its full pixel size and the circle only ever showed + // the transparent corner of it, over the category colour. + render() + const holder = document.createElement('div') + holder.innerHTML = iconHtmlOf(screen.getAllByTestId('marker')[0]) + const img = holder.querySelector('img')! + + expect(img.getAttribute('src')).toBe('/uploads/places/wide.jpg') + expect(img.style.width).toBe('100%') + expect(img.style.height).toBe('100%') + }) + + it('FE-COMP-MAPVIEW-081: taking the upload off a place asks for its auto photo again, without a reload', () => { + const place = buildMapPlace({ id: 28, lat: 48, lng: 2, google_place_id: 'gp-28', name: 'Tower', image_url: '/uploads/places/own.jpg' }) + const { rerender } = render() + expect(vi.mocked(photoService.fetchPhoto)).not.toHaveBeenCalled() + + rerender() + + expect(vi.mocked(photoService.fetchPhoto)).toHaveBeenCalledWith('gp-28', 'gp-28', 48, 2, 'Tower') + expect(thumbCallbacks.has('gp-28')).toBe(true) + }) }) // The marker HTML is a hand-built string handed to L.divIcon, i.e. innerHTML. diff --git a/client/src/components/Map/MapView.tsx b/client/src/components/Map/MapView.tsx index 169f8bfcb..82332b5bf 100644 --- a/client/src/components/Map/MapView.tsx +++ b/client/src/components/Map/MapView.tsx @@ -195,7 +195,7 @@ function createPlaceIcon(place, orderNumbers, isSelected) { box-shadow:${shadow}; overflow:hidden;background:${bgColor}; "> - + ${markerPhotoHtml(place.image_url)} ${badgeHtml} `, @@ -543,7 +543,7 @@ function MapContextMenuHandler({ onContextMenu }: { onContextMenu: ((e: L.Leafle // Module-level photo cache shared with PlaceAvatar import { getCached, isLoading, fetchPhoto, onThumbReady, getAllThumbs } from '../../services/photoService' -import { isCustomPlaceImage, photoCacheKey } from './placePhoto' +import { isCustomPlaceImage, markerPhotoHtml, photoCacheKey, photoSourcesKey } from './placePhoto' import { useAuthStore } from '../../store/authStore' import { useGeolocation } from '../../hooks/useGeolocation' import LocationButton from './LocationButton' @@ -838,7 +838,7 @@ export const MapView = memo(function MapView({ const pendingThumbsRef = useRef>({}) const thumbRafRef = useRef(null) - const placeIds = useMemo(() => places.map(p => p.id).join(','), [places]) + const photoSources = useMemo(() => photoSourcesKey(places), [places]) // Flattened [lat,lng] points of the selected day's route, so the bounds fit can // include the full polyline once it has been computed. const routeCoords = useMemo<[number, number][]>(() => (route || []).flat() as [number, number][], [route]) @@ -895,7 +895,7 @@ export const MapView = memo(function MapView({ thumbRafRef.current = null } } - }, [placeIds, placesPhotosEnabled]) + }, [photoSources, placesPhotosEnabled]) const isTouchDevice = typeof window !== 'undefined' && navigator.maxTouchPoints > 0 // Drag a marker onto a day (#891). Pointer-driven, so it is off wherever diff --git a/client/src/components/Map/MapViewGL.test.tsx b/client/src/components/Map/MapViewGL.test.tsx index 513e1ddc1..43e0d9104 100644 --- a/client/src/components/Map/MapViewGL.test.tsx +++ b/client/src/components/Map/MapViewGL.test.tsx @@ -1735,7 +1735,7 @@ describe('MapViewGL', () => { expect(srcs()).toContain('data:image/png;base64,BBB') }) - it('FE-COMP-MAPVIEWGL-056: an in-flight photo is not requested twice and the proxy url is used as the id', async () => { + it('FE-COMP-MAPVIEWGL-056: an in-flight photo is not requested twice and a picked proxy url is both id and cache key', async () => { vi.mocked(photoService.isLoading).mockImplementation((key: string) => key === 'osm-loading') const places = [ buildMapPlace({ id: 94, lat: 48.4, lng: 2.4, osm_id: 'osm-loading' }), @@ -1746,7 +1746,56 @@ describe('MapViewGL', () => { await act(async () => {}) expect(photoService.fetchPhoto).toHaveBeenCalledTimes(1) - expect(photoService.fetchPhoto).toHaveBeenCalledWith('osm-5', '/api/maps/place-photo/abc', 48.5, 2.5, 'Museum') + expect(photoService.fetchPhoto).toHaveBeenCalledWith('/api/maps/place-photo/abc', '/api/maps/place-photo/abc', 48.5, 2.5, 'Museum') + }) + + it('FE-COMP-MAPVIEWGL-101: an uploaded photo fills its marker whatever its proportions', async () => { + // A phone photo is no 48px square like the thumbs. Sized by attributes it lost to + // the stylesheet's `height: auto` and sat as a sliver in a disc of category colour. + loadOnAttach() + render() + await act(async () => {}) + + const img = glMarkers.created[0].element.querySelector('img')! + expect(img.getAttribute('src')).toBe('/uploads/places/wide.jpg') + expect(img.style.width).toBe('100%') + expect(img.style.height).toBe('100%') + }) + + it('FE-COMP-MAPVIEWGL-102: taking the upload off a place asks for its auto photo again, without a reload', async () => { + const place = buildMapPlace({ id: 15, lat: 48.15, lng: 2.15, google_place_id: 'gp-15', name: 'Tower', image_url: '/uploads/places/own.jpg' }) + const { rerender } = render() + await act(async () => {}) + expect(photoService.fetchPhoto).not.toHaveBeenCalled() + + rerender() + await act(async () => {}) + + expect(photoService.fetchPhoto).toHaveBeenCalledWith('gp-15', 'gp-15', 48.15, 2.15, 'Tower') + }) + + it('FE-COMP-MAPVIEWGL-103: picking another suggested photo replaces the thumb of the old one', async () => { + loadOnAttach() + const first = '/api/maps/place-photo/gp-16~p0/bytes' + const second = '/api/maps/place-photo/gp-16~p1/bytes' + // Everything seen before the new pick has a thumb: the place's auto photo under + // its provider id and the first pick under its own url. + vi.mocked(photoService.getCached).mockImplementation((key: string) => ( + key === 'gp-16' || key === first + ? ({ thumbDataUrl: 'data:image/png;base64,OLD' } as unknown as ReturnType) + : undefined + )) + const place = buildMapPlace({ id: 16, lat: 48.16, lng: 2.16, google_place_id: 'gp-16', name: 'Tower', image_url: first }) + const { rerender } = render() + await flushFrames() + + rerender() + await flushFrames() + + expect(photoService.fetchPhoto).toHaveBeenCalledWith(second, second, 48.16, 2.16, 'Tower') + // Until its thumb is ready the marker shows the new pick itself, not the old thumb. + const latest = glMarkers.created[glMarkers.created.length - 1] + expect(latest.element.querySelector('img')?.getAttribute('src')).toBe(second) }) it('FE-COMP-MAPVIEWGL-066: a place with neither provider id nor coordinates has no cache key and is skipped', async () => { diff --git a/client/src/components/Map/MapViewGL.tsx b/client/src/components/Map/MapViewGL.tsx index db8584d28..9f21dfb4f 100644 --- a/client/src/components/Map/MapViewGL.tsx +++ b/client/src/components/Map/MapViewGL.tsx @@ -12,7 +12,7 @@ import { useTranslation } from '../../i18n/TranslationContext' import { MapLayerSwitcher, MAP_LAYER_SWITCHER_INSET, type BaseLayer } from './MapLayerSwitcher' import { useAuthStore } from '../../store/authStore' import { getCached, isLoading, fetchPhoto, onThumbReady, getAllThumbs } from '../../services/photoService' -import { isCustomPlaceImage, photoCacheKey } from './placePhoto' +import { isCustomPlaceImage, markerPhotoHtml, photoCacheKey, photoSourcesKey } from './placePhoto' import { CATEGORY_ICON_MAP } from '../shared/categoryIcons' import { isStandardFamily, supportsCustom3d, wantsTerrain, addCustom3dBuildings, addTerrainAndSky } from './mapboxSetup' import { attachLocationMarker, type LocationMarkerHandle } from './locationMarkerMapbox' @@ -20,7 +20,6 @@ import { ReservationMapboxOverlay } from './reservationsMapbox' import { useTransportRoutes } from '../../hooks/useTransportRoutes' import { visibleRouteReservations } from '../../utils/reservationRoutes' import { safeHexColor } from '../../utils/safeColor' -import { escapeHtml } from '@trek/shared' import { MAPBOX_DEFAULT_STYLE, styleForActiveProvider, basemapLanguage, type GlMapProvider } from './glProviders' import LocationButton from './LocationButton' import { useGeolocation } from '../../hooks/useGeolocation' @@ -406,7 +405,7 @@ function createMarkerElement(place: Place & { category_color?: string; category_ overflow:hidden;background:${bgColor}; box-sizing:content-box; "> - + ${markerPhotoHtml(photoUrl)} ${badgeHtml} ` @@ -1720,7 +1719,7 @@ export function MapViewGL({ // simultaneous thumb arrivals into one re-render. const pendingThumbsRef = useRef>({}) const thumbRafRef = useRef(null) - const placeIds = useMemo(() => places.map(p => p.id).join(','), [places]) + const photoSources = useMemo(() => photoSourcesKey(places), [places]) useEffect(() => { if (!places || places.length === 0 || !placesPhotosEnabled) return const cleanups: (() => void)[] = [] @@ -1771,7 +1770,7 @@ export function MapViewGL({ thumbRafRef.current = null } } - }, [placeIds, placesPhotosEnabled]) // eslint-disable-line react-hooks/exhaustive-deps + }, [photoSources, placesPhotosEnabled]) // eslint-disable-line react-hooks/exhaustive-deps // Reconcile markers with places + photos. The clustered GeoJSON source decides // which points are currently unclustered, and we render the existing rich HTML diff --git a/client/src/components/Map/placePhoto.test.ts b/client/src/components/Map/placePhoto.test.ts new file mode 100644 index 000000000..811eb26cc --- /dev/null +++ b/client/src/components/Map/placePhoto.test.ts @@ -0,0 +1,67 @@ +import { describe, it, expect } from 'vitest' +import { isCustomPlaceImage, markerPhotoHtml, photoCacheKey, photoSourcesKey } from './placePhoto' + +function imgOf(html: string): HTMLImageElement { + const holder = document.createElement('div') + holder.innerHTML = html + return holder.querySelector('img')! +} + +describe('isCustomPlaceImage', () => { + it('FE-MAP-PLACEPHOTO-001: an upload or an inline image is shown as it is, a provider photo is not', () => { + expect(isCustomPlaceImage('/uploads/places/a.jpg')).toBe(true) + expect(isCustomPlaceImage('data:image/png;base64,AAA')).toBe(true) + expect(isCustomPlaceImage('/api/maps/place-photo/gp-1~p0/bytes')).toBe(false) + expect(isCustomPlaceImage(null)).toBe(false) + }) +}) + +describe('markerPhotoHtml', () => { + it('FE-MAP-PLACEPHOTO-002: sizes the picture by its style, where no stylesheet rule can undo it', () => { + // Preflight's `height: auto` and Leaflet's `width: auto` both outrank a width or + // height attribute, so a photo that is not a small square has to be pinned here. + const img = imgOf(markerPhotoHtml('/uploads/places/wide.jpg')) + expect(img.getAttribute('src')).toBe('/uploads/places/wide.jpg') + expect(img.style.width).toBe('100%') + expect(img.style.height).toBe('100%') + expect(img.hasAttribute('width')).toBe(false) + expect(img.hasAttribute('height')).toBe(false) + }) + + it('FE-MAP-PLACEPHOTO-003: escapes the url, which a user can set', () => { + const html = markerPhotoHtml('/uploads/x" onerror="alert(1)" y="') + expect(html).not.toContain('onerror="alert(1)"') + expect(imgOf(html).getAttribute('src')).toBe('/uploads/x" onerror="alert(1)" y="') + }) +}) + +describe('photoCacheKey', () => { + it('FE-MAP-PLACEPHOTO-004: a picked provider photo is its own key, ahead of the provider id', () => { + expect(photoCacheKey({ image_url: '/api/maps/place-photo/gp-1~p2/bytes', google_place_id: 'gp-1', lat: 1, lng: 2 })) + .toBe('/api/maps/place-photo/gp-1~p2/bytes') + }) + + it('FE-MAP-PLACEPHOTO-005: otherwise the provider id, then the coordinates, then nothing', () => { + expect(photoCacheKey({ image_url: '/uploads/places/a.jpg', google_place_id: 'gp-1', osm_id: 'node/1' })).toBe('gp-1') + expect(photoCacheKey({ osm_id: 'node/1', lat: 1, lng: 2 })).toBe('node/1') + expect(photoCacheKey({ lat: 0, lng: 0 })).toBe('0,0') + expect(photoCacheKey({ image_url: 'https://example.com/a.jpg', lat: null, lng: null })).toBe('') + }) +}) + +describe('photoSourcesKey', () => { + const place = { id: 7, google_place_id: 'gp-7', lat: 48, lng: 2 } + + it('FE-MAP-PLACEPHOTO-006: changes when an upload comes off or another photo is picked', () => { + const withUpload = photoSourcesKey([{ ...place, image_url: '/uploads/places/a.jpg' }]) + const bare = photoSourcesKey([{ ...place, image_url: null }]) + const picked = photoSourcesKey([{ ...place, image_url: '/api/maps/place-photo/gp-7~p0/bytes' }]) + const pickedAgain = photoSourcesKey([{ ...place, image_url: '/api/maps/place-photo/gp-7~p1/bytes' }]) + expect(new Set([withUpload, bare, picked, pickedAgain]).size).toBe(4) + }) + + it('FE-MAP-PLACEPHOTO-007: stays put when one upload replaces another, which needs no thumb', () => { + expect(photoSourcesKey([{ ...place, image_url: '/uploads/places/a.jpg' }])) + .toBe(photoSourcesKey([{ ...place, image_url: '/uploads/places/b.jpg' }])) + }) +}) diff --git a/client/src/components/Map/placePhoto.ts b/client/src/components/Map/placePhoto.ts index 71874b82c..3086a9002 100644 --- a/client/src/components/Map/placePhoto.ts +++ b/client/src/components/Map/placePhoto.ts @@ -1,3 +1,16 @@ +import { escapeHtml } from '@trek/shared' + +const PHOTO_PROXY_PREFIX = '/api/maps/place-photo/' + +/** The fields of a place that decide which picture its marker gets. */ +interface PhotoSource { + image_url?: string | null + google_place_id?: string | null + osm_id?: string | null + lat?: number | null + lng?: number | null +} + /** * A place's image_url that is directly displayable on a map marker and should * win over the auto-fetched provider thumbnail — a custom uploaded image (#1136) @@ -11,18 +24,41 @@ export function isCustomPlaceImage(url: string | null | undefined): boolean { /** * Cache key under which a place's auto-fetched thumbnail is stored: the provider - * id when there is one, otherwise the coordinates. A place with neither has no - * identity to cache under, so it gets the empty string — callers skip it rather + * photo the user picked when there is one, else the provider id, otherwise the + * coordinates. The picked photo comes first because picking another picture of the + * same place leaves its provider id alone, and the thumb cached under that id + * would go on answering for the new one. A place with none of these has no + * identity to cache under, so it gets the empty string: callers skip it rather * than filing every coordinate-less place under one shared entry. */ -export function photoCacheKey(place: { - google_place_id?: string | null - osm_id?: string | null - lat?: number | null - lng?: number | null -}): string { +export function photoCacheKey(place: PhotoSource): string { + if (place.image_url?.startsWith(PHOTO_PROXY_PREFIX)) return place.image_url if (place.google_place_id) return place.google_place_id if (place.osm_id) return place.osm_id if (place.lat == null || place.lng == null) return '' return `${place.lat},${place.lng}` } + +/** + * What the maps' thumb loading depends on, as one string to key the effect on. + * Place ids alone missed every change of picture: taking an upload off a place or + * picking another suggested photo changes no id, so the marker kept a category + * dot or the old thumb until the page was reloaded. + */ +export function photoSourcesKey(places: Array): string { + return places.map(p => `${p.id}:${isCustomPlaceImage(p.image_url) ? 'own' : photoCacheKey(p)}`).join('|') +} + +/** + * The picture inside a round place marker, for both the Leaflet and the GL map. + * + * Sized by its style and not by width/height attributes, because stylesheet rules + * beat attributes: Tailwind's preflight gives every img `height: auto`, and + * Leaflet's marker pane adds `width: auto`. The 48px square thumbs got away with + * that. An uploaded photo kept its own proportions on the GL map and its full pixel + * size on Leaflet, where only the rounded-off corner of a 4000px image lands inside + * the circle, so the marker showed nothing but its category colour. + */ +export function markerPhotoHtml(url: string): string { + return `` +}