chore(html): anchor by attachment index, not by attachment type

packages/html-reporter/src/chip.tsx

@@ -28,7 +28,7 @@ export const Chip: React.FC<{
   setExpanded?: (expanded: boolean) => void,
   children?: any,
   dataTestId?: string,
-  targetRef?: React.RefObject<HTMLDivElement>,
+  targetRef?: React.RefCallback<HTMLDivElement>,
 }> = ({ header, expanded, setExpanded, children, noInsets, dataTestId, targetRef }) => {
   return <div className='chip' data-testid={dataTestId} ref={targetRef}>
     <div
@@ -49,7 +49,7 @@ export const AutoChip: React.FC<{
   noInsets?: boolean,
   children?: any,
   dataTestId?: string,
-  targetRef?: React.RefObject<HTMLDivElement>,
+  targetRef?: React.RefCallback<HTMLDivElement>,
 }> = ({ header, initialExpanded, noInsets, children, dataTestId, targetRef }) => {
   const [expanded, setExpanded] = React.useState(initialExpanded || initialExpanded === undefined);
   return <Chip

packages/html-reporter/src/links.tsx

@@ -76,8 +76,9 @@ export const AttachmentLink: React.FunctionComponent<{
   href?: string,
   linkName?: string,
   openInNewTab?: boolean,
-}> = ({ attachment, href, linkName, openInNewTab }) => {
-  return <TreeItem title={<span>
+  targetRef?: React.RefCallback<HTMLDivElement>,
+}> = ({ attachment, href, targetRef, linkName, openInNewTab }) => {
+  return <TreeItem title={<span ref={targetRef}>
     {attachment.contentType === kMissingContentType ? icons.warning() : icons.attachment()}
     {attachment.path && <a href={href || attachment.path} download={downloadFileNameForAttachment(attachment)}>{linkName || attachment.name}</a>}
     {!attachment.path && (

packages/html-reporter/src/reportView.tsx

@@ -76,7 +76,7 @@ const TestCaseViewLoader: React.FC<{
   const searchParams = new URLSearchParams(window.location.hash.slice(1));
   const [test, setTest] = React.useState<TestCase | undefined>();
   const testId = searchParams.get('testId');
-  const anchor = (searchParams.get('anchor') || '') as 'video' | 'diff' | '';
+  const anchor = searchParams.has('anchor') ? Number(searchParams.get('anchor')!) : undefined;
   const run = +(searchParams.get('run') || '0');
 
   const testIdToFileIdMap = React.useMemo(() => {

packages/html-reporter/src/testCaseView.spec.tsx

@@ -63,7 +63,7 @@ const testCase: TestCase = {
 };
 
 test('should render test case', async ({ mount }) => {
-  const component = await mount(<TestCaseView projectNames={['chromium', 'webkit']} test={testCase} run={0} anchor=''></TestCaseView>);
+  const component = await mount(<TestCaseView projectNames={['chromium', 'webkit']} test={testCase} run={0}></TestCaseView>);
   await expect(component.getByText('Annotation text', { exact: false }).first()).toBeVisible();
   await expect(component.getByText('Hidden annotation')).toBeHidden();
   await component.getByText('Annotations').click();
@@ -79,7 +79,7 @@ test('should render test case', async ({ mount }) => {
 test('should render copy buttons for annotations', async ({ mount, page, context }) => {
   await context.grantPermissions(['clipboard-read', 'clipboard-write']);
 
-  const component = await mount(<TestCaseView projectNames={['chromium', 'webkit']} test={testCase} run={0} anchor=''></TestCaseView>);
+  const component = await mount(<TestCaseView projectNames={['chromium', 'webkit']} test={testCase} run={0}></TestCaseView>);
   await expect(component.getByText('Annotation text', { exact: false }).first()).toBeVisible();
   await component.getByText('Annotation text', { exact: false }).first().hover();
   await expect(component.locator('.test-case-annotation').getByLabel('Copy to clipboard').first()).toBeVisible();
@@ -108,7 +108,7 @@ const annotationLinkRenderingTestCase: TestCase = {
 };
 
 test('should correctly render links in annotations', async ({ mount }) => {
-  const component = await mount(<TestCaseView projectNames={['chromium', 'webkit']} test={annotationLinkRenderingTestCase} run={0} anchor=''></TestCaseView>);
+  const component = await mount(<TestCaseView projectNames={['chromium', 'webkit']} test={annotationLinkRenderingTestCase} run={0}></TestCaseView>);
 
   const firstLink = await component.getByText('https://playwright.dev/docs/intro').first();
   await expect(firstLink).toBeVisible();
@@ -166,7 +166,7 @@ const attachmentLinkRenderingTestCase: TestCase = {
 };
 
 test('should correctly render links in attachments', async ({ mount }) => {
-  const component = await mount(<TestCaseView projectNames={['chromium', 'webkit']} test={attachmentLinkRenderingTestCase} run={0} anchor=''></TestCaseView>);
+  const component = await mount(<TestCaseView projectNames={['chromium', 'webkit']} test={attachmentLinkRenderingTestCase} run={0}></TestCaseView>);
   await component.getByText('first attachment').click();
   const body = await component.getByText('The body with https://playwright.dev/docs/intro link');
   await expect(body).toBeVisible();
@@ -175,7 +175,7 @@ test('should correctly render links in attachments', async ({ mount }) => {
 });
 
 test('should correctly render links in attachment name', async ({ mount }) => {
-  const component = await mount(<TestCaseView projectNames={['chromium', 'webkit']} test={attachmentLinkRenderingTestCase} run={0} anchor=''></TestCaseView>);
+  const component = await mount(<TestCaseView projectNames={['chromium', 'webkit']} test={attachmentLinkRenderingTestCase} run={0}></TestCaseView>);
   const link = component.getByText('attachment with inline link').locator('a');
   await expect(link).toHaveAttribute('href', 'https://github.com/microsoft/playwright/issues/31284');
   await expect(link).toHaveText('https://github.com/microsoft/playwright/issues/31284');

packages/html-reporter/src/testCaseView.tsx

@@ -31,7 +31,7 @@ import { CopyToClipboardContainer } from './copyToClipboard';
 export const TestCaseView: React.FC<{
   projectNames: string[],
   test: TestCase | undefined,
-  anchor: 'video' | 'diff' | '',
+  anchor?: number,
   run: number,
 }> = ({ projectNames, test, run, anchor }) => {
   const [selectedResultIndex, setSelectedResultIndex] = React.useState(run);
@@ -69,7 +69,7 @@ export const TestCaseView: React.FC<{
       test.results.map((result, index) => ({
         id: String(index),
         title: <div style={{ display: 'flex', alignItems: 'center' }}>{statusIcon(result.status)} {retryLabel(index)}</div>,
-        render: () => <TestResultView test={test!} result={result} anchor={anchor}></TestResultView>
+        render: () => <TestResultView result={result} anchor={anchor}></TestResultView>
       })) || []} selectedTab={String(selectedResultIndex)} setSelectedTab={id => setSelectedResultIndex(+id)} />}
   </div>;
 };

packages/html-reporter/src/testFileView.tsx

@@ -14,7 +14,7 @@
   limitations under the License.
 */
 
-import type { HTMLReport, TestCaseSummary, TestFileSummary } from './types';
+import type { HTMLReport, TestAttachment, TestCaseSummary, TestFileSummary, TestResultSummary } from './types';
 import * as React from 'react';
 import { hashStringToInt, msToString } from './utils';
 import { Chip } from './chip';
@@ -70,16 +70,25 @@ export const TestFileView: React.FC<React.PropsWithChildren<{
   </Chip>;
 };
 
-function imageDiffBadge(test: TestCaseSummary): JSX.Element | undefined {
-  const resultWithImageDiff = test.results.find(result => result.attachments.some(attachment => {
-    return attachment.contentType.startsWith('image/') && !!attachment.name.match(/-(expected|actual|diff)/);
+function findResultWithAttachment(test: TestCaseSummary, predicate: (attachment: TestAttachment) => boolean): { index: number, anchor: number } | undefined {
+  let anchor = 0;
+  const index = test.results.findIndex(result => result.attachments.some((attachment, i) => {
+    if (predicate(attachment)) {
+      anchor = i;
+      return true;
+    }
   }));
-  return resultWithImageDiff ? <Link href={`#?testId=${test.testId}&anchor=diff&run=${test.results.indexOf(resultWithImageDiff)}`} title='View images' className='test-file-badge'>{image()}</Link> : undefined;
+  return index === -1 ? undefined : { index, anchor };
+}
+
+function imageDiffBadge(test: TestCaseSummary): JSX.Element | undefined {
+  const resultWithImageDiff = findResultWithAttachment(test, attachment => attachment.contentType.startsWith('image/') && !!attachment.name.match(/-(expected|actual|diff)/));
+  return resultWithImageDiff ? <Link href={`#?testId=${test.testId}&anchor=${resultWithImageDiff.anchor}&run=${resultWithImageDiff.index}`} title='View images' className='test-file-badge'>{image()}</Link> : undefined;
 }
 
 function videoBadge(test: TestCaseSummary): JSX.Element | undefined {
-  const resultWithVideo = test.results.find(result => result.attachments.some(attachment => attachment.name === 'video'));
-  return resultWithVideo ? <Link href={`#?testId=${test.testId}&anchor=video&run=${test.results.indexOf(resultWithVideo)}`} title='View video' className='test-file-badge'>{video()}</Link> : undefined;
+  const resultWithVideo = findResultWithAttachment(test, attachment => attachment.name === 'video');
+  return resultWithVideo ? <Link href={`#?testId=${test.testId}&anchor=${resultWithVideo.anchor}&run=${resultWithVideo.index}`} title='View video' className='test-file-badge'>{video()}</Link> : undefined;
 }
 
 function traceBadge(test: TestCaseSummary): JSX.Element | undefined {

packages/html-reporter/src/testResultView.tsx

@@ -14,7 +14,7 @@
   limitations under the License.
 */
 
-import type { TestAttachment, TestCase, TestResult, TestStep } from './types';
+import type { TestAttachment, TestResult, TestStep } from './types';
 import * as React from 'react';
 import { TreeItem } from './treeItem';
 import { msToString } from './utils';
@@ -61,10 +61,24 @@ function groupImageDiffs(screenshots: Set<TestAttachment>): ImageDiff[] {
   return [...snapshotNameToImageDiff.values()];
 }
 
+function getDiffAttachmentRepresentative(diff: ImageDiff, result: TestResult): TestAttachment {
+  let representative: TestAttachment | undefined = undefined;
+  let min = Number.MAX_VALUE;
+  for (const attachment of [diff.actual?.attachment, diff.diff?.attachment, diff.expected?.attachment]) {
+    if (!attachment)
+      continue;
+    const index = result.attachments.indexOf(attachment);
+    if (index < min) {
+      min = index;
+      representative = attachment;
+    }
+  }
+  return representative!;
+}
+
 export const TestResultView: React.FC<{
-  test: TestCase,
   result: TestResult,
-  anchor: 'video' | 'diff' | '',
+  anchor?: number,
 }> = ({ result, anchor }) => {
 
   const { screenshots, videos, traces, otherAttachments, diffs, errors, htmls } = React.useMemo(() => {
@@ -80,19 +94,24 @@ export const TestResultView: React.FC<{
     return { screenshots: [...screenshots], videos, traces, otherAttachments, diffs, errors, htmls };
   }, [result]);
 
-  const videoRef = React.useRef<HTMLDivElement>(null);
-  const imageDiffRef = React.useRef<HTMLDivElement>(null);
-
-  const [scrolled, setScrolled] = React.useState(false);
+  const attachmentRefs = React.useRef<HTMLElement[]>([]);
   React.useEffect(() => {
-    if (scrolled)
+    if (anchor === undefined)
       return;
-    setScrolled(true);
-    if (anchor === 'video')
-      videoRef.current?.scrollIntoView({ block: 'start', inline: 'start' });
-    if (anchor === 'diff')
-      imageDiffRef.current?.scrollIntoView({ block: 'start', inline: 'start' });
-  }, [scrolled, anchor, setScrolled, videoRef]);
+
+    const attachmentRef = attachmentRefs.current[anchor];
+    if (!attachmentRef)
+      return;
+
+    attachmentRef.scrollIntoView({ block: 'start', inline: 'start' });
+  }, [attachmentRefs, anchor]);
+  const targetRef = (attachment: TestAttachment): React.RefCallback<HTMLElement> => el => {
+    const index = result.attachments.indexOf(attachment);
+    if (el)
+      attachmentRefs.current[index] = el;
+    else
+      delete attachmentRefs.current[index];
+  };
 
   return <div className='test-result'>
     {!!errors.length && <AutoChip header='Errors'>
@@ -107,7 +126,7 @@ export const TestResultView: React.FC<{
     </AutoChip>}
 
     {diffs.map((diff, index) =>
-      <AutoChip key={`diff-${index}`} dataTestId='test-results-image-diff' header={`Image mismatch: ${diff.name}`} targetRef={imageDiffRef}>
+      <AutoChip key={`diff-${index}`} dataTestId='test-results-image-diff' header={`Image mismatch: ${diff.name}`} targetRef={targetRef(getDiffAttachmentRepresentative(diff, result))}>
         <ImageDiffView key='image-diff' diff={diff}></ImageDiffView>
       </AutoChip>
     )}
@@ -118,7 +137,7 @@ export const TestResultView: React.FC<{
           <a href={a.path}>
             <img className='screenshot' src={a.path} />
           </a>
-          <AttachmentLink attachment={a}></AttachmentLink>
+          <AttachmentLink attachment={a} targetRef={targetRef(a)}></AttachmentLink>
         </div>;
       })}
     </AutoChip>}
@@ -132,12 +151,12 @@ export const TestResultView: React.FC<{
       </div>}
     </AutoChip>}
 
-    {!!videos.length && <AutoChip header='Videos' targetRef={videoRef}>
+    {!!videos.length && <AutoChip header='Videos'>
       {videos.map((a, i) => <div key={`video-${i}`}>
         <video controls>
           <source src={a.path} type={a.contentType}/>
         </video>
-        <AttachmentLink attachment={a}></AttachmentLink>
+        <AttachmentLink attachment={a} targetRef={targetRef(a)}/>
       </div>)}
     </AutoChip>}
 
@@ -145,7 +164,7 @@ export const TestResultView: React.FC<{
       {[...htmls].map((a, i) => (
         <AttachmentLink key={`html-link-${i}`} attachment={a} openInNewTab />)
       )}
-      {[...otherAttachments].map((a, i) => <AttachmentLink key={`attachment-link-${i}`} attachment={a}></AttachmentLink>)}
+      {[...otherAttachments].map((a, i) => <AttachmentLink key={`attachment-link-${i}`} attachment={a} targetRef={targetRef(a)}></AttachmentLink>)}
     </AutoChip>}
   </div>;
 };

tests/playwright-test/reporter-html.spec.ts

@@ -175,8 +175,9 @@ for (const useIntermediateMergeReport of [true, false] as const) {
       expect(result.failed).toBe(1);
 
       await showReport();
-      await page.click('text=fails');
-      await expect(page.locator('text=Image mismatch')).toBeVisible();
+
+      await page.locator('.test-file-test').filter({ hasText: 'fails' }).getByTitle('View images').click();
+      await expect(page.locator('text=Image mismatch')).toBeInViewport();
       await expect(page.locator('text=Snapshot mismatch')).toHaveCount(0);
 
       await expect(page.getByTestId('test-screenshot-error-view').getByTestId('test-result-image-mismatch-tabs').locator('div')).toHaveText([