From 5115fb8331c9f1cc7b27e6fa1779781567c9c70d Mon Sep 17 00:00:00 2001 From: larcane97 <70624819+larcane97@users.noreply.github.com> Date: Fri, 23 Oct 2026 00:58:58 +0900 Subject: [PATCH 1/2] fix(feedback): Wait for frames without the form before capturing the screenshot The screenshot editor hides the feedback form and then calls `getDisplayMedia` in the same task. The capture starts from the last frame the browser painted, which on a busy machine can still be a picture of the form, so the form ends up in the screenshot that is sent with the feedback. Wait for two animation frames after the form is hidden before requesting the capture. Give up after one second so a tab that draws no frames (e.g. a background tab) does not hold the screenshot forever. Fixes #25657 Co-Authored-By: Claude Sonnet 5.5 --- .../suites/feedback/screenshotCapture/init.js | 7 ++ .../suites/feedback/screenshotCapture/test.ts | 56 +++++++++++++++ .../components/useTakeScreenshot.tsx | 25 +++++++ .../components/useTakeScreenshot.test.ts | 71 +++++++++++++++++++ 4 files changed, 159 insertions(+) create mode 100644 dev-packages/browser-integration-tests/suites/feedback/screenshotCapture/init.js create mode 100644 dev-packages/browser-integration-tests/suites/feedback/screenshotCapture/test.ts create mode 100644 packages/feedback/test/screenshot/components/useTakeScreenshot.test.ts diff --git a/dev-packages/browser-integration-tests/suites/feedback/screenshotCapture/init.js b/dev-packages/browser-integration-tests/suites/feedback/screenshotCapture/init.js new file mode 100644 index 000000000000..94453fb8e47a --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/feedback/screenshotCapture/init.js @@ -0,0 +1,7 @@ +import * as Sentry from '@sentry/browser'; +import { feedbackIntegration } from '@sentry/browser'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + integrations: [feedbackIntegration({ colorScheme: 'light' })], +}); diff --git a/dev-packages/browser-integration-tests/suites/feedback/screenshotCapture/test.ts b/dev-packages/browser-integration-tests/suites/feedback/screenshotCapture/test.ts new file mode 100644 index 000000000000..2a4a25f8ca75 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/feedback/screenshotCapture/test.ts @@ -0,0 +1,56 @@ +import { expect } from '@playwright/test'; +import { sentryTest } from '../../../utils/fixtures'; +import { shouldSkipFeedbackTest } from '../../../utils/helpers'; + +type CaptureWindow = Window & { framesWithoutFormAtCapture?: number }; + +// Whether the capture still shows the form depends on how busy the machine is, so this counts the frames the page +// drew with the form hidden by the time the capture is requested instead. +sentryTest( + 'draws two frames without the feedback form before it asks for the screen capture', + async ({ getLocalTestUrl, page, browserName }) => { + sentryTest.skip(shouldSkipFeedbackTest()); + sentryTest.skip(browserName !== 'chromium', 'The canvas screen-capture fixture requires Chromium.'); + + await page.addInitScript(() => { + // The fixture runs over HTTP, but screenshot capture is stubbed below. + Object.defineProperty(window, 'isSecureContext', { value: true }); + + let framesWithoutForm = 0; + const countFrame = (): void => { + const form = document + .getElementById('sentry-feedback') + ?.shadowRoot?.querySelector('dialog.dialog')?.parentElement; + if (form?.style.display === 'none') { + framesWithoutForm += 1; + } + requestAnimationFrame(countFrame); + }; + requestAnimationFrame(countFrame); + + Object.defineProperty(navigator, 'mediaDevices', { + value: { + getDisplayMedia: async () => { + (window as CaptureWindow).framesWithoutFormAtCapture = framesWithoutForm; + const canvas = document.createElement('canvas'); + canvas.width = 1600; + canvas.height = 1000; + const ctx = canvas.getContext('2d')!; + ctx.fillStyle = 'white'; + ctx.fillRect(0, 0, canvas.width, canvas.height); + return canvas.captureStream(); + }, + }, + configurable: true, + }); + }); + + await page.goto(await getLocalTestUrl({ testDir: __dirname, handleLazyLoadedFeedback: true })); + await page.getByRole('button', { name: 'Report a Bug', exact: true }).click(); + await page.getByRole('button', { name: 'Add a screenshot', exact: true }).click(); + await expect(page.locator('#foreground')).toBeVisible(); + + const framesWithoutForm = await page.evaluate(() => (window as CaptureWindow).framesWithoutFormAtCapture); + expect(framesWithoutForm).toBeGreaterThanOrEqual(2); + }, +); diff --git a/packages/feedback/src/screenshot/components/useTakeScreenshot.tsx b/packages/feedback/src/screenshot/components/useTakeScreenshot.tsx index 220c2fdaa4b0..c4ad8e4ad814 100644 --- a/packages/feedback/src/screenshot/components/useTakeScreenshot.tsx +++ b/packages/feedback/src/screenshot/components/useTakeScreenshot.tsx @@ -14,6 +14,27 @@ interface Props { type UseTakeScreenshot = ({ onBeforeScreenshot, onScreenshot, onAfterScreenshot, onError }: Props) => void; +const FRAMES_BEFORE_CAPTURE = 2; +const MAX_WAIT_BEFORE_CAPTURE_MS = 1000; + +/** Resolves once `frames` animation frames have been drawn, or after `timeout` ms if the page draws none. */ +function waitForFrames(frames: number, timeout: number): Promise { + return new Promise(resolve => { + const timeoutId = setTimeout(resolve, timeout); + let remaining = frames; + const onFrame = (): void => { + remaining -= 1; + if (remaining > 0) { + WINDOW.requestAnimationFrame(onFrame); + return; + } + clearTimeout(timeoutId); + resolve(); + }; + WINDOW.requestAnimationFrame(onFrame); + }); +} + export function useTakeScreenshotFactory({ hooks }: FactoryParams): UseTakeScreenshot { function useDpi(): number { const [dpi, setDpi] = hooks.useState(WINDOW.devicePixelRatio ?? 1); @@ -38,6 +59,10 @@ export function useTakeScreenshotFactory({ hooks }: FactoryParams): UseTakeScree const takeScreenshot = async (): Promise => { onBeforeScreenshot(); + // The capture starts from the last frame the browser painted, which can still show the form that + // `onBeforeScreenshot` just hid. A tab that draws no frames (e.g. a background tab) must not hold it forever. + await waitForFrames(FRAMES_BEFORE_CAPTURE, MAX_WAIT_BEFORE_CAPTURE_MS); + // Chrome will animate a top-bar which can shrink the window height by a // few pixels. The exact amount depends on how fast it takes to exec // the onloadedmetadata callback. diff --git a/packages/feedback/test/screenshot/components/useTakeScreenshot.test.ts b/packages/feedback/test/screenshot/components/useTakeScreenshot.test.ts new file mode 100644 index 000000000000..753bf78c5300 --- /dev/null +++ b/packages/feedback/test/screenshot/components/useTakeScreenshot.test.ts @@ -0,0 +1,71 @@ +/** + * @vitest-environment jsdom + */ +import { h, render } from 'preact'; +import * as hooks from 'preact/hooks'; +import { act } from 'preact/test-utils'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { useTakeScreenshotFactory } from '../../../src/screenshot/components/useTakeScreenshot'; + +describe('useTakeScreenshot', () => { + const getDisplayMedia = vi.fn(); + const onBeforeScreenshot = vi.fn(); + const onError = vi.fn(); + let pendingFrames: FrameRequestCallback[] = []; + + /** Runs the callbacks queued so far, like the browser does at the start of one frame. */ + async function drawFrame(): Promise { + pendingFrames.splice(0).forEach(callback => callback(0)); + await vi.advanceTimersByTimeAsync(0); + } + + function mountHook(): void { + const useTakeScreenshot = useTakeScreenshotFactory({ hooks }); + const Probe = (): null => { + useTakeScreenshot({ onBeforeScreenshot, onScreenshot: vi.fn(), onAfterScreenshot: vi.fn(), onError }); + return null; + }; + act(() => { + render(h(Probe, null), document.body); + }); + } + + beforeEach(() => { + pendingFrames = []; + getDisplayMedia.mockRejectedValue(new Error('NotAllowedError')); + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }); + vi.stubGlobal('requestAnimationFrame', (callback: FrameRequestCallback) => pendingFrames.push(callback)); + vi.stubGlobal('matchMedia', () => ({ addEventListener: vi.fn(), removeEventListener: vi.fn() })); + Object.defineProperty(navigator, 'mediaDevices', { value: { getDisplayMedia }, configurable: true }); + }); + + afterEach(() => { + render(null, document.body); + vi.unstubAllGlobals(); + vi.useRealTimers(); + vi.clearAllMocks(); + }); + + it('requests the capture only after the page has drawn two frames without the form', async () => { + mountHook(); + + expect(onBeforeScreenshot).toHaveBeenCalledTimes(1); + expect(getDisplayMedia).not.toHaveBeenCalled(); + + await drawFrame(); + expect(getDisplayMedia).not.toHaveBeenCalled(); + + await drawFrame(); + expect(getDisplayMedia).toHaveBeenCalledTimes(1); + }); + + it('requests the capture after one second when the page draws no frames', async () => { + mountHook(); + + await vi.advanceTimersByTimeAsync(999); + expect(getDisplayMedia).not.toHaveBeenCalled(); + + await vi.advanceTimersByTimeAsync(1); + expect(getDisplayMedia).toHaveBeenCalledTimes(1); + }); +}); From 1d29c9866d7c5ab5eb588ea1c2aa29ed04ffb387 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 23 Oct 2026 10:58:50 +0200 Subject: [PATCH 2/2] fix(feedback): Lower screenshot frame wait timeout to 300ms Safari only lets getDisplayMedia use the click's user gesture for 1s after the click. With a 1s fallback timeout, the capture always ran after the gesture expired and Safari rejected it with InvalidStateError. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../feedback/src/screenshot/components/useTakeScreenshot.tsx | 3 ++- .../test/screenshot/components/useTakeScreenshot.test.ts | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/feedback/src/screenshot/components/useTakeScreenshot.tsx b/packages/feedback/src/screenshot/components/useTakeScreenshot.tsx index c4ad8e4ad814..19ef97edd32e 100644 --- a/packages/feedback/src/screenshot/components/useTakeScreenshot.tsx +++ b/packages/feedback/src/screenshot/components/useTakeScreenshot.tsx @@ -15,7 +15,8 @@ interface Props { type UseTakeScreenshot = ({ onBeforeScreenshot, onScreenshot, onAfterScreenshot, onError }: Props) => void; const FRAMES_BEFORE_CAPTURE = 2; -const MAX_WAIT_BEFORE_CAPTURE_MS = 1000; +// Safari drops the user gesture that `getDisplayMedia` needs 1s after the click, so stay well below that. +const MAX_WAIT_BEFORE_CAPTURE_MS = 300; /** Resolves once `frames` animation frames have been drawn, or after `timeout` ms if the page draws none. */ function waitForFrames(frames: number, timeout: number): Promise { diff --git a/packages/feedback/test/screenshot/components/useTakeScreenshot.test.ts b/packages/feedback/test/screenshot/components/useTakeScreenshot.test.ts index 753bf78c5300..308deeead7b3 100644 --- a/packages/feedback/test/screenshot/components/useTakeScreenshot.test.ts +++ b/packages/feedback/test/screenshot/components/useTakeScreenshot.test.ts @@ -59,10 +59,10 @@ describe('useTakeScreenshot', () => { expect(getDisplayMedia).toHaveBeenCalledTimes(1); }); - it('requests the capture after one second when the page draws no frames', async () => { + it('requests the capture after 300ms when the page draws no frames', async () => { mountHook(); - await vi.advanceTimersByTimeAsync(999); + await vi.advanceTimersByTimeAsync(299); expect(getDisplayMedia).not.toHaveBeenCalled(); await vi.advanceTimersByTimeAsync(1);