test: address review feedback for CanvasModeSelector E2E tests

- Replace raw CSS attribute selectors with getByRole/getByTestId semantic locators - Extract repeated locators into getLocators() helper - Add nextFrame() consistently after all interactions - Consolidate 3 near-identical aria-checked tests into 1 - Remove redundant keyboard wrap-around tests - Fix non-retrying shortcut hint assertions (not.toBeEmpty) - Add data-testid="shortcut-hint" to component spans - Replace page.keyboard.press() with locator.press() per standards - Use for...of instead of forEach per codebase convention
2026-04-20 06:20:11 +00:00 · 2026-04-08 22:10:14 -07:00
parent 58646a56e0
commit 6936e94d4b
2 changed files with 109 additions and 139 deletions
--- a/browser_tests/tests/canvasModeSelector.spec.ts
+++ b/browser_tests/tests/canvasModeSelector.spec.ts
@@ -1,26 +1,24 @@
+import type { Page } from '@playwright/test'
 import { expect } from '@playwright/test'

 import { comfyPageFixture as test } from '../fixtures/ComfyPage'

-const TOOLBAR = '[role="toolbar"][aria-label="Canvas Toolbar"]'
-const TRIGGER = '[aria-haspopup="menu"][aria-label="Canvas Mode"]'
-const MENU = '[role="menu"][aria-label="Canvas Mode"]'
-const SELECT_ITEM = '[role="menuitemradio"][aria-label="Select"]'
-const HAND_ITEM = '[role="menuitemradio"][aria-label="Hand"]'
+const getLocators = (page: Page) => ({
+  trigger: page.getByRole('button', { name: 'Canvas Mode' }),
+  menu: page.getByRole('menu', { name: 'Canvas Mode' }),
+  selectItem: page.getByRole('menuitemradio', { name: 'Select' }),
+  handItem: page.getByRole('menuitemradio', { name: 'Hand' })
+})

 const MODES = [
  {
    label: 'Select',
-    itemSelector: SELECT_ITEM,
-    inactiveItemSelector: HAND_ITEM,
    activateCommand: 'Comfy.Canvas.Unlock',
    isReadOnly: false,
    iconPattern: /lucide--mouse-pointer-2/
  },
  {
    label: 'Hand',
-    itemSelector: HAND_ITEM,
-    inactiveItemSelector: SELECT_ITEM,
    activateCommand: 'Comfy.Canvas.Lock',
    isReadOnly: true,
    iconPattern: /lucide--hand/
@@ -38,7 +36,7 @@ test.describe('CanvasModeSelector', { tag: '@canvas' }, () => {
    test('visible in canvas toolbar with ARIA markup', async ({
      comfyPage
    }) => {
-      const trigger = comfyPage.page.locator(TOOLBAR).locator(TRIGGER)
+      const { trigger } = getLocators(comfyPage.page)
      await expect(trigger).toBeVisible()
      await expect(trigger).toHaveAttribute('aria-expanded', 'false')
    })
@@ -49,10 +47,8 @@ test.describe('CanvasModeSelector', { tag: '@canvas' }, () => {
      }) => {
        await comfyPage.command.executeCommand(mode.activateCommand)
        await comfyPage.nextFrame()
-        const modeIcon = comfyPage.page
-          .locator(TRIGGER)
-          .locator('i[aria-hidden="true"]')
-          .first()
+        const { trigger } = getLocators(comfyPage.page)
+        const modeIcon = trigger.locator('i[aria-hidden="true"]').first()
        await expect(modeIcon).toHaveClass(mode.iconPattern)
      })
    }
@@ -60,35 +56,42 @@ test.describe('CanvasModeSelector', { tag: '@canvas' }, () => {

  test.describe('Popover lifecycle', () => {
    test('opens when trigger is clicked', async ({ comfyPage }) => {
-      const trigger = comfyPage.page.locator(TRIGGER)
+      const { trigger, menu } = getLocators(comfyPage.page)
      await trigger.click()
-      await expect(comfyPage.page.locator(MENU)).toBeVisible()
+      await comfyPage.nextFrame()
+      await expect(menu).toBeVisible()
      await expect(trigger).toHaveAttribute('aria-expanded', 'true')
    })

    test('closes when trigger is clicked again', async ({ comfyPage }) => {
-      const trigger = comfyPage.page.locator(TRIGGER)
+      const { trigger, menu } = getLocators(comfyPage.page)
      await trigger.click()
-      await expect(comfyPage.page.locator(MENU)).toBeVisible()
+      await comfyPage.nextFrame()
+      await expect(menu).toBeVisible()
      await trigger.click()
-      await expect(comfyPage.page.locator(MENU)).not.toBeVisible()
+      await comfyPage.nextFrame()
+      await expect(menu).not.toBeVisible()
      await expect(trigger).toHaveAttribute('aria-expanded', 'false')
    })

    test('closes after a mode item is selected', async ({ comfyPage }) => {
-      await comfyPage.page.locator(TRIGGER).click()
-      await expect(comfyPage.page.locator(MENU)).toBeVisible()
-      await comfyPage.page.locator(HAND_ITEM).click()
-      await expect(comfyPage.page.locator(MENU)).not.toBeVisible()
+      const { trigger, menu, handItem } = getLocators(comfyPage.page)
+      await trigger.click()
+      await comfyPage.nextFrame()
+      await expect(menu).toBeVisible()
+      await handItem.click()
+      await comfyPage.nextFrame()
+      await expect(menu).not.toBeVisible()
    })

    test('closes when Escape is pressed', async ({ comfyPage }) => {
-      const trigger = comfyPage.page.locator(TRIGGER)
+      const { trigger, menu, selectItem } = getLocators(comfyPage.page)
      await trigger.click()
-      await expect(comfyPage.page.locator(MENU)).toBeVisible()
      await comfyPage.nextFrame()
-      await comfyPage.page.keyboard.press('Escape')
-      await expect(comfyPage.page.locator(MENU)).not.toBeVisible()
+      await expect(menu).toBeVisible()
+      await selectItem.press('Escape')
+      await comfyPage.nextFrame()
+      await expect(menu).not.toBeVisible()
      await expect(trigger).toHaveAttribute('aria-expanded', 'false')
    })
  })
@@ -102,9 +105,15 @@ test.describe('CanvasModeSelector', { tag: '@canvas' }, () => {
          await comfyPage.command.executeCommand('Comfy.Canvas.Lock')
          await comfyPage.nextFrame()
        }
-        await comfyPage.page.locator(TRIGGER).click()
-        await expect(comfyPage.page.locator(MENU)).toBeVisible()
-        await comfyPage.page.locator(mode.itemSelector).click()
+        const { trigger, menu, selectItem, handItem } = getLocators(
+          comfyPage.page
+        )
+        const item = mode.isReadOnly ? handItem : selectItem
+        await trigger.click()
+        await comfyPage.nextFrame()
+        await expect(menu).toBeVisible()
+        await item.click()
+        await comfyPage.nextFrame()
        await expect
          .poll(() => comfyPage.canvasOps.isReadOnly())
          .toBe(mode.isReadOnly)
@@ -118,9 +127,12 @@ test.describe('CanvasModeSelector', { tag: '@canvas' }, () => {
        await comfyPage.canvasOps.isReadOnly(),
        'Precondition: canvas starts in Select mode'
      ).toBe(false)
-      await comfyPage.page.locator(TRIGGER).click()
-      await expect(comfyPage.page.locator(MENU)).toBeVisible()
-      await comfyPage.page.locator(SELECT_ITEM).click()
+      const { trigger, menu, selectItem } = getLocators(comfyPage.page)
+      await trigger.click()
+      await comfyPage.nextFrame()
+      await expect(menu).toBeVisible()
+      await selectItem.click()
+      await comfyPage.nextFrame()
      await expect.poll(() => comfyPage.canvasOps.isReadOnly()).toBe(false)
    })
  })
@@ -129,50 +141,14 @@ test.describe('CanvasModeSelector', { tag: '@canvas' }, () => {
    test('aria-checked marks Select active on default load', async ({
      comfyPage
    }) => {
-      await comfyPage.page.locator(TRIGGER).click()
-      await expect(comfyPage.page.locator(MENU)).toBeVisible()
-      await expect(comfyPage.page.locator(SELECT_ITEM)).toHaveAttribute(
-        'aria-checked',
-        'true'
+      const { trigger, menu, selectItem, handItem } = getLocators(
+        comfyPage.page
      )
-      await expect(comfyPage.page.locator(HAND_ITEM)).toHaveAttribute(
-        'aria-checked',
-        'false'
-      )
-    })
-
-    test('aria-checked flips after switching to Hand', async ({
-      comfyPage
-    }) => {
-      await comfyPage.page.locator(TRIGGER).click()
-      await comfyPage.page.locator(HAND_ITEM).click()
-      await comfyPage.page.locator(TRIGGER).click()
-      await expect(comfyPage.page.locator(MENU)).toBeVisible()
-      await expect(comfyPage.page.locator(HAND_ITEM)).toHaveAttribute(
-        'aria-checked',
-        'true'
-      )
-      await expect(comfyPage.page.locator(SELECT_ITEM)).toHaveAttribute(
-        'aria-checked',
-        'false'
-      )
-    })
-
-    test('aria-checked reflects mode change from external command', async ({
-      comfyPage
-    }) => {
-      await comfyPage.command.executeCommand('Comfy.Canvas.Lock')
+      await trigger.click()
      await comfyPage.nextFrame()
-      await comfyPage.page.locator(TRIGGER).click()
-      await expect(comfyPage.page.locator(MENU)).toBeVisible()
-      await expect(comfyPage.page.locator(HAND_ITEM)).toHaveAttribute(
-        'aria-checked',
-        'true'
-      )
-      await expect(comfyPage.page.locator(SELECT_ITEM)).toHaveAttribute(
-        'aria-checked',
-        'false'
-      )
+      await expect(menu).toBeVisible()
+      await expect(selectItem).toHaveAttribute('aria-checked', 'true')
+      await expect(handItem).toHaveAttribute('aria-checked', 'false')
    })

    for (const mode of MODES) {
@@ -181,56 +157,47 @@ test.describe('CanvasModeSelector', { tag: '@canvas' }, () => {
      }) => {
        await comfyPage.command.executeCommand(mode.activateCommand)
        await comfyPage.nextFrame()
-        await comfyPage.page.locator(TRIGGER).click()
-        await expect(comfyPage.page.locator(MENU)).toBeVisible()
-        await expect(comfyPage.page.locator(mode.itemSelector)).toHaveAttribute(
-          'tabindex',
-          '0'
+        const { trigger, menu, selectItem, handItem } = getLocators(
+          comfyPage.page
        )
-        await expect(
-          comfyPage.page.locator(mode.inactiveItemSelector)
-        ).toHaveAttribute('tabindex', '-1')
+        await trigger.click()
+        await comfyPage.nextFrame()
+        await expect(menu).toBeVisible()
+
+        const activeItem = mode.isReadOnly ? handItem : selectItem
+        const inactiveItem = mode.isReadOnly ? selectItem : handItem
+
+        await expect(activeItem).toHaveAttribute('tabindex', '0')
+        await expect(inactiveItem).toHaveAttribute('tabindex', '-1')
      })
    }
  })

  test.describe('Keyboard navigation', () => {
    test('ArrowDown moves focus from Select to Hand', async ({ comfyPage }) => {
-      await comfyPage.page.locator(TRIGGER).click()
-      await expect(comfyPage.page.locator(MENU)).toBeVisible()
+      const { trigger, menu, selectItem, handItem } = getLocators(
+        comfyPage.page
+      )
+      await trigger.click()
      await comfyPage.nextFrame()
-      await comfyPage.page.keyboard.press('ArrowDown')
-      await expect(comfyPage.page.locator(HAND_ITEM)).toBeFocused()
-    })
-
-    test('ArrowUp wraps focus from Select to Hand', async ({ comfyPage }) => {
-      await comfyPage.page.locator(TRIGGER).click()
-      await expect(comfyPage.page.locator(MENU)).toBeVisible()
-      await comfyPage.nextFrame()
-      await comfyPage.page.keyboard.press('ArrowUp')
-      await expect(comfyPage.page.locator(HAND_ITEM)).toBeFocused()
-    })
-
-    test('ArrowDown wraps focus from Hand to Select', async ({ comfyPage }) => {
-      await comfyPage.command.executeCommand('Comfy.Canvas.Lock')
-      await comfyPage.nextFrame()
-      await comfyPage.page.locator(TRIGGER).click()
-      await expect(comfyPage.page.locator(MENU)).toBeVisible()
-      await comfyPage.nextFrame()
-      await comfyPage.page.keyboard.press('ArrowDown')
-      await expect(comfyPage.page.locator(SELECT_ITEM)).toBeFocused()
+      await expect(menu).toBeVisible()
+      await selectItem.press('ArrowDown')
+      await expect(handItem).toBeFocused()
    })

    test('Escape closes popover and restores focus to trigger', async ({
      comfyPage
    }) => {
-      const trigger = comfyPage.page.locator(TRIGGER)
+      const { trigger, menu, selectItem, handItem } = getLocators(
+        comfyPage.page
+      )
      await trigger.click()
-      await expect(comfyPage.page.locator(MENU)).toBeVisible()
      await comfyPage.nextFrame()
-      await comfyPage.page.keyboard.press('ArrowDown')
-      await comfyPage.page.keyboard.press('Escape')
-      await expect(comfyPage.page.locator(MENU)).not.toBeVisible()
+      await expect(menu).toBeVisible()
+      await selectItem.press('ArrowDown')
+      await handItem.press('Escape')
+      await comfyPage.nextFrame()
+      await expect(menu).not.toBeVisible()
      await expect(trigger).toBeFocused()
    })
  })
@@ -242,10 +209,14 @@ test.describe('CanvasModeSelector', { tag: '@canvas' }, () => {
      }) => {
        await comfyPage.command.executeCommand(mode.activateCommand)
        await comfyPage.nextFrame()
-        await comfyPage.page.locator(TRIGGER).click()
-        await expect(comfyPage.page.locator(MENU)).toBeVisible()
+        const { trigger, menu, selectItem, handItem } = getLocators(
+          comfyPage.page
+        )
+        const item = mode.isReadOnly ? handItem : selectItem
+        await trigger.click()
        await comfyPage.nextFrame()
-        await expect(comfyPage.page.locator(mode.itemSelector)).toBeFocused()
+        await expect(menu).toBeVisible()
+        await expect(item).toBeFocused()
      })
    }
  })
@@ -261,10 +232,8 @@ test.describe('CanvasModeSelector', { tag: '@canvas' }, () => {
      await comfyPage.canvas.press('KeyH')
      await comfyPage.nextFrame()
      expect(await comfyPage.canvasOps.isReadOnly()).toBe(true)
-      const modeIcon = comfyPage.page
-        .locator(TRIGGER)
-        .locator('i[aria-hidden="true"]')
-        .first()
+      const { trigger } = getLocators(comfyPage.page)
+      const modeIcon = trigger.locator('i[aria-hidden="true"]').first()
      await expect(modeIcon).toHaveClass(/lucide--hand/)
    })

@@ -280,10 +249,8 @@ test.describe('CanvasModeSelector', { tag: '@canvas' }, () => {
      await comfyPage.canvas.press('KeyV')
      await comfyPage.nextFrame()
      expect(await comfyPage.canvasOps.isReadOnly()).toBe(false)
-      const modeIcon = comfyPage.page
-        .locator(TRIGGER)
-        .locator('i[aria-hidden="true"]')
-        .first()
+      const { trigger } = getLocators(comfyPage.page)
+      const modeIcon = trigger.locator('i[aria-hidden="true"]').first()
      await expect(modeIcon).toHaveClass(/lucide--mouse-pointer-2/)
    })
  })
@@ -292,20 +259,17 @@ test.describe('CanvasModeSelector', { tag: '@canvas' }, () => {
    test('menu items show non-empty keyboard shortcut text', async ({
      comfyPage
    }) => {
-      await comfyPage.page.locator(TRIGGER).click()
-      await expect(comfyPage.page.locator(MENU)).toBeVisible()
-      const selectHint = comfyPage.page.locator(`${SELECT_ITEM} > span`)
-      const handHint = comfyPage.page.locator(`${HAND_ITEM} > span`)
-      const selectText = await selectHint.textContent()
-      const handText = await handHint.textContent()
-      expect(
-        selectText?.trim().length,
-        'Select shortcut hint is empty'
-      ).toBeGreaterThan(0)
-      expect(
-        handText?.trim().length,
-        'Hand shortcut hint is empty'
-      ).toBeGreaterThan(0)
+      const { trigger, menu, selectItem, handItem } = getLocators(
+        comfyPage.page
+      )
+      await trigger.click()
+      await comfyPage.nextFrame()
+      await expect(menu).toBeVisible()
+      const selectHint = selectItem.getByTestId('shortcut-hint')
+      const handHint = handItem.getByTestId('shortcut-hint')
+
+      await expect(selectHint).not.toBeEmpty()
+      await expect(handHint).not.toBeEmpty()
    })
  })
 })
--- a/src/components/graph/CanvasModeSelector.vue
+++ b/src/components/graph/CanvasModeSelector.vue
@@ -55,9 +55,11 @@
          <i class="icon-[lucide--mouse-pointer-2] size-4" aria-hidden="true" />
          <span>{{ $t('graphCanvasMenu.select') }}</span>
        </div>
-        <span class="text-[9px] text-text-primary">{{
-          unlockCommandText
-        }}</span>
+        <span
+          class="text-[9px] text-text-primary"
+          data-testid="shortcut-hint"
+          >{{ unlockCommandText }}</span
+        >
      </button>

      <button
@@ -76,7 +78,11 @@
          <i class="icon-[lucide--hand] size-4" aria-hidden="true" />
          <span>{{ $t('graphCanvasMenu.hand') }}</span>
        </div>
-        <span class="text-[9px] text-text-primary">{{ lockCommandText }}</span>
+        <span
+          class="text-[9px] text-text-primary"
+          data-testid="shortcut-hint"
+          >{{ lockCommandText }}</span
+        >
      </button>
    </div>
  </Popover>