From 3fe1263643b3b8590c63b0f4df677d174a626bc8 Mon Sep 17 00:00:00 2001 From: Simon Knott Date: Fri, 6 Sep 2024 16:24:33 +0200 Subject: [PATCH] feat(trace viewer): show Screenshot instead of Snapshot (#32248) Closes https://github.com/microsoft/playwright/issues/23964. Trace snapshots are a best-effort snapshots of the browser DOM, but we can't guarantee them to be exactly what the browser showed. One example of this is `canvas` elements, where you just can't see their contents. That makes snapshots useful, but not perfect. For those cases where the snapshot doesn't show everything, this PR introduces a new setting to show a screenshot instead. You won't be able to scroll or inspect the DOM or select a locator anymore. But if the snapshot was missing something, or displaying something wrong, you can now check the screenshot instead. --- packages/trace-viewer/src/entries.ts | 1 + packages/trace-viewer/src/snapshotServer.ts | 3 +- packages/trace-viewer/src/traceModernizer.ts | 1 + packages/trace-viewer/src/ui/inspectorTab.tsx | 6 ++- packages/trace-viewer/src/ui/modelUtil.ts | 6 +++ packages/trace-viewer/src/ui/snapshotTab.tsx | 38 +++++++++++++++---- packages/trace-viewer/src/ui/uiModeView.tsx | 3 ++ packages/trace-viewer/src/ui/workbench.tsx | 8 +++- packages/web/src/uiUtils.ts | 2 +- tests/config/traceViewerFixtures.ts | 6 +-- tests/library/trace-viewer.spec.ts | 30 +++++++++++++++ 11 files changed, 88 insertions(+), 16 deletions(-) diff --git a/packages/trace-viewer/src/entries.ts b/packages/trace-viewer/src/entries.ts index bca5751ab2..98d39927f7 100644 --- a/packages/trace-viewer/src/entries.ts +++ b/packages/trace-viewer/src/entries.ts @@ -41,6 +41,7 @@ export type ContextEntry = { }; export type PageEntry = { + pageId: string, screencastFrames: { sha1: string, timestamp: number, diff --git a/packages/trace-viewer/src/snapshotServer.ts b/packages/trace-viewer/src/snapshotServer.ts index b1dd371cb3..d41dcbdbbd 100644 --- a/packages/trace-viewer/src/snapshotServer.ts +++ b/packages/trace-viewer/src/snapshotServer.ts @@ -44,7 +44,8 @@ export class SnapshotServer { const snapshot = this._snapshot(pathname.substring('/snapshotInfo'.length), searchParams); return this._respondWithJson(snapshot ? { viewport: snapshot.viewport(), - url: snapshot.snapshot().frameUrl + url: snapshot.snapshot().frameUrl, + timestamp: snapshot.snapshot().timestamp, } : { error: 'No snapshot found' }); diff --git a/packages/trace-viewer/src/traceModernizer.ts b/packages/trace-viewer/src/traceModernizer.ts index b15ad527b0..7c6ff19ce6 100644 --- a/packages/trace-viewer/src/traceModernizer.ts +++ b/packages/trace-viewer/src/traceModernizer.ts @@ -58,6 +58,7 @@ export class TraceModernizer { let pageEntry = this._pageEntries.get(pageId); if (!pageEntry) { pageEntry = { + pageId, screencastFrames: [], }; this._pageEntries.set(pageId, pageEntry); diff --git a/packages/trace-viewer/src/ui/inspectorTab.tsx b/packages/trace-viewer/src/ui/inspectorTab.tsx index 7b3a90b709..ba37cf1bd1 100644 --- a/packages/trace-viewer/src/ui/inspectorTab.tsx +++ b/packages/trace-viewer/src/ui/inspectorTab.tsx @@ -17,7 +17,7 @@ import { CodeMirrorWrapper } from '@web/components/codeMirrorWrapper'; import type { Language } from '@web/components/codeMirrorWrapper'; import { ToolbarButton } from '@web/components/toolbarButton'; -import { copy } from '@web/uiUtils'; +import { copy, useSetting } from '@web/uiUtils'; import * as React from 'react'; import './sourceTab.css'; @@ -27,10 +27,12 @@ export const InspectorTab: React.FunctionComponent<{ highlightedLocator: string, setHighlightedLocator: (locator: string) => void, }> = ({ sdkLanguage, setIsInspecting, highlightedLocator, setHighlightedLocator }) => { + const [showScreenshot] = useSetting('screenshot-instead-of-snapshot', false); + return
Locator
- { + { // Updating text needs to go first - react can squeeze a render between the state updates. setHighlightedLocator(text); setIsInspecting(false); diff --git a/packages/trace-viewer/src/ui/modelUtil.ts b/packages/trace-viewer/src/ui/modelUtil.ts index c82cbc99a6..0c22d954f4 100644 --- a/packages/trace-viewer/src/ui/modelUtil.ts +++ b/packages/trace-viewer/src/ui/modelUtil.ts @@ -22,6 +22,7 @@ import type { ContextEntry, PageEntry } from '../entries'; import type { StackFrame } from '@protocol/channels'; const contextSymbol = Symbol('context'); +const pageSymbol = Symbol('page'); const nextInContextSymbol = Symbol('next'); const prevInListSymbol = Symbol('prev'); const eventsSymbol = Symbol('events'); @@ -148,6 +149,7 @@ function indexModel(context: ContextEntry) { for (let i = 0; i < context.actions.length; ++i) { const action = context.actions[i] as any; action[contextSymbol] = context; + action[pageSymbol] = context.pages.find(page => page.pageId === action.pageId); } let lastNonRouteAction = undefined; for (let i = context.actions.length - 1; i >= 0; i--) { @@ -356,6 +358,10 @@ export function prevInList(action: ActionTraceEvent): ActionTraceEvent { return (action as any)[prevInListSymbol]; } +export function pageForAction(action: ActionTraceEvent): PageEntry { + return (action as any)[pageSymbol]; +} + export function stats(action: ActionTraceEvent): { errors: number, warnings: number } { let errors = 0; let warnings = 0; diff --git a/packages/trace-viewer/src/ui/snapshotTab.tsx b/packages/trace-viewer/src/ui/snapshotTab.tsx index 4faa668677..229d9f8cef 100644 --- a/packages/trace-viewer/src/ui/snapshotTab.tsx +++ b/packages/trace-viewer/src/ui/snapshotTab.tsx @@ -17,10 +17,10 @@ import './snapshotTab.css'; import * as React from 'react'; import type { ActionTraceEvent } from '@trace/trace'; -import { context, prevInList } from './modelUtil'; +import { context, type MultiTraceModel, pageForAction, prevInList } from './modelUtil'; import { Toolbar } from '@web/components/toolbar'; import { ToolbarButton } from '@web/components/toolbarButton'; -import { clsx, useMeasure } from '@web/uiUtils'; +import { clsx, useMeasure, useSetting } from '@web/uiUtils'; import { InjectedScript } from '@injected/injectedScript'; import { Recorder } from '@injected/recorder/recorder'; import ConsoleAPI from '@injected/consoleApi'; @@ -30,8 +30,18 @@ import { locatorOrSelectorAsSelector } from '@isomorphic/locatorParser'; import { TabbedPaneTab } from '@web/components/tabbedPane'; import { BrowserFrame } from './browserFrame'; +function findClosest(items: T[], target: number) { + return items.find((item, index) => { + if (index === items.length - 1) + return true; + const next = items[index + 1]; + return Math.abs(item.timestamp - target) < Math.abs(next.timestamp - target); + }); +} + export const SnapshotTab: React.FunctionComponent<{ action: ActionTraceEvent | undefined, + model?: MultiTraceModel, sdkLanguage: Language, testIdAttributeName: string, isInspecting: boolean, @@ -39,9 +49,10 @@ export const SnapshotTab: React.FunctionComponent<{ highlightedLocator: string, setHighlightedLocator: (locator: string) => void, openPage?: (url: string, target?: string) => Window | any, -}> = ({ action, sdkLanguage, testIdAttributeName, isInspecting, setIsInspecting, highlightedLocator, setHighlightedLocator, openPage }) => { +}> = ({ action, model, sdkLanguage, testIdAttributeName, isInspecting, setIsInspecting, highlightedLocator, setHighlightedLocator, openPage }) => { const [measure, ref] = useMeasure(); const [snapshotTab, setSnapshotTab] = React.useState<'action'|'before'|'after'>('action'); + const [showScreenshotInsteadOfSnapshot] = useSetting('screenshot-instead-of-snapshot', false); type Snapshot = { action: ActionTraceEvent, snapshotName: string, point?: { x: number, y: number } }; const { snapshots } = React.useMemo(() => { @@ -90,7 +101,7 @@ export const SnapshotTab: React.FunctionComponent<{ const iframeRef0 = React.useRef(null); const iframeRef1 = React.useRef(null); - const [snapshotInfo, setSnapshotInfo] = React.useState({ viewport: kDefaultViewport, url: '' }); + const [snapshotInfo, setSnapshotInfo] = React.useState<{ viewport: typeof kDefaultViewport, url: string, timestamp?: number }>({ viewport: kDefaultViewport, url: '', timestamp: undefined }); const loadingRef = React.useRef({ iteration: 0, visibleIframe: 0 }); React.useEffect(() => { @@ -99,13 +110,14 @@ export const SnapshotTab: React.FunctionComponent<{ const newVisibleIframe = 1 - loadingRef.current.visibleIframe; loadingRef.current.iteration = thisIteration; - const newSnapshotInfo = { url: '', viewport: kDefaultViewport }; + const newSnapshotInfo = { url: '', viewport: kDefaultViewport, timestamp: undefined }; if (snapshotInfoUrl) { const response = await fetch(snapshotInfoUrl); const info = await response.json(); if (!info.error) { newSnapshotInfo.url = info.url; newSnapshotInfo.viewport = info.viewport; + newSnapshotInfo.timestamp = info.timestamp; } } @@ -154,6 +166,15 @@ export const SnapshotTab: React.FunctionComponent<{ y: (measure.height - snapshotContainerSize.height) / 2, }; + const page = action ? pageForAction(action) : undefined; + const screencastFrame = React.useMemo( + () => { + if (snapshotInfo.timestamp && page?.screencastFrames) + return findClosest(page.screencastFrames, snapshotInfo.timestamp); + }, + [page?.screencastFrames, snapshotInfo.timestamp] + ); + return
- setIsInspecting(!isInspecting)} /> + setIsInspecting(!isInspecting)} disabled={showScreenshotInsteadOfSnapshot} /> {['action', 'before', 'after'].map(tab => { return ; })}
- { + { if (!openPage) openPage = window.open; const win = openPage(popoutUrl || '', '_blank'); @@ -209,7 +230,8 @@ export const SnapshotTab: React.FunctionComponent<{ transform: `translate(${translate.x}px, ${translate.y}px) scale(${scale})`, }}> -
+ {(showScreenshotInsteadOfSnapshot && screencastFrame) && {`Screenshot ${renderTitle(snapshotTab)}`} src={`sha1/${screencastFrame.sha1}`} width={screencastFrame.width} height={screencastFrame.height} />} +
diff --git a/packages/trace-viewer/src/ui/uiModeView.tsx b/packages/trace-viewer/src/ui/uiModeView.tsx index 86e6bff80e..cad1c2eb58 100644 --- a/packages/trace-viewer/src/ui/uiModeView.tsx +++ b/packages/trace-viewer/src/ui/uiModeView.tsx @@ -107,6 +107,8 @@ export const UIModeView: React.FC<{}> = ({ const [updateSnapshots, setUpdateSnapshots] = React.useState(queryParams.updateSnapshots === 'all'); const [showRouteActions, setShowRouteActions] = useSetting('show-route-actions', true); const [darkMode, setDarkMode] = useDarkModeSetting(); + const [showScreenshot, setShowScreenshot] = useSetting('screenshot-instead-of-snapshot', false); + const inputRef = React.useRef(null); @@ -517,6 +519,7 @@ export const UIModeView: React.FC<{}> = ({ {settingsVisible && }
} diff --git a/packages/trace-viewer/src/ui/workbench.tsx b/packages/trace-viewer/src/ui/workbench.tsx index 491a8eac16..3565eb3850 100644 --- a/packages/trace-viewer/src/ui/workbench.tsx +++ b/packages/trace-viewer/src/ui/workbench.tsx @@ -73,6 +73,8 @@ export const Workbench: React.FunctionComponent<{ const [selectedTime, setSelectedTime] = React.useState(); const [sidebarLocation, setSidebarLocation] = useSetting<'bottom' | 'right'>('propertiesSidebarLocation', 'bottom'); const [showRouteActions, setShowRouteActions] = useSetting('show-route-actions', true); + const [showScreenshot, setShowScreenshot] = useSetting('screenshot-instead-of-snapshot', false); + const filteredActions = React.useMemo(() => { return (model?.actions || []).filter(action => showRouteActions || !isRouteAction(action)); @@ -299,7 +301,10 @@ export const Workbench: React.FunctionComponent<{ const settingsTab: TabbedPaneTabModel = { id: 'settings', title: 'Settings', - component: , + component: , }; return
@@ -325,6 +330,7 @@ export const Workbench: React.FunctionComponent<{ settingName='actionListSidebar' main={(name: string | undefined, defaultValue: S, title?: string): [S, React.Dispatch>] { +export function useSetting(name: string | undefined, defaultValue: S): [S, React.Dispatch>] { if (name) defaultValue = settings.getObject(name, defaultValue); const [value, setValue] = React.useState(defaultValue); diff --git a/tests/config/traceViewerFixtures.ts b/tests/config/traceViewerFixtures.ts index 7044e30a29..3b79a55e98 100644 --- a/tests/config/traceViewerFixtures.ts +++ b/tests/config/traceViewerFixtures.ts @@ -31,7 +31,7 @@ type BaseWorkerFixtures = { export type TraceViewerFixtures = { showTraceViewer: (trace: string[], options?: {host?: string, port?: number}) => Promise; - runAndTrace: (body: () => Promise) => Promise; + runAndTrace: (body: () => Promise, optsOverrides?: Parameters[0]) => Promise; }; class TraceViewerPage { @@ -127,9 +127,9 @@ export const traceViewerFixtures: Fixtures { - await use(async (body: () => Promise) => { + await use(async (body: () => Promise, optsOverrides = {}) => { const traceFile = testInfo.outputPath('trace.zip'); - await context.tracing.start({ snapshots: true, screenshots: true, sources: true }); + await context.tracing.start({ snapshots: true, screenshots: true, sources: true, ...optsOverrides }); await body(); await context.tracing.stop({ path: traceFile }); return showTraceViewer([traceFile]); diff --git a/tests/library/trace-viewer.spec.ts b/tests/library/trace-viewer.spec.ts index 4ebffc1f42..f8aaf9d0df 100644 --- a/tests/library/trace-viewer.spec.ts +++ b/tests/library/trace-viewer.spec.ts @@ -1465,3 +1465,33 @@ test('should serve css without content-type', async ({ page, runAndTrace, server const snapshotFrame = await traceViewer.snapshotFrame('page.goto'); await expect(snapshotFrame.locator('body')).toHaveCSS('background-color', 'rgb(255, 0, 0)', { timeout: 0 }); }); + +test('should allow showing screenshots instead of snapshots', async ({ runAndTrace, page, server }) => { + const traceViewer = await runAndTrace(async () => { + await page.goto(server.PREFIX + '/one-style.html'); + }); + + const screenshot = traceViewer.page.getByAltText(`Screenshot of page.goto > Action`); + const snapshot = (await traceViewer.snapshotFrame('page.goto')).owner(); + await expect(snapshot).toBeVisible(); + await expect(screenshot).not.toBeVisible(); + + await traceViewer.page.getByTitle('Settings').click(); + await traceViewer.page.getByText('Show screenshot instead of snapshot').setChecked(true); + + await expect(snapshot).not.toBeVisible(); + await expect(screenshot).toBeVisible(); +}); + +test('should handle case where neither snapshots nor screenshots exist', async ({ runAndTrace, page, server }) => { + const traceViewer = await runAndTrace(async () => { + await page.goto(server.PREFIX + '/one-style.html'); + }, { snapshots: false, screenshots: false }); + + await traceViewer.page.getByTitle('Settings').click(); + await traceViewer.page.getByText('Show screenshot instead of snapshot').setChecked(true); + + const screenshot = traceViewer.page.getByAltText(`Screenshot of page.goto > Action`); + await expect(screenshot).not.toBeVisible(); +}); +