feat(codegen): prefer frame name over url when unique (#5175)

src/server/supplements/recorder/codeGenerator.ts

@@ -18,10 +18,13 @@ import type { BrowserContextOptions, LaunchOptions } from '../../../..';
 import { Frame } from '../../frames';
 import { LanguageGenerator } from './language';
 import { Action, Signal } from './recorderActions';
+import { describeFrame } from './utils';
 
 export type ActionInContext = {
   pageAlias: string;
-  frame: Frame;
+  frameName?: string;
+  frameUrl: string;
+  isMainFrame: boolean;
   action: Action;
   committed?: boolean;
 }
@@ -124,7 +127,7 @@ export class CodeGenerator {
     if (signal.name === 'navigation') {
       this.addAction({
         pageAlias,
-        frame,
+        ...describeFrame(frame),
         committed: true,
         action: {
           name: 'navigate',

src/server/supplements/recorder/csharp.ts

@@ -24,7 +24,7 @@ import deviceDescriptors = require('../../deviceDescriptors');
 export class CSharpLanguageGenerator implements LanguageGenerator {
 
   generateAction(actionInContext: ActionInContext, performingAction: boolean): string {
-    const { action, pageAlias, frame } = actionInContext;
+    const { action, pageAlias } = actionInContext;
     const formatter = new CSharpFormatter(0);
     formatter.newLine();
     formatter.add('// ' + actionTitle(action));
@@ -36,8 +36,10 @@ export class CSharpLanguageGenerator implements LanguageGenerator {
       return formatter.format();
     }
 
-    const subject = !frame.parentFrame() ? pageAlias :
-      `${pageAlias}.GetFrame(url: '${frame.url()}')`;
+    const subject = actionInContext.isMainFrame ? pageAlias :
+      (actionInContext.frameName ?
+        `${pageAlias}.GetFrame(name: ${quote(actionInContext.frameName)})` :
+        `${pageAlias}.GetFrame(url: ${quote(actionInContext.frameUrl)})`);
 
     let navigationSignal: NavigationSignal | undefined;
     let popupSignal: PopupSignal | undefined;

src/server/supplements/recorder/javascript.ts

@@ -24,7 +24,7 @@ import deviceDescriptors = require('../../deviceDescriptors');
 export class JavaScriptLanguageGenerator implements LanguageGenerator {
 
   generateAction(actionInContext: ActionInContext, performingAction: boolean): string {
-    const { action, pageAlias, frame } = actionInContext;
+    const { action, pageAlias } = actionInContext;
     const formatter = new JavaScriptFormatter(2);
     formatter.newLine();
     formatter.add('// ' + actionTitle(action));
@@ -36,8 +36,10 @@ export class JavaScriptLanguageGenerator implements LanguageGenerator {
       return formatter.format();
     }
 
-    const subject = !frame.parentFrame() ? pageAlias :
-      `${pageAlias}.frame(${formatObject({ url: frame.url() })})`;
+    const subject = actionInContext.isMainFrame ? pageAlias :
+      (actionInContext.frameName ?
+        `${pageAlias}.frame(${formatObject({ name: actionInContext.frameName })})` :
+        `${pageAlias}.frame(${formatObject({ url: actionInContext.frameUrl })})`);
 
     let navigationSignal: NavigationSignal | undefined;
     let popupSignal: PopupSignal | undefined;

src/server/supplements/recorder/python.ts

@@ -33,7 +33,7 @@ export class PythonLanguageGenerator implements LanguageGenerator {
   }
 
   generateAction(actionInContext: ActionInContext, performingAction: boolean): string {
-    const { action, pageAlias, frame } = actionInContext;
+    const { action, pageAlias } = actionInContext;
     const formatter = new PythonFormatter(4);
     formatter.newLine();
     formatter.add('# ' + actionTitle(action));
@@ -45,8 +45,10 @@ export class PythonLanguageGenerator implements LanguageGenerator {
       return formatter.format();
     }
 
-    const subject = !frame.parentFrame() ? pageAlias :
-      `${pageAlias}.frame(${formatOptions({ url: frame.url() }, false)})`;
+    const subject = actionInContext.isMainFrame ? pageAlias :
+      (actionInContext.frameName ?
+        `${pageAlias}.frame(${formatOptions({ name: actionInContext.frameName }, false)})` :
+        `${pageAlias}.frame(${formatOptions({ url: actionInContext.frameUrl }, false)})`);
 
     let navigationSignal: NavigationSignal | undefined;
     let popupSignal: PopupSignal | undefined;

src/server/supplements/recorder/utils.ts

@@ -46,3 +46,13 @@ export function toModifiers(modifiers: number): ('Alt' | 'Control' | 'Meta' | 'S
     result.push('Shift');
   return result;
 }
+
+export function describeFrame(frame: Frame): { frameName?: string, frameUrl: string, isMainFrame: boolean } {
+  const page = frame._page;
+  if (page.mainFrame() === frame)
+    return { isMainFrame: true, frameUrl: frame.url() };
+  const frames = page.frames().filter(f => f.name() === frame.name());
+  if (frames.length === 1 && frames[0] === frame)
+    return { isMainFrame: false, frameUrl: frame.url(), frameName: frame.name() };
+  return { isMainFrame: false, frameUrl: frame.url() };
+}

src/server/supplements/recorderSupplement.ts

@@ -17,7 +17,7 @@
 import * as actions from './recorder/recorderActions';
 import type * as channels from '../../protocol/channels';
 import { CodeGenerator, ActionInContext } from './recorder/codeGenerator';
-import { toClickOptions, toModifiers } from './recorder/utils';
+import { describeFrame, toClickOptions, toModifiers } from './recorder/utils';
 import { Page } from '../page';
 import { Frame } from '../frames';
 import { BrowserContext } from '../browserContext';
@@ -151,7 +151,7 @@ export class RecorderSupplement {
       this._pageAliases.delete(page);
       this._generator.addAction({
         pageAlias,
-        frame: page.mainFrame(),
+        ...describeFrame(page.mainFrame()),
         committed: true,
         action: {
           name: 'closePage',
@@ -174,7 +174,7 @@ export class RecorderSupplement {
     if (!isPopup) {
       this._generator.addAction({
         pageAlias,
-        frame: page.mainFrame(),
+        ...describeFrame(page.mainFrame()),
         committed: true,
         action: {
           name: 'openPage',
@@ -190,7 +190,7 @@ export class RecorderSupplement {
     const controller = new ProgressController();
     const actionInContext: ActionInContext = {
       pageAlias: this._pageAliases.get(page)!,
-      frame,
+      ...describeFrame(frame),
       action
     };
     this._generator.willPerformAction(actionInContext);
@@ -218,10 +218,9 @@ export class RecorderSupplement {
   }
 
   private async _recordAction(frame: Frame, action: actions.Action) {
-    // We are lacking frame.page() in
     this._generator.addAction({
       pageAlias: this._pageAliases.get(frame._page)!,
-      frame,
+      ...describeFrame(frame),
       action
     });
   }

test/cli/cli-codegen.spec.ts

@@ -597,4 +597,45 @@ describe('cli codegen', (test, { browserName, headful }) => {
       page.click('input[id=checkbox]')
     ]);
   });
+
+  it('should prefer frame name', async ({ page, recorder, server }) => {
+    await recorder.setContentAndWait(`
+      <iframe src='./frames/frame.html' name='one'></iframe>
+      <iframe src='./frames/frame.html' name='two'></iframe>
+      <iframe src='./frames/frame.html'></iframe>
+    `, server.EMPTY_PAGE, 4);
+    const frameOne = page.frame({ name: 'one' });
+    const frameTwo = page.frame({ name: 'two' });
+    const otherFrame = page.frames().find(f => f !== page.mainFrame() && !f.name());
+
+    await Promise.all([
+      recorder.waitForOutput('one'),
+      frameOne.click('div'),
+    ]);
+    expect(recorder.output()).toContain(`
+  // Click text="Hi, I'm frame"
+  await page.frame({
+    name: 'one'
+  }).click('text="Hi, I\\'m frame"');`);
+
+    await Promise.all([
+      recorder.waitForOutput('two'),
+      frameTwo.click('div'),
+    ]);
+    expect(recorder.output()).toContain(`
+  // Click text="Hi, I'm frame"
+  await page.frame({
+    name: 'two'
+  }).click('text="Hi, I\\'m frame"');`);
+
+    await Promise.all([
+      recorder.waitForOutput('url: \''),
+      otherFrame.click('div'),
+    ]);
+    expect(recorder.output()).toContain(`
+  // Click text="Hi, I'm frame"
+  await page.frame({
+    url: '${otherFrame.url()}'
+  }).click('text="Hi, I\\'m frame"');`);
+  });
 });

test/cli/cli.fixtures.ts

@@ -127,15 +127,20 @@ class Recorder {
     this._actionPerformedCallback = () => { };
   }
 
-  async setContentAndWait(content: string, url: string = 'about:blank') {
-    await this.setPageContentAndWait(this.page, content, url);
+  async setContentAndWait(content: string, url: string = 'about:blank', frameCount: number = 1) {
+    await this.setPageContentAndWait(this.page, content, url, frameCount);
   }
 
-  async setPageContentAndWait(page: Page, content: string, url: string = 'about:blank') {
+  async setPageContentAndWait(page: Page, content: string, url: string = 'about:blank', frameCount: number = 1) {
     let callback;
     const result = new Promise(f => callback = f);
     await page.goto(url);
-    await page.exposeBinding('_recorderScriptReadyForTest', (source, arg) => callback(arg));
+    const frames = new Set<any>();
+    await page.exposeBinding('_recorderScriptReadyForTest', (source, arg) => {
+      frames.add(source.frame);
+      if (frames.size === frameCount)
+        callback(arg);
+    });
     await Promise.all([
       result,
       page.setContent(content)