From 4cba3450a7b5fabed7ed606692157c04333f5913 Mon Sep 17 00:00:00 2001 From: Tane Morgan <464864+tanem@users.noreply.github.com> Date: Wed, 5 Aug 2026 06:44:05 +1200 Subject: [PATCH 1/5] Add a loadingDelay prop Milliseconds to wait before rendering `loading`, default 0. An injection that finishes sooner never renders it at all. The flash it suppresses is not new and not about the request cache: a 30ms cold load in Chrome 151 paints a loading element on 5 frames out of 14, which every version has done. NN/g's position is that a sub-second indicator is worse than none, because the user cannot tell what flashed. Default 0, so this is purely opt-in. The package cannot tell a spinner from a skeleton sized to reserve the SVG's space, and delaying the latter trades one layout shift for two, so only the consumer can choose. The delay is its own effect keyed on `isLoading` rather than a dependency of the injection effect, so changing it does not re-inject. Initial state reads the prop instead of starting false: effects run after paint, so starting false would cost the default a frame without the loader. `fallback` is not delayed. An error costs a round trip that no cache short-circuits, so there is no flash to suppress there. Co-Authored-By: Claude Opus 5 --- README.md | 9 ++++ src/ReactSVG.tsx | 39 ++++++++++++++- src/types.ts | 1 + test/browser.spec.tsx | 114 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 162 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 4918f8d45..94e7e0d48 100644 --- a/README.md +++ b/README.md @@ -44,6 +44,7 @@ const App = () => | [`fallback`](#fallback) | `React.ElementType` | none | | [`httpRequestWithCredentials`](#httprequestwithcredentials) | `boolean` | `false` | | [`loading`](#loading) | `React.ElementType` | none | +| [`loadingDelay`](#loadingdelay) | `number` | `0` | | [`onError`](#onerror) | `(error: unknown) => void` | noop | | [`renumerateIRIElements`](#renumerateirielements) | `boolean` | `true` | | [`title`](#title) | `string` | `''` | @@ -84,6 +85,14 @@ Whether cross-site Access-Control requests for the SVG are made using credential Rendered inside the wrapper until the SVG is injected. Can be a string, class component or function component. Nothing is rendered in its place when unset. +#### `loadingDelay` + +Milliseconds to wait before rendering `loading`. At the default `0` it renders immediately. Set a value and an injection that finishes sooner - a warm request cache, a localhost or `file://` read, a warm CDN edge - never renders `loading` at all. + +Use it when `loading` is a spinner, where a sub-second appearance is worse than none: the user can't tell what flashed. Leave it at `0` when `loading` is a skeleton sized to hold the SVG's space, because delaying that trades one layout shift for two. + +200-300ms is the usual industry choice. The delay applies to `loading` only - `fallback` always renders as soon as the error arrives, since an error costs a round trip that no cache short-circuits. + #### `onError` Called if an error occurs. `error` is an `unknown` value. diff --git a/src/ReactSVG.tsx b/src/ReactSVG.tsx index ab1ae7fc7..e732206c4 100644 --- a/src/ReactSVG.tsx +++ b/src/ReactSVG.tsx @@ -31,6 +31,7 @@ export const ReactSVG: React.ForwardRefExoticComponent< fallback: Fallback, httpRequestWithCredentials = false, loading: Loading, + loadingDelay = 0, onError = () => undefined, renumerateIRIElements = true, src, @@ -44,6 +45,15 @@ export const ReactSVG: React.ForwardRefExoticComponent< const [hasError, setHasError] = React.useState(false) const [isLoading, setIsLoading] = React.useState(true) + // `loading` is held back until the delay elapses, so an injection that + // resolves sooner - a warm cache, localhost, a file:// read - never paints + // an indicator at all. The initial value has to account for the delay + // rather than start false: effects run after paint, so initialising to + // false would cost the default a frame without the loader. + const [hasLoadingDelayElapsed, setHasLoadingDelayElapsed] = React.useState( + loadingDelay <= 0, + ) + const reactWrapperRef = React.useRef(null) // The callbacks are read through a ref so that changing them - which inline @@ -232,6 +242,33 @@ export const ReactSVG: React.ForwardRefExoticComponent< wrapper, ]) + // Keyed on `isLoading` rather than living in the injection effect, so that + // changing `loadingDelay` restarts the timer without re-running the + // injection. The injection effect setting `isLoading` back to true is what + // restarts the delay for a re-injection. + React.useEffect(() => { + if (!isLoading) { + return + } + + /* eslint-disable @eslint-react/set-state-in-effect */ + if (loadingDelay <= 0) { + setHasLoadingDelayElapsed(true) + return + } + + setHasLoadingDelayElapsed(false) + /* eslint-enable @eslint-react/set-state-in-effect */ + + const timeoutId = setTimeout(() => { + setHasLoadingDelayElapsed(true) + }, loadingDelay) + + return () => { + clearTimeout(timeoutId) + } + }, [isLoading, loadingDelay]) + const Wrapper = wrapper return ( @@ -245,7 +282,7 @@ export const ReactSVG: React.ForwardRefExoticComponent< } : {})} > - {isLoading && Loading && } + {isLoading && hasLoadingDelayElapsed && Loading && } {hasError && Fallback && } ) diff --git a/src/types.ts b/src/types.ts index a9b8aa96c..9ae72d163 100644 --- a/src/types.ts +++ b/src/types.ts @@ -9,6 +9,7 @@ interface BaseProps { fallback?: React.ElementType httpRequestWithCredentials?: boolean loading?: React.ElementType + loadingDelay?: number onError?: (error: unknown) => void renumerateIRIElements?: boolean src: string diff --git a/test/browser.spec.tsx b/test/browser.spec.tsx index 2b73ade20..2d2c5df59 100644 --- a/test/browser.spec.tsx +++ b/test/browser.spec.tsx @@ -272,6 +272,120 @@ describe('while running in a browser environment', () => { ) }) + it('should hold the loader back until loadingDelay has elapsed', async () => { + const loading = () => loading + + faker.seed(133) + const uuid = faker.string.uuid() + + nock('http://localhost') + .get(`/${uuid}.svg`) + .delay(200) + .reply(200, source, { 'Content-Type': 'image/svg+xml' }) + + const { container, queryByText } = render( + , + ) + + expect(queryByText('loading')).toBeNull() + + await waitFor(() => expect(queryByText('loading')).toBeTruthy()) + + await waitFor(() => + expect(container.querySelectorAll('.injected-svg')).toHaveLength(1), + ) + }) + + // The case the prop exists for. Same warm-cache setup as above, but the + // delay outlasts the cache hit, so the loader never reaches the DOM at all + // rather than painting and being pulled away again. + it('should never render the loader when the injection beats loadingDelay', async () => { + const loading = () => loading + const src = 'http://localhost/loading-delay-cache.svg' + + nock('http://localhost') + .get('/loading-delay-cache.svg') + .reply(200, source, { 'Content-Type': 'image/svg+xml' }) + + const first = render() + + await waitFor(() => + expect(first.container.querySelectorAll('.injected-svg')).toHaveLength(1), + ) + + first.unmount() + nock.cleanAll() + + const second = render( + , + ) + + expect(second.queryByText('loading')).toBeNull() + + await waitFor(() => + expect(second.container.querySelectorAll('.injected-svg')).toHaveLength( + 1, + ), + ) + + expect(second.queryByText('loading')).toBeNull() + }) + + it('should render the loader immediately when loadingDelay is zero', async () => { + const loading = () => loading + + faker.seed(134) + const uuid = faker.string.uuid() + + nock('http://localhost') + .get(`/${uuid}.svg`) + .reply(200, source, { 'Content-Type': 'image/svg+xml' }) + + const { container, getByText } = render( + , + ) + + expect(getByText('loading')).toBeTruthy() + + await waitFor(() => + expect(container.querySelectorAll('.injected-svg')).toHaveLength(1), + ) + }) + + // The delay is for the loading path only. An error costs a round trip that + // no cache short-circuits, so there is no flash to suppress, and holding the + // fallback back would only delay the message. + it('should not delay the fallback', async () => { + const fallback = () => fallback + const loading = () => loading + + faker.seed(135) + const uuid = faker.string.uuid() + + nock('http://localhost').get(`/${uuid}.svg`).reply(404) + + const { findByText, queryByText } = render( + , + ) + + await findByText('fallback') + + expect(queryByText('loading')).toBeNull() + }) + it('allows rendering of span wrappers', async () => { faker.seed(132) const uuid = faker.string.uuid() From eed92b7bad9cb3f625e801a592c8a29a5346261b Mon Sep 17 00:00:00 2001 From: Tane Morgan <464864+tanem@users.noreply.github.com> Date: Wed, 5 Aug 2026 08:16:10 +1200 Subject: [PATCH 2/5] Clear the loadingDelay flag when a re-injection starts `hasLoadingDelayElapsed` was reset by the delay effect, which is keyed on `isLoading` and so only runs once the injection effect's render has committed. The render in between still saw the flag left true by the previous injection, so a re-injection mounted `loading` immediately regardless of the delay. Measured against dist/ in Chrome by sampling requestAnimationFrame, which runs immediately before paint: over 60 re-injections with a 5000ms delay the loader mounted every time, and 8 of those reached a painted frame. The runs that painted are the ones where it stayed mounted longest, 1.0-2.0ms against a 0.1ms floor, so a slower device widens the window. That is the flash the prop exists to suppress. Clearing it in the injection effect puts it in the same commit as `setIsLoading(true)`, so no render sees the stale value. The prop is read through a ref, like the callbacks above it, to keep it out of the injection effect's dependency list: changing the delay must not re-inject. It clears to `loadingDelay <= 0` rather than false for the reason the initial state does, or the default would lose a frame without the loader on every re-injection. The test counts renders of `loading` rather than querying the DOM, because the stale mount lasts a fraction of a millisecond: long enough for Chrome to paint, far too short to catch after the fact. Also renumbers three seeds the new tests shared with existing ones, which resolved to the same URL and so shared svg-injector's cache. Co-Authored-By: Claude Opus 5 --- src/ReactSVG.tsx | 26 ++++++++++++++--- test/browser.spec.tsx | 66 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 85 insertions(+), 7 deletions(-) diff --git a/src/ReactSVG.tsx b/src/ReactSVG.tsx index e732206c4..04020dafd 100644 --- a/src/ReactSVG.tsx +++ b/src/ReactSVG.tsx @@ -69,6 +69,15 @@ export const ReactSVG: React.ForwardRefExoticComponent< callbacksRef.current = { afterInjection, beforeInjection, onError } }) + // Read through a ref for the same reason, and declared here so it is + // current by the time the injection effect runs: that effect has to clear + // the elapsed flag, but listing `loadingDelay` as a dependency would make + // changing the delay re-inject. + const loadingDelayRef = React.useRef(loadingDelay) + React.useEffect(() => { + loadingDelayRef.current = loadingDelay + }) + const refCallback = React.useCallback( (reactWrapper: WrapperType | null) => { reactWrapperRef.current = reactWrapper @@ -107,11 +116,20 @@ export const ReactSVG: React.ForwardRefExoticComponent< } // A new injection is starting, so any result from the previous one is - // stale. On mount both values already hold these defaults and React bails - // out, so this only re-renders when a dependency actually changed. + // stale. On mount all three values already hold these defaults and React + // bails out, so this only re-renders when a dependency actually changed. + // + // The elapsed flag has to be cleared here rather than left to the delay + // effect below: that effect is keyed on `isLoading`, so it only reacts + // once this render has committed, and the render in between would still + // see a flag left true by the previous injection and mount `loading` + // regardless of the delay. It takes `loadingDelay <= 0` rather than a + // plain false for the reason the initial state does - the default would + // otherwise lose a frame to a re-injection. /* eslint-disable @eslint-react/set-state-in-effect */ setHasError(false) setIsLoading(true) + setHasLoadingDelayElapsed(loadingDelayRef.current <= 0) /* eslint-enable @eslint-react/set-state-in-effect */ let nonReactTarget: WrapperType @@ -244,8 +262,8 @@ export const ReactSVG: React.ForwardRefExoticComponent< // Keyed on `isLoading` rather than living in the injection effect, so that // changing `loadingDelay` restarts the timer without re-running the - // injection. The injection effect setting `isLoading` back to true is what - // restarts the delay for a re-injection. + // injection. Only the timer lives here; a re-injection's flag is cleared by + // the injection effect itself, which is a render earlier than this can run. React.useEffect(() => { if (!isLoading) { return diff --git a/test/browser.spec.tsx b/test/browser.spec.tsx index 2d2c5df59..198966835 100644 --- a/test/browser.spec.tsx +++ b/test/browser.spec.tsx @@ -275,7 +275,7 @@ describe('while running in a browser environment', () => { it('should hold the loader back until loadingDelay has elapsed', async () => { const loading = () => loading - faker.seed(133) + faker.seed(190) const uuid = faker.string.uuid() nock('http://localhost') @@ -335,10 +335,70 @@ describe('while running in a browser environment', () => { expect(second.queryByText('loading')).toBeNull() }) + // The delay is per injection, so an elapsed one must not carry over: changing + // `src` starts a fresh delay that has to hold the loader back again. Counts + // renders of `loading` rather than querying the DOM, because carrying the + // flag over mounts it for a fraction of a millisecond before the delay effect + // pulls it again - long enough for Chrome to paint it, far too short for a + // query after the fact to catch. + it('should not carry an elapsed loadingDelay into a re-injection', async () => { + let loadingRenders = 0 + const loading = () => { + loadingRenders += 1 + return loading + } + + faker.seed(189) + const first = faker.string.uuid() + const second = faker.string.uuid() + + nock('http://localhost') + .get(`/${first}.svg`) + .delay(200) + .reply(200, source, { 'Content-Type': 'image/svg+xml' }) + .get(`/${second}.svg`) + .delay(200) + .reply(200, source, { 'Content-Type': 'image/svg+xml' }) + + const { container, rerender } = render( + , + ) + + // The first delay has to actually elapse, or there is no elapsed flag to + // carry over and the assertion below passes without exercising anything. + await waitFor(() => expect(loadingRenders).toBeGreaterThan(0)) + + await waitFor(() => + expect(container.querySelectorAll('.injected-svg')).toHaveLength(1), + ) + + loadingRenders = 0 + + rerender( + , + ) + + await waitFor(() => expect(nock.isDone()).toBe(true)) + + await waitFor(() => + expect(container.querySelectorAll('.injected-svg')).toHaveLength(1), + ) + + expect(loadingRenders).toBe(0) + }) + it('should render the loader immediately when loadingDelay is zero', async () => { const loading = () => loading - faker.seed(134) + faker.seed(191) const uuid = faker.string.uuid() nock('http://localhost') @@ -367,7 +427,7 @@ describe('while running in a browser environment', () => { const fallback = () => fallback const loading = () => loading - faker.seed(135) + faker.seed(192) const uuid = faker.string.uuid() nock('http://localhost').get(`/${uuid}.svg`).reply(404) From b06a2cb40d794ca65308fc8c0a5cac849236e8e6 Mon Sep 17 00:00:00 2001 From: Tane Morgan <464864+tanem@users.noreply.github.com> Date: Thu, 6 Aug 2026 05:38:29 +1200 Subject: [PATCH 3/5] Record a screen-reader run against the loadingDelay work Announcement behaviour is unchanged from 2026-08-04: step 0 announces, every other case is silent, and a 2515ms mount is as silent as the millisecond-scale ones. The entry says what the run does not cover, since that is easy to read the wrong way. `loadingDelay` defaults to 0, so every step here exercises the default path and none of them sets the prop. A delay long enough to suppress the mount leaves no element to announce, which the DOM log settles without a screen reader, and no step changes `src` on a mounted component, which is the path eed92b7b corrects. Co-Authored-By: Claude Opus 5 --- test/manual/README.md | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/test/manual/README.md b/test/manual/README.md index 9bc33bfe3..03c128850 100644 --- a/test/manual/README.md +++ b/test/manual/README.md @@ -136,16 +136,16 @@ VoiceOver / macOS version: Recorded so a later run has something to compare against. -19.0.0, 2026-08-04, Safari 26.5 on macOS 26.5, VoiceOver with the caption panel -open: +19.0.0 plus the `loadingDelay` work (eed92b7b), 2026-08-06, Safari 26.5 on macOS +15.7.7, VoiceOver with the caption panel open: -| Case | Caption panel | DOM | -| ------------------------------------------------------- | ------------- | ------------------------------------ | -| 0 — existing live region, text changed | announces | n/a | -| 0b — `role="status"` element inserted already-populated | silent | n/a | -| A — cached remount, `role="status"` | silent | 8 elements, 0 requests | -| B — cached remount, plain span | silent | 8 elements, median 2.0ms, 0 requests | -| 4 — `loading`, `role="status"`, ~2.5s mounted | silent | 1 element | +| Case | Caption panel | DOM | +| ------------------------------------------------------- | ------------- | ------------------------------------------------ | +| 0 — existing live region, text changed | announces | n/a | +| 0b — `role="status"` element inserted already-populated | silent | n/a | +| A — cached remount, `role="status"` | silent | 8 elements, median 6.0ms, range 5.0-6.0, 0 requests | +| B — cached remount, plain span | silent | 8 elements, median 7.0ms, range 6.0-7.0, 0 requests | +| 4 — `loading`, `role="status"`, ~2.5s mounted | silent | 1 element, 2515.0ms | **No announcement, and lifetime is not the variable.** VoiceOver announces a live region whose content changes and ignores one that arrives with its content @@ -153,8 +153,22 @@ already in it. Step 0b establishes that with neither React nor svg-injector in the picture, so it is platform behaviour react-svg inherits. React mounts a `loading` component as a complete element, which is always the second shape, and a `role="status"` element mounted for a full 2.5 seconds was as silent as the -two-millisecond ones. Live-region semantics made no difference either: A and B -were equally silent. +millisecond-scale ones. Live-region semantics made no difference either: A and B +were equally silent. Unchanged from the 2026-08-04 run. + +What this run does and does not cover. `loadingDelay` defaults to 0, so the +default path mounts `loading` exactly as before, and that is the path every step +here exercises - the harness never sets the prop. So this is a no-regression +check, not coverage of the prop: a delay long enough to suppress the mount +leaves no element to announce, which the DOM log settles without a screen +reader. No step changes `src` on a mounted component either, which is the path +the re-injection fix in eed92b7b corrects. + +Lifetimes came out longer than the 2026-08-04 run, B's median 7.0ms against +2.0ms. Both are the same shape - every cached remount mounts and unmounts the +element - and at this scale the figure tracks the machine and browser build +rather than anything in the package, so it is recorded rather than read as a +change. Re-run this against a different browser or screen reader, or if the mounting behaviour changes. From 1fcdc160356eb39bacc60494c015753ade103c40 Mon Sep 17 00:00:00 2001 From: Tane Morgan Date: Thu, 6 Aug 2026 06:30:22 +1200 Subject: [PATCH 4/5] Re-arm loadingDelay when a re-injection starts mid-flight The delay effect was keyed on `isLoading` and `loadingDelay`. A re-injection that begins while the previous request is still in flight leaves `isLoading` true the whole way through, so when `loadingDelay` is held constant, which is the usual way to pass it, neither dependency moved and the effect never re-ran. That showed up two ways: - If the first delay had elapsed, clearing the flag left nothing to set it again, and `loading` stayed suppressed for the whole of the second injection however slow it was. - If it had not, the first injection's timer survived and fired against the old start time, so `loading` appeared early. The first is a regression from clearing the flag in the injection effect: before that the flag stayed true and the loader stayed on screen. An `injectionId` counter, bumped whenever an injection starts, gives the effect something that moves every time. It is skipped for the first injection so mount still settles in one render. Clearing the flag in the injection effect stays as it was: the delay effect still only runs a render later, and without the clear that render would see a stale true. The existing re-injection test moved both of the original dependencies at once, waiting for the first injection to finish and also changing the delay, which is what hid the case where neither moves. Four tests now walk the rest of the matrix. Screen-reader run re-recorded against the fix. Announcement behaviour is unchanged; the harness still has no step that swaps `src` on a live component, so it does not reach either re-injection path. --- src/ReactSVG.tsx | 45 ++++++--- test/browser.spec.tsx | 222 ++++++++++++++++++++++++++++++++++++++++++ test/manual/README.md | 42 ++++---- 3 files changed, 279 insertions(+), 30 deletions(-) diff --git a/src/ReactSVG.tsx b/src/ReactSVG.tsx index 04020dafd..5e7bec271 100644 --- a/src/ReactSVG.tsx +++ b/src/ReactSVG.tsx @@ -54,6 +54,18 @@ export const ReactSVG: React.ForwardRefExoticComponent< loadingDelay <= 0, ) + // Bumped whenever an injection starts, so the delay effect below can tell + // one injection from the next. `isLoading` can't carry that on its own: a + // re-injection that begins while the previous request is still in flight + // leaves it true throughout, so with an unchanged `loadingDelay` neither of + // that effect's other dependencies would change and the timer would never + // re-arm. + // + // Skipped for the first injection, so mount still settles in one render - + // every value that effect writes already holds its default there. + const [injectionId, setInjectionId] = React.useState(0) + const hasInjectedRef = React.useRef(false) + const reactWrapperRef = React.useRef(null) // The callbacks are read through a ref so that changing them - which inline @@ -116,20 +128,26 @@ export const ReactSVG: React.ForwardRefExoticComponent< } // A new injection is starting, so any result from the previous one is - // stale. On mount all three values already hold these defaults and React - // bails out, so this only re-renders when a dependency actually changed. + // stale. On mount the three flags already hold these defaults and React + // bails out - and `injectionId` is skipped there for the same reason - so + // this only re-renders when a dependency actually changed. // // The elapsed flag has to be cleared here rather than left to the delay - // effect below: that effect is keyed on `isLoading`, so it only reacts - // once this render has committed, and the render in between would still - // see a flag left true by the previous injection and mount `loading` - // regardless of the delay. It takes `loadingDelay <= 0` rather than a - // plain false for the reason the initial state does - the default would - // otherwise lose a frame to a re-injection. + // effect below: that effect only reacts once this render has committed, + // and the render in between would still see a flag left true by the + // previous injection and mount `loading` regardless of the delay. It + // takes `loadingDelay <= 0` rather than a plain false for the reason the + // initial state does - the default would otherwise lose a frame to a + // re-injection. /* eslint-disable @eslint-react/set-state-in-effect */ setHasError(false) setIsLoading(true) setHasLoadingDelayElapsed(loadingDelayRef.current <= 0) + if (hasInjectedRef.current) { + setInjectionId((id) => id + 1) + } else { + hasInjectedRef.current = true + } /* eslint-enable @eslint-react/set-state-in-effect */ let nonReactTarget: WrapperType @@ -260,10 +278,15 @@ export const ReactSVG: React.ForwardRefExoticComponent< wrapper, ]) - // Keyed on `isLoading` rather than living in the injection effect, so that - // changing `loadingDelay` restarts the timer without re-running the + // Keyed on `injectionId` rather than living in the injection effect, so + // that changing `loadingDelay` restarts the timer without re-running the // injection. Only the timer lives here; a re-injection's flag is cleared by // the injection effect itself, which is a render earlier than this can run. + // + // `isLoading` is a dependency so the timer is cleared once an injection + // finishes, and `injectionId` so it re-arms for every injection - including + // one that starts while the previous request is still in flight, which + // leaves `isLoading` true the whole way through. React.useEffect(() => { if (!isLoading) { return @@ -285,7 +308,7 @@ export const ReactSVG: React.ForwardRefExoticComponent< return () => { clearTimeout(timeoutId) } - }, [isLoading, loadingDelay]) + }, [injectionId, isLoading, loadingDelay]) const Wrapper = wrapper diff --git a/test/browser.spec.tsx b/test/browser.spec.tsx index 198966835..7d8212c5e 100644 --- a/test/browser.spec.tsx +++ b/test/browser.spec.tsx @@ -395,6 +395,228 @@ describe('while running in a browser environment', () => { expect(loadingRenders).toBe(0) }) + // The four tests below walk the rest of the re-injection matrix. The one + // above only covers a re-injection that starts after the previous one + // finished, and which also changes `loadingDelay` - between them those two + // things move both of the delay effect's original dependencies, so they hid + // the case where neither moves. + + // A re-injection that starts while the previous request is still in flight + // leaves `isLoading` true throughout. With `loadingDelay` held constant - + // the usual way to pass it - nothing the delay effect reads as a prop + // changes, so the timer has to re-arm off the injection itself or the loader + // is suppressed for the whole of the second injection. + it('should re-arm loadingDelay for a re-injection that starts mid-flight', async () => { + const loading = () => loading + + faker.seed(193) + const first = faker.string.uuid() + const second = faker.string.uuid() + + nock('http://localhost') + .get(`/${first}.svg`) + .delay(1000) + .reply(200, source, { 'Content-Type': 'image/svg+xml' }) + .get(`/${second}.svg`) + .delay(600) + .reply(200, source, { 'Content-Type': 'image/svg+xml' }) + + const { rerender } = render( + , + ) + + // The first delay elapses well inside its own request, so the loader is on + // screen at the moment the second injection starts. + await waitFor(() => expect(screen.queryByText('loading')).not.toBeNull()) + + rerender( + , + ) + + // Cleared in the same commit the injection starts, so it is gone before + // the new delay begins. + expect(screen.queryByText('loading')).toBeNull() + + await waitFor(() => expect(screen.queryByText('loading')).not.toBeNull(), { + timeout: 400, + }) + }) + + // The previous injection's timer has to be cleared rather than left running. + // A surviving one fires against the old start time, so the loader appears + // before the new injection's own delay has elapsed. Asserts on elapsed time + // rather than presence, since both outcomes end with the loader on screen - + // only the timing separates them. + it('should not let one injection loadingDelay timer fire for the next', async () => { + const loading = () => loading + + faker.seed(194) + const first = faker.string.uuid() + const second = faker.string.uuid() + + nock('http://localhost') + .get(`/${first}.svg`) + .delay(2000) + .reply(200, source, { 'Content-Type': 'image/svg+xml' }) + .get(`/${second}.svg`) + .delay(2000) + .reply(200, source, { 'Content-Type': 'image/svg+xml' }) + + const { rerender } = render( + , + ) + + // Re-inject part-way through the first delay, so a surviving timer fires + // ~100ms later rather than a full 500ms. Overshooting this sleep on a slow + // machine costs nothing: the test then just becomes the case above. + await new Promise((resolve) => setTimeout(resolve, 400)) + + const reinjectedAt = Date.now() + + rerender( + , + ) + + await waitFor(() => expect(screen.queryByText('loading')).not.toBeNull(), { + timeout: 1200, + }) + + expect(Date.now() - reinjectedAt).toBeGreaterThanOrEqual(300) + }) + + it('should restart loadingDelay for a re-injection that leaves it unchanged', async () => { + const loading = () => loading + + faker.seed(195) + const first = faker.string.uuid() + const second = faker.string.uuid() + + nock('http://localhost') + .get(`/${first}.svg`) + .reply(200, source, { 'Content-Type': 'image/svg+xml' }) + .get(`/${second}.svg`) + .delay(600) + .reply(200, source, { 'Content-Type': 'image/svg+xml' }) + + const { container, rerender } = render( + , + ) + + await waitFor(() => + expect(container.querySelectorAll('.injected-svg')).toHaveLength(1), + ) + + rerender( + , + ) + + await waitFor(() => expect(screen.queryByText('loading')).not.toBeNull(), { + timeout: 400, + }) + }) + + it('should re-arm loadingDelay when a mid-flight re-injection changes it', async () => { + const loading = () => loading + + faker.seed(196) + const first = faker.string.uuid() + const second = faker.string.uuid() + + nock('http://localhost') + .get(`/${first}.svg`) + .delay(1000) + .reply(200, source, { 'Content-Type': 'image/svg+xml' }) + .get(`/${second}.svg`) + .delay(600) + .reply(200, source, { 'Content-Type': 'image/svg+xml' }) + + const { rerender } = render( + , + ) + + await waitFor(() => expect(screen.queryByText('loading')).not.toBeNull()) + + rerender( + , + ) + + await waitFor(() => expect(screen.queryByText('loading')).not.toBeNull(), { + timeout: 400, + }) + }) + + it('should restart loadingDelay for a re-injection after an error', async () => { + const fallback = () => fallback + const loading = () => loading + + faker.seed(197) + const first = faker.string.uuid() + const second = faker.string.uuid() + + nock('http://localhost') + .get(`/${first}.svg`) + .reply(404) + .get(`/${second}.svg`) + .delay(600) + .reply(200, source, { 'Content-Type': 'image/svg+xml' }) + + const { rerender } = render( + , + ) + + await waitFor(() => expect(screen.queryByText('fallback')).not.toBeNull()) + + rerender( + , + ) + + await waitFor(() => expect(screen.queryByText('loading')).not.toBeNull(), { + timeout: 400, + }) + }) + it('should render the loader immediately when loadingDelay is zero', async () => { const loading = () => loading diff --git a/test/manual/README.md b/test/manual/README.md index 03c128850..d0b7bba51 100644 --- a/test/manual/README.md +++ b/test/manual/README.md @@ -136,16 +136,17 @@ VoiceOver / macOS version: Recorded so a later run has something to compare against. -19.0.0 plus the `loadingDelay` work (eed92b7b), 2026-08-06, Safari 26.5 on macOS -15.7.7, VoiceOver with the caption panel open: - -| Case | Caption panel | DOM | -| ------------------------------------------------------- | ------------- | ------------------------------------------------ | -| 0 — existing live region, text changed | announces | n/a | -| 0b — `role="status"` element inserted already-populated | silent | n/a | -| A — cached remount, `role="status"` | silent | 8 elements, median 6.0ms, range 5.0-6.0, 0 requests | -| B — cached remount, plain span | silent | 8 elements, median 7.0ms, range 6.0-7.0, 0 requests | -| 4 — `loading`, `role="status"`, ~2.5s mounted | silent | 1 element, 2515.0ms | +19.0.0 plus the `loadingDelay` work and the mid-flight re-injection fix in this +commit, 2026-08-06, Safari 26.5 on macOS 15.7.7, VoiceOver with the caption +panel open: + +| Case | Caption panel | DOM | +| ------------------------------------------------------- | ------------- | --------------------------------------------------- | +| 0 — existing live region, text changed | announces | n/a | +| 0b — `role="status"` element inserted already-populated | silent | n/a | +| A — cached remount, `role="status"` | silent | 8 elements, median 7.0ms, range 6.0-7.0, 0 requests | +| B — cached remount, plain span | silent | 8 elements, median 8.0ms, range 7.0-8.0, 0 requests | +| 4 — `loading`, `role="status"`, ~2.5s mounted | silent | 1 element, 2514.0ms | **No announcement, and lifetime is not the variable.** VoiceOver announces a live region whose content changes and ignores one that arrives with its content @@ -154,21 +155,24 @@ the picture, so it is platform behaviour react-svg inherits. React mounts a `loading` component as a complete element, which is always the second shape, and a `role="status"` element mounted for a full 2.5 seconds was as silent as the millisecond-scale ones. Live-region semantics made no difference either: A and B -were equally silent. Unchanged from the 2026-08-04 run. +were equally silent. Unchanged across all three recorded runs. What this run does and does not cover. `loadingDelay` defaults to 0, so the default path mounts `loading` exactly as before, and that is the path every step here exercises - the harness never sets the prop. So this is a no-regression check, not coverage of the prop: a delay long enough to suppress the mount leaves no element to announce, which the DOM log settles without a screen -reader. No step changes `src` on a mounted component either, which is the path -the re-injection fix in eed92b7b corrects. - -Lifetimes came out longer than the 2026-08-04 run, B's median 7.0ms against -2.0ms. Both are the same shape - every cached remount mounts and unmounts the -element - and at this scale the figure tracks the machine and browser build -rather than anything in the package, so it is recorded rather than read as a -change. +reader. No step changes `src` on a mounted component either, so neither +re-injection path is exercised - not the one that starts after the previous +injection finished, and not the mid-flight one this commit fixes. A and B remount +a fresh tree instead. Covering those would need a step that swaps `src` on a live +component, which the harness does not have. + +Lifetimes drift by about a millisecond a run - B's median has gone 2.0ms, +7.0ms, 8.0ms across the three - while the shape never changes: every cached +remount mounts and unmounts the element. At this scale the figure tracks the +machine and browser build rather than anything in the package, so it is recorded +rather than read as a change. Re-run this against a different browser or screen reader, or if the mounting behaviour changes. From 994f337d57bca892b8ecfe5530764dc47c18325a Mon Sep 17 00:00:00 2001 From: Tane Morgan Date: Thu, 6 Aug 2026 06:52:57 +1200 Subject: [PATCH 5/5] Add a mid-flight re-injection probe to the manual harness Both Chrome measurements this branch has relied on came from a probe that was never committed, so neither could be re-run. This is that probe, as a step in the harness. It swaps `src` on a live component with `loadingDelay` set, which no other step does - the rest remount a fresh tree - and samples requestAnimationFrame, which is the question jsdom cannot answer at all: `loadingDelay` exists to keep a loader off the screen, and only a real browser paints. Two phases that control each other. `rearm` gives the second injection long enough that the loader has to come back; `suppress` gives it less than the delay, so the loader must stay down. A zero from `suppress` means nothing on its own, and means a good deal next to thirty from `rearm` in the same sitting. Recorded run is Chrome 151 on macOS, 30 runs a phase: rearm 30/30 mounted and painted, suppress 0/30 both, nothing left on screen at the end of either. Against dist/ built from b06a2cb4, the commit before the fix, rearm reads 0/30 - so the probe can see the regression rather than only agreeing with the current code. Needs no screen reader, so unlike the rest of the harness it reads the same however it is driven. --- test/manual/README.md | 51 ++++++++++++ test/manual/app.mjs | 181 +++++++++++++++++++++++++++++++++++++++++ test/manual/index.html | 11 +++ 3 files changed, 243 insertions(+) diff --git a/test/manual/README.md b/test/manual/README.md index d0b7bba51..bee00677e 100644 --- a/test/manual/README.md +++ b/test/manual/README.md @@ -91,6 +91,21 @@ actually reading. - **4 — slow cold load, `role="status"`.** A cold load held open for ~2.5 seconds, so the loading element is mounted for a human-scale stretch rather than a couple of milliseconds. +- **5 — mid-flight re-injection probe.** The odd one out, and the only step that + swaps `src` on a live component rather than remounting a fresh tree. Sets + `loadingDelay`, re-injects while the first request is still in flight, and + samples `requestAnimationFrame` to see whether a frame was ever painted with + the loader on screen. Two phases that control each other: `rearm` gives the + second injection long enough that the loader has to come back, `suppress` + gives it less than the delay so the loader must stay down. Needs no screen + reader, only a foregrounded tab — rAF stops in a background one. Takes about a + minute. + +Step 5 answers a question the rest of this harness cannot, and one jsdom cannot +either: `loadingDelay` exists to stop a loader reaching the screen, and only a +real browser paints. It is also the only step whose result does not depend on +who is running it, so it is the one worth re-running on any change to the delay +logic. Its result is recorded separately below. Step 4 is **not** the instrument check, though an earlier version of this harness treated it as one. It inserts an element that already carries @@ -177,6 +192,42 @@ rather than read as a change. Re-run this against a different browser or screen reader, or if the mounting behaviour changes. +## Last probe run + +Step 5 is recorded separately because it is a different instrument. It needs no +screen reader, only a foregrounded tab, so unlike the steps above it reads the +same however it is driven and can be re-run by anyone. + +Chrome 151 on macOS, 30 runs per phase, against `dist/` built from this commit: + +| Phase | Expectation | Mounted after the swap | Painted | Still up at the end | +| ---------- | -------------------------- | ---------------------- | ------- | ------------------- | +| `rearm` | the loader has to come back | 30/30 | 30/30 | 0/30 | +| `suppress` | the loader must stay down | 0/30 | 0/30 | 0/30 | + +**The two phases are each other's control.** `suppress` returning zero only +means something because `rearm` returned thirty in the same sitting: together +they say the probe could see a loader and still saw none where none belonged. +Run against `dist/` built from b06a2cb4, the commit before the fix, `rearm` +reads 0/30 instead - the loader never comes back for the second injection - +which is the regression the fix closes and the reason this step exists. + +Two figures were wrong before they were right, and both were the probe rather +than the package. Counting every frame after the swap reported `suppress` as +3/30 painted, because the loader already legitimately on screen keeps painting +for a frame or two while React commits the swap; the count now ignores any +element that did not arrive after the swap. And checking what was still on +screen the moment `.injected-svg` appeared reported `rearm` as 1/30 lingering, +because svg-injector inserts the SVG before it calls back, so React has not yet +committed `isLoading` false; the check now settles first. + +What this does not cover. Only the mid-flight re-injection path, and only the +paint question - whether assistive technology reacts to any of it is what steps +0 through 4 are for, and they still never set `loadingDelay`. The frame count +is a floor rather than a measurement: rAF samples at about 60Hz, so a mount +shorter than a frame can be real and go uncounted. Mounts are the sensitive +figure; frames only say whether one reached the screen. + The mechanics were re-checked in Chrome 151 on 2026-08-04, after the move here and to React from `node_modules`: all six steps ran, both cached remounts served 0 requests, the control held its loading element for 2509ms, and the console was diff --git a/test/manual/app.mjs b/test/manual/app.mjs index 93ccb3e83..9a7ba8afc 100644 --- a/test/manual/app.mjs +++ b/test/manual/app.mjs @@ -42,6 +42,12 @@ const statusLoading = (label) => () => const plainLoading = (label) => () => h('span', { 'data-loading': label }, `Loading ${label}`) +// Hoisted rather than built per render like the two above. Step 5 re-renders +// the same root to swap `src`, and a fresh component type each time would make +// React unmount and remount the loading element on its own - mutations the +// probe would then count as the thing it is measuring. +const probeLoading = statusLoading('probe') + const Grid = ({ makeLoading, srcs }) => h( 'div', @@ -63,6 +69,10 @@ const Grid = ({ makeLoading, srcs }) => const enteredAt = new Map() const lifetimes = [] +// Set while step 5 is running. That step does 30 runs a phase, so the +// per-element lines below would bury the result; it counts instead. +let probeRun = null + const noteLoadingNodes = (node, kind) => { if (node.nodeType !== Node.ELEMENT_NODE) { return @@ -72,6 +82,12 @@ const noteLoadingNodes = (node, kind) => { ...node.querySelectorAll('[data-loading]'), ] for (const element of matches) { + if (probeRun) { + if (kind === 'added' && probeRun.swapped) { + probeRun.mountsAfterSwap += 1 + } + continue + } const label = element.dataset.loading if (kind === 'added') { enteredAt.set(label, performance.now()) @@ -154,6 +170,134 @@ const resetRun = () => { enteredAt.clear() } +const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)) + +// Step 5's two phases. Both bring the loader up, then re-inject while the first +// request is still in flight and hold `loadingDelay` constant across the swap - +// the case where neither of the delay effect's dependencies moves, so nothing +// about the swap is visible to it unless the injection itself says so. +// +// They are each other's control. `rearm` gives the second injection long enough +// that the loader has to come back; `suppress` gives it less than the delay, so +// the loader must stay down. A zero from `suppress` only means something +// because `rearm` is non-zero in the same sitting: together they say the probe +// could see a loader and still saw none. +const PROBE_RUNS = 30 + +const PROBE_PHASES = [ + { + expectation: 'the loader has to come back', + firstMs: 600, + loadingDelay: 80, + name: 'rearm ', + secondMs: 600, + swapAt: 200, + }, + { + expectation: 'the loader must stay down', + firstMs: 800, + loadingDelay: 300, + name: 'suppress', + secondMs: 120, + swapAt: 400, + }, +] + +// Three figures per phase, and they answer different questions. +// +// `mounted` comes from the MutationObserver and catches an element however +// briefly it existed. `painted` comes from rAF sampling and says whether one +// was ever on screen at a paint; a mount the frame count misses is real but +// sub-frame, which is the whole reason this step runs in a browser rather than +// in jsdom, where only the first figure is available at all. +// +// `lingering` is the one that keeps the other two honest. Both of those only +// see an element that arrives after the swap, so a loader left over from +// before it - the failure the flag-clearing in the injection effect exists to +// prevent - would read as two clean zeroes. Checking what is still on screen +// when the run ends closes that off. +const runProbePhase = async (phase) => { + let lingering = 0 + let mounted = 0 + let painted = 0 + let voided = 0 + + const render = (src) => + root.render( + h(ReactSVG, { + loading: probeLoading, + loadingDelay: phase.loadingDelay, + src, + }), + ) + + for (let run = 0; run < PROBE_RUNS; run += 1) { + const stamp = `${phase.name.trim()}-${run}-${performance.now().toFixed(0)}` + + probeRun = { mountsAfterSwap: 0, swapped: false } + + root = createRoot(stage) + render(`/slow.svg?ms=${phase.firstMs}&n=${stamp}-a`) + + await sleep(phase.swapAt) + + // The loader has to be up before the swap, or there is no elapsed delay to + // carry across it and the run exercises nothing. + if (!stage.querySelector('[data-loading]')) { + voided += 1 + unmount() + continue + } + + let sampling = true + let frames = 0 + const sample = () => { + if (!sampling) { + return + } + // Only frames showing a loader that arrived after the swap. The one + // already on screen keeps painting for a frame or two while React + // commits the swap, and counting those would report a flash where there + // is nothing but the tail of a legitimate loader. + if ( + probeRun.mountsAfterSwap > 0 && + stage.querySelector('[data-loading]') + ) { + frames += 1 + } + requestAnimationFrame(sample) + } + + probeRun.swapped = true + requestAnimationFrame(sample) + render(`/slow.svg?ms=${phase.secondMs}&n=${stamp}-b`) + + await waitForInjections(1) + sampling = false + + // svg-injector inserts the SVG and only then calls back, so the injection + // is visible in the DOM a moment before React commits `isLoading` false + // and pulls the loader. Settle first, or this reads that gap as a loader + // the component failed to take down. + await sleep(100) + + if (stage.querySelector('[data-loading]')) { + lingering += 1 + } + if (probeRun.mountsAfterSwap > 0) { + mounted += 1 + } + if (frames > 0) { + painted += 1 + } + + unmount() + } + + probeRun = null + return { lingering, mounted, painted, voided } +} + const announcer = document.getElementById('announcer') const insertionPoint = document.getElementById('insertion-point') @@ -226,6 +370,43 @@ const steps = { log('Ready. Watch the caption panel, then run step 2.') }, + // The only step that swaps `src` on a live component rather than remounting a + // fresh tree, and the only one whose question needs a real browser: whether a + // frame was ever painted with the loader on screen. Needs no screen reader, + // so unlike the rest of this harness it reads the same however it is driven. + async probe() { + unmount() + resetRun() + clearLog(`Step 5: mid-flight re-injection, ${PROBE_RUNS} runs per phase.`) + log(' keep this tab foregrounded - rAF stops in a background tab') + + const results = [] + for (const phase of PROBE_PHASES) { + log(` ${phase.name} ${phase.expectation}`) + const result = await runProbePhase(phase) + results.push(result) + log( + ` mounted after the swap ${result.mounted}/${PROBE_RUNS}, ` + + `painted ${result.painted}/${PROBE_RUNS}, ` + + `still up at the end ${result.lingering}/${PROBE_RUNS}` + + (result.voided ? `, ${result.voided} void` : ''), + ) + } + + const [rearm, suppress] = results + log('') + log( + rearm.mounted === PROBE_RUNS && + suppress.mounted === 0 && + rearm.lingering === 0 && + suppress.lingering === 0 + ? ' PASS: the delay re-arms for the second injection, and still holds' + + ' back a loader the injection beats' + : ' FAIL: read the three figures against each other before believing' + + ' any of them', + ) + }, + async status() { await cachedRemount('A', statusLoading, 'role="status"') }, diff --git a/test/manual/index.html b/test/manual/index.html index 93cb32792..8d522fb7b 100644 --- a/test/manual/index.html +++ b/test/manual/index.html @@ -136,6 +136,14 @@

Does a briefly-mounted loading element announce?

loading elements.

+

+ Step 5 is the exception to all of that: it needs no screen reader, only a + foregrounded tab. It swaps src on a live component with + loadingDelay set and samples + requestAnimationFrame, which is the one question jsdom cannot + answer — whether a frame was ever painted with the loader on screen. +

+

Run the steps in order

+