Skip to content

fix(v10/feedback): Correct overlapping screenshot annotations and drag dimming #25046

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
msonnb merged 1 commit into from
Oct 5, 2026
+92 −2
Merged

fix(v10/feedback): Correct overlapping screenshot annotations and drag dimming #25046

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions dev-packages/browser-integration-tests/suites/feedback/screenshotEditor/init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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' })],
});
81 changes: 81 additions & 0 deletions dev-packages/browser-integration-tests/suites/feedback/screenshotEditor/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
import type { Page } from '@playwright/test';
import { expect } from '@playwright/test';
import { sentryTest } from '../../../utils/fixtures';
import { shouldSkipFeedbackTest } from '../../../utils/helpers';

async function drawBox(page: Page, from: { x: number; y: number }, to: { x: number; y: number }): Promise<void> {
await page.mouse.move(from.x, from.y);
await page.mouse.down();
await page.mouse.move(to.x, to.y, { steps: 3 });
await page.mouse.up();
}

sentryTest.beforeEach(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 });
Object.defineProperty(navigator, 'mediaDevices', {
value: {
getDisplayMedia: async () => {
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();
await expect.poll(() => page.locator('#background').evaluate((canvas: HTMLCanvasElement) => canvas.width)).toBe(1600);
});

sentryTest('starts a highlight at the cursor inside an existing box', async ({ page }) => {
const canvas = page.locator('#foreground');
const bounds = (await canvas.boundingBox())!;
const point = (x: number, y: number) => ({ x: bounds.x + x, y: bounds.y + y });
await drawBox(page, point(80, 60), point(280, 240));
await expect(page.locator('.editor__rect')).toHaveCount(1);
const start = point(160, 140);
const end = point(240, 200);

await drawBox(page, start, end);

await expect(page.locator('.editor__rect')).toHaveCount(2);
const box = (await page.locator('.editor__rect').nth(1).boundingBox())!;
expect(box.x).toBeCloseTo(start.x, 0);
expect(box.y).toBeCloseTo(start.y, 0);
expect(box.width).toBeCloseTo(end.x - start.x, 0);
expect(box.height).toBeCloseTo(end.y - start.y, 0);
});

sentryTest('keeps background dimming unchanged while dragging another box', async ({ page }) => {
const canvas = page.locator('#foreground');
const bounds = (await canvas.boundingBox())!;
const backgroundPixel = () =>
canvas.evaluate((element: HTMLCanvasElement) =>
Array.from(element.getContext('2d')!.getImageData(1400, 800, 1, 1).data),
);
await drawBox(page, { x: bounds.x + 80, y: bounds.y + 60 }, { x: bounds.x + 240, y: bounds.y + 180 });
await expect(page.locator('.editor__rect')).toHaveCount(1);
await expect.poll(backgroundPixel).toEqual([0, 0, 0, 64]);

await page.mouse.move(bounds.x + 280, bounds.y + 60);
await page.mouse.down();
await page.mouse.move(bounds.x + 360, bounds.y + 180, { steps: 3 });

expect(await backgroundPixel()).toEqual([0, 0, 0, 64]);
await page.mouse.up();
await expect(page.locator('.editor__rect')).toHaveCount(2);
await expect.poll(backgroundPixel).toEqual([0, 0, 0, 64]);
});
6 changes: 4 additions & 2 deletions packages/feedback/src/screenshot/components/ScreenshotEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ interface DrawCommand {
function drawRect(command: DrawCommand, ctx: CanvasRenderingContext2D, color: string): void {
switch (command.type) {
case 'highlight': {
ctx.save();
// creates a shadow around
ctx.shadowColor = 'rgba(0, 0, 0, 0.7)';
Comment on lines 51 to 55

@sentry sentry Bot Oct 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Canvas context state is not restored after drawing a highlight, causing shadow properties to leak and affect subsequent drawing operations like the background dimming overlay.
Severity: LOW

Suggested Fix

Wrap the drawing logic for highlights, specifically where ctx.shadowColor and ctx.shadowBlur are set, within ctx.save() and ctx.restore() calls. This will isolate the state changes to the highlight drawing operation and prevent them from affecting other parts of the canvas.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: packages/feedback/src/screenshot/components/ScreenshotEditor.tsx#L51-L55

Potential issue: Shadow properties like `shadowColor` and `shadowBlur`, set when drawing
a `highlight` annotation, are not reset. Because the canvas context is reused across
drawing operations, these shadow styles persist and are incorrectly applied to
subsequent drawings, such as the background dimming overlay. This results in unintended
visual artifacts on the screenshot editor.

Did we get this right? 👍 / 👎 to inform future reviews.

ctx.shadowBlur = 50;
Expand All @@ -60,6 +61,7 @@ function drawRect(command: DrawCommand, ctx: CanvasRenderingContext2D, color: st

// cut out the inside of the rectangle
ctx.clearRect(command.x, command.y, command.w, command.h);
ctx.restore();

break;
}
Expand Down Expand Up @@ -229,8 +231,8 @@ export function ScreenshotEditorFactory({
const boundingRect = mouseRef.current.getBoundingClientRect();
const startingPoint: DrawCommand = {
type: action,
x: e.offsetX / scaleFactor,
y: e.offsetY / scaleFactor,
x: (e.clientX - boundingRect.x) / scaleFactor,
y: (e.clientY - boundingRect.y) / scaleFactor,
w: 0,
h: 0,
Comment on lines +234 to 237

@sentry sentry Bot Oct 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Starting a new annotation by clicking on an existing one uses incorrect relative coordinates from e.offsetX/Y, causing the new shape to be misplaced.
Severity: MEDIUM

Suggested Fix

In the onMouseDown handler, calculate the starting coordinates relative to the parent overlay element instead of relying on e.offsetX/Y. This can be done using e.clientX and e.clientY along with the bounding rectangle of the overlay element to ensure a consistent coordinate system.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: packages/feedback/src/screenshot/components/ScreenshotEditor.tsx#L234-L237

Potential issue: When starting a new annotation by clicking inside an existing one, the
starting coordinates are calculated incorrectly. The `onMouseDown` handler uses
`e.offsetX` and `e.offsetY`, which are relative to the clicked element (`e.target`). If
the click is on an existing annotation `<div>`, the coordinates are relative to that
`div`. However, subsequent mouse move events calculate coordinates relative to the
parent overlay, causing a discrepancy that misplaces the newly drawn shape.

Did we get this right? 👍 / 👎 to inform future reviews.

};
Expand Down
Loading