feat(inspector): scope pickLocator mode to the owning page only (microsoft#39631)

Author: yury-s

packages/dashboard/src/dashboard.tsx

@@ -44,7 +44,7 @@ export const Dashboard: React.FC<{ wsUrl?: string }> = ({ wsUrl }) => {
   const [url, setUrl] = React.useState('');
   const [frame, setFrame] = React.useState<DashboardChannelEvents['frame']>();
   const [showInspector, setShowInspector] = React.useState(false);
-  const [picking, setPicking] = React.useState(false);
+  const [pickingTabId, setPickingTabId] = React.useState<string | null>(null);
   const [locatorToast, setLocatorToast] = React.useState<{ text: string; timer: ReturnType<typeof setTimeout> }>();
 
   const [channel, setChannel] = React.useState<DashboardClientChannel | undefined>();
@@ -62,7 +62,7 @@ export const Dashboard: React.FC<{ wsUrl?: string }> = ({ wsUrl }) => {
     channel.onopen = () => {
       setChannel(channel);
       setInteractive(false);
-      setPicking(false);
+      setPickingTabId(null);
     };
 
     channel.on('tabs', params => {
@@ -92,7 +92,7 @@ export const Dashboard: React.FC<{ wsUrl?: string }> = ({ wsUrl }) => {
     channel.on('elementPicked', params => {
       const locator = asLocator('javascript', params.selector);
       navigator.clipboard?.writeText(locator).catch(() => {});
-      setPicking(false);
+      setPickingTabId(null);
       setLocatorToast(old => {
         clearTimeout(old?.timer);
         return { text: locator, timer: setTimeout(() => setLocatorToast(undefined), 3000) };
@@ -102,7 +102,7 @@ export const Dashboard: React.FC<{ wsUrl?: string }> = ({ wsUrl }) => {
     channel.onclose = () => {
       setChannel(undefined);
       setInteractive(false);
-      setPicking(false);
+      setPickingTabId(null);
       setShowInspector(false);
     };
 
@@ -185,10 +185,10 @@ export const Dashboard: React.FC<{ wsUrl?: string }> = ({ wsUrl }) => {
   }
 
   function onScreenKeyDown(e: React.KeyboardEvent) {
-    if (picking && e.key === 'Escape') {
+    if (pickingTabId !== null && e.key === 'Escape') {
       e.preventDefault();
       channel?.cancelPickLocator();
-      setPicking(false);
+      setPickingTabId(null);
       return;
     }
     if (!interactive)
@@ -216,6 +216,7 @@ export const Dashboard: React.FC<{ wsUrl?: string }> = ({ wsUrl }) => {
   }
 
   const selectedTab = tabs?.find(t => t.selected);
+  const picking = selectedTab?.pageId === pickingTabId;
 
   let overlayText: string | undefined;
   if (!channel)
@@ -270,7 +271,7 @@ export const Dashboard: React.FC<{ wsUrl?: string }> = ({ wsUrl }) => {
             title='Read-only mode'
             onClick={() => {
               channel?.cancelPickLocator();
-              setPicking(false);
+              setPickingTabId(null);
               setShowInspector(false);
               setInteractive(false);
             }}
@@ -319,15 +320,15 @@ export const Dashboard: React.FC<{ wsUrl?: string }> = ({ wsUrl }) => {
         title='Pick locator'
         aria-pressed={picking}
         disabled={!channel}
-        onClick={async () => {
+        onClick={() => {
           if (picking) {
-            await channel?.cancelPickLocator();
-            setPicking(false);
+            channel?.cancelPickLocator();
+            setPickingTabId(null);
           } else {
             setInteractive(true);
-            await channel?.pickLocator();
-            setPicking(true);
+            setPickingTabId(selectedTab?.pageId ?? null);
             screenRef.current?.focus();
+            channel?.pickLocator();
           }
         }}
       >

packages/playwright-core/src/server/dispatchers/pageDispatcher.ts

@@ -354,7 +354,7 @@ export class PageDispatcher extends Dispatcher<Page, channels.PageChannel, Brows
 
   async pickLocator(params: channels.PagePickLocatorParams, progress: Progress): Promise<channels.PagePickLocatorResult> {
     const recorder = await Recorder.forContext(this._page.browserContext, { omitCallTracking: true, hideToolbar: true });
-    const selector = await recorder.pickLocator(progress);
+    const selector = await recorder.pickLocator(progress, this._page);
     return { selector };
   }
 

packages/playwright-core/src/server/recorder.ts

@@ -94,6 +94,7 @@ export class Recorder extends EventEmitter<RecorderEventMap> implements Instrume
   private _listeners: RegisteredListener[] = [];
   private _enabled: boolean = false;
   private _callLogs: CallLog[] = [];
+  private _pickLocatorPage: Page | undefined;
 
   static forContext(context: BrowserContext, params: RecorderParams): Promise<Recorder> {
     let recorderPromise = (context as any)[recorderSymbol] as Promise<Recorder>;
@@ -178,8 +179,11 @@ export class Recorder extends EventEmitter<RecorderEventMap> implements Instrume
             }
           }
         }
+        let mode = this._mode;
+        if (this._pickLocatorPage && source.page !== this._pickLocatorPage)
+          mode = 'none';
         const uiState: UIState = {
-          mode: this._mode,
+          mode,
           actionPoint,
           actionSelector,
           ariaTemplate: this._highlightedElement.ariaTemplate,
@@ -260,12 +264,19 @@ export class Recorder extends EventEmitter<RecorderEventMap> implements Instrume
     this.emit(RecorderEvent.ModeChanged, this._mode);
     this._setEnabled(this._isRecording());
     this._debugger.setMuted(this._isRecording());
-    if (this._mode !== 'none' && this._mode !== 'standby' && this._context.pages().length === 1)
-      this._context.pages()[0].bringToFront().catch(() => {});
+    if (this._mode !== 'none' && this._mode !== 'standby') {
+      let pageToFocus = this._pickLocatorPage;
+      if (!pageToFocus && this._context.pages().length === 1)
+        pageToFocus = this._context.pages()[0];
+      pageToFocus?.bringToFront().catch(() => {});
+    }
     await this._refreshOverlay();
   }
 
-  async pickLocator(progress: Progress): Promise<string> {
+  async pickLocator(progress: Progress, page: Page): Promise<string> {
+    if (this._mode !== 'none')
+      await this.setMode('none');
+
     const selectorPromise = new ManualPromise<string>();
     let recorderChangedState = false;
     const onElementPicked = (elementInfo: ElementInfo) => {
@@ -277,25 +288,22 @@ export class Recorder extends EventEmitter<RecorderEventMap> implements Instrume
       recorderChangedState = true;
       selectorPromise.reject(new Error('Locator picking was cancelled'));
     };
-    const onContextClosed = () => {
-      recorderChangedState = true;
-      selectorPromise.reject(new Error('Context was closed'));
-    };
     const listeners: RegisteredListener[] = [
       eventsHelper.addEventListener(this, RecorderEvent.ElementPicked, onElementPicked),
       eventsHelper.addEventListener(this, RecorderEvent.ModeChanged, onModeChanged),
-      eventsHelper.addEventListener(this, RecorderEvent.ContextClosed, onContextClosed),
     ];
     try {
       const doPickLocator = async () => {
         // Prevent unhandled rejection in case of cancellation during setMode
         selectorPromise.catch(() => {});
+        this._pickLocatorPage = page;
         await this.setMode('inspecting');
         return await selectorPromise;
       };
       return await progress.race(doPickLocator());
     } finally {
       eventsHelper.removeEventListeners(listeners);
+      this._pickLocatorPage = undefined;
       if (!recorderChangedState)
         await this.setMode('none');
     }

packages/playwright-core/src/server/webkit/wkPage.ts

@@ -694,7 +694,7 @@ export class WKPage implements PageDelegate {
   }
 
   async bringToFront(): Promise<void> {
-    this._pageProxySession.send('Target.activate', {
+    await this._pageProxySession.send('Target.activate', {
       targetId: this._session.sessionId
     });
   }

packages/playwright-core/src/tools/dashboard/dashboardController.ts

@@ -241,14 +241,18 @@ export class DashboardConnection implements Transport, DashboardChannel {
     const pages = this._context.pages();
     if (pages.length === 0)
       return [];
-    const dashboardUrl = await this._dashboardUrl(pages[0]);
-    return await Promise.all(pages.map(async page => ({
-      pageId: this._pageId(page),
-      title: await page.title() || page.url(),
-      url: page.url(),
-      selected: page === this.selectedPage,
-      inspectorUrl: dashboardUrl ? await this._pageInspectorUrl(page, dashboardUrl) : 'data:text/plain,Dashboard only supported in Chromium based browsers',
-    })));
+    const devtoolsUrl = await this._devtoolsUrl(pages[0]);
+    return await Promise.all(pages.map(async page => {
+      // page.title() throws on navigation.
+      const title = await page.title().catch(() => undefined) || `Loading ${page.url()}`;
+      return {
+        pageId: this._pageId(page),
+        title,
+        url: page.url(),
+        selected: page === this.selectedPage,
+        inspectorUrl: devtoolsUrl ? await this._pageInspectorUrl(page, devtoolsUrl) : 'data:text/plain,Dashboard only supported in Chromium based browsers',
+      };
+    }));
   }
 
   pageForId(pageId: string) {
@@ -259,19 +263,19 @@ export class DashboardConnection implements Transport, DashboardChannel {
     return (p as any)._guid;
   }
 
-  private async _dashboardUrl(page: api.Page) {
+  private async _devtoolsUrl(page: api.Page) {
     const cdpPort = (this._browserDescriptor.browser.launchOptions as any).cdpPort;
     if (cdpPort)
-      return new URL(`http://localhost:${cdpPort}/dashboard/`);
+      return new URL(`http://localhost:${cdpPort}/devtools/`);
 
     const browserRevision = await getBrowserRevision(page);
     if (!browserRevision)
       return null;
     return new URL(`https://chrome-devtools-frontend.appspot.com/serve_rev/${browserRevision}/`);
   }
 
-  private async _pageInspectorUrl(page: api.Page, dashboardUrl: URL): Promise<string | undefined> {
-    const inspector = new URL('./devtools_app.html', dashboardUrl);
+  private async _pageInspectorUrl(page: api.Page, devtoolsUrl: URL): Promise<string | undefined> {
+    const inspector = new URL('./devtools_app.html', devtoolsUrl);
     const cdp = new URL(this._cdpUrl);
     cdp.searchParams.set('cdpPageId', this._pageId(page));
     inspector.searchParams.set('ws', `${cdp.host}${cdp.pathname}${cdp.search}`);

tests/library/inspector/recorder-api.spec.ts

@@ -167,14 +167,25 @@ test('page.pickLocator should return locator for picked element', async ({ page
 });
 
 test('page.cancelPickLocator should cancel ongoing pickLocator', async ({ page }) => {
-  await page.setContent(`<button>Submit</button>`);
-
-  const scriptReady = page.waitForEvent('console', msg => msg.text() === 'Recorder script ready for test');
   const pickPromise = page.pickLocator();
-  await scriptReady;
-
   await Promise.all([
     page.cancelPickLocator(),
-    expect(pickPromise).rejects.toThrow('Locator picking was cancelled'),
+    expect(pickPromise).rejects.toThrow('Locator picking was cancelled')
   ]);
 });
+
+test('closing page should cancel ongoing pickLocator', async ({ page }) => {
+  await page.setContent(`<button>Click me</button>`);
+  const pickPromise = page.pickLocator().catch(e => e.message);
+  await page.close();
+  expect(await pickPromise).toContain('Target page, context or browser has been closed');
+});
+
+test('page2.pickLocator() should cancel page1.pickLocator()', async ({ page, context }) => {
+  const pick1Promise = page.pickLocator().catch(e => e.message);
+
+  const page2 = await context.newPage();
+  page2.pickLocator().catch(() => {});
+
+  expect(await pick1Promise).toContain('Locator picking was cancelled');
+});