Skip to content
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.

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

feat(trace-viewer): Add setting for display canvas content in snapshots #34010

Merged
merged 15 commits into from
Jan 8, 2025

Conversation

agg23
Copy link
Contributor

@agg23 agg23 commented Dec 13, 2024

As discussed in #33940, this PR adds a new setting to the Trace Viewer that allows users to toggle the error-prone psuedo-display of canvas content in snapshots. By default, canvas display is reverted to the previous functionality of displaying a checkered background. If enabled, the canvas content will be extracted from the browser screenshot and displayed.

Additionally adds a Settings dialog dropdown for Trace Viewer, as to group all settings.

Trace Viewer

image

UI Mode

image
image
image

This comment has been minimized.

else
canvas.title = `Canvas contents are displayed on a best-effort basis based on viewport screenshots taken during test execution.`;
} else {
canvas.title = 'Canvas content display is disabled.';
Copy link
Contributor Author

@agg23 agg23 Dec 16, 2024

Choose a reason for hiding this comment

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

Open to comments/suggestions on these strings

value: shouldPopulateCanvasFromScreenshot,
set: setShouldPopulateCanvasFromScreenshot,
name: 'Display canvas content',
title: 'Attempt to display the captured canvas appearance in the snapshot preview. May not be accurate.'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to comments/suggestions on these strings

Copy link
Member

Choose a reason for hiding this comment

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

"Best effort canvas element visualization." ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That string is very long. I'd prefer it be shorter so it fits better in the settings UI. I do like the content though.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

value: shouldPopulateCanvasFromScreenshot,
set: setShouldPopulateCanvasFromScreenshot,
name: 'Display canvas content',
title: 'Attempt to display the captured canvas appearance in the snapshot preview. May not be accurate.'
Copy link
Member

Choose a reason for hiding this comment

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

"Best effort canvas element visualization." ?

packages/trace-viewer/src/ui/recorder/recorderView.tsx Outdated Show resolved Hide resolved
packages/trace-viewer/src/ui/settingsToolbar.css Outdated Show resolved Hide resolved
packages/trace-viewer/src/ui/shared/dialog.tsx Outdated Show resolved Hide resolved
packages/trace-viewer/src/ui/settingsView.tsx Outdated Show resolved Hide resolved

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@agg23 agg23 requested a review from pavelfeldman January 6, 2025 18:15
tests/library/trace-viewer.spec.ts Outdated Show resolved Hide resolved
tests/library/trace-viewer.spec.ts Outdated Show resolved Hide resolved
packages/trace-viewer/src/ui/settingsToolbarButton.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Jan 7, 2025

Test results for "tests 1"

3 failed
❌ [installation tests] › tests/playwright-electron-should-work.spec.ts:21:5 › electron should work @package-installations-ubuntu-latest
❌ [installation tests] › tests/playwright-electron-should-work.spec.ts:31:5 › electron should work with special characters in path @package-installations-ubuntu-latest
❌ [installation tests] › tests/playwright-electron-should-work.spec.ts:44:5 › should work when wrapped inside @playwright/test and trace is enabled @package-installations-ubuntu-latest

8 flaky ⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [installation tests] › tests/playwright-electron-should-work.spec.ts:21:5 › electron should work @package-installations-macos-latest
⚠️ [playwright-test] › tests/ui-mode-test-ct.spec.ts:117:5 › should run component tests after editing test and component @ubuntu-latest-node20-1
⚠️ [webkit-library] › tests/library/browsercontext-clearcookies.spec.ts:92:3 › should remove cookies by domain @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/page-event-crash.spec.ts:67:1 › should cancel navigation when page crashes @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/proxy.spec.ts:44:3 › should use proxy for second page @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/selector-generator.spec.ts:463:5 › selector generator › should generate label selector @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-set-input-files.spec.ts:245:3 › should upload large file with relative path @webkit-ubuntu-22.04-node18

37535 passed, 654 skipped
✔️✔️✔️

Merge workflow run.

@agg23 agg23 merged commit ada68cd into microsoft:main Jan 8, 2025
27 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants