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..5e7bec271 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,27 @@ 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, + ) + + // 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 @@ -59,6 +81,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 @@ -97,11 +128,26 @@ 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 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 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 @@ -232,6 +278,38 @@ export const ReactSVG: React.ForwardRefExoticComponent< wrapper, ]) + // 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 + } + + /* 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) + } + }, [injectionId, isLoading, loadingDelay]) + const Wrapper = wrapper return ( @@ -245,7 +323,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..7d8212c5e 100644 --- a/test/browser.spec.tsx +++ b/test/browser.spec.tsx @@ -272,6 +272,402 @@ describe('while running in a browser environment', () => { ) }) + it('should hold the loader back until loadingDelay has elapsed', async () => { + const loading = () => loading + + faker.seed(190) + 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() + }) + + // 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) + }) + + // 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 + + faker.seed(191) + 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(192) + 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() diff --git a/test/manual/README.md b/test/manual/README.md index 9bc33bfe3..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 @@ -136,16 +151,17 @@ 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 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, 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 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 @@ -153,12 +169,65 @@ 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 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, 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. +## 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

+