Rework theme menu (#5161)

* Change theme "button" to sub menu of all themes

* Add test for theme menu

* Prevent separator being added before View

* Refactor test

* Update locales [skip ci]

* Fix has-text vs text-is change breaking other tests

---------

Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: bymyself <cbyrne@comfy.org>

Author: 3 people

browser_tests/fixtures/ComfyPage.ts

@@ -453,6 +453,32 @@ export class ComfyPage {
     await workflowsTab.close()
   }
 
+  /**
+   * Attach a screenshot to the test report.
+   * By default, screenshots are only taken in non-CI environments.
+   * @param name - Name for the screenshot attachment
+   * @param options - Optional configuration
+   * @param options.runInCI - Whether to take screenshot in CI (default: false)
+   * @param options.fullPage - Whether to capture full page (default: false)
+   */
+  async attachScreenshot(
+    name: string,
+    options: { runInCI?: boolean; fullPage?: boolean } = {}
+  ) {
+    const { runInCI = false, fullPage = false } = options
+
+    // Skip in CI unless explicitly requested
+    if (process.env.CI && !runInCI) {
+      return
+    }
+
+    const testInfo = comfyPageFixture.info()
+    await testInfo.attach(name, {
+      body: await this.page.screenshot({ fullPage }),
+      contentType: 'image/png'
+    })
+  }
+
   async resetView() {
     if (await this.resetViewButton.isVisible()) {
       await this.resetViewButton.click()

browser_tests/fixtures/components/Topbar.ts

@@ -1,7 +1,13 @@
-import { Locator, Page } from '@playwright/test'
+import { Locator, Page, expect } from '@playwright/test'
 
 export class Topbar {
-  constructor(public readonly page: Page) {}
+  private readonly menuLocator: Locator
+  private readonly menuTrigger: Locator
+
+  constructor(public readonly page: Page) {
+    this.menuLocator = page.locator('.comfy-command-menu')
+    this.menuTrigger = page.locator('.comfyui-logo-wrapper')
+  }
 
   async getTabNames(): Promise<string[]> {
     return await this.page
@@ -15,10 +21,33 @@ export class Topbar {
       .innerText()
   }
 
-  getMenuItem(itemLabel: string): Locator {
+  /**
+   * Get a menu item by its label, optionally within a specific parent container
+   */
+  getMenuItem(itemLabel: string, parent?: Locator): Locator {
+    if (parent) {
+      return parent.locator(`.p-tieredmenu-item:has-text("${itemLabel}")`)
+    }
+
     return this.page.locator(`.p-menubar-item-label:text-is("${itemLabel}")`)
   }
 
+  /**
+   * Get the visible submenu (last visible submenu in case of nested menus)
+   */
+  getVisibleSubmenu(): Locator {
+    return this.page.locator('.p-tieredmenu-submenu:visible').last()
+  }
+
+  /**
+   * Check if a menu item has an active checkmark
+   */
+  async isMenuItemActive(menuItem: Locator): Promise<boolean> {
+    const checkmark = menuItem.locator('.pi-check')
+    const classes = await checkmark.getAttribute('class')
+    return classes ? !classes.includes('invisible') : false
+  }
+
   getWorkflowTab(tabName: string): Locator {
     return this.page
       .locator(`.workflow-tabs .workflow-label:has-text("${tabName}")`)
@@ -66,10 +95,50 @@ export class Topbar {
 
   async openTopbarMenu() {
     await this.page.waitForTimeout(1000)
-    await this.page.locator('.comfyui-logo-wrapper').click()
-    const menu = this.page.locator('.comfy-command-menu')
-    await menu.waitFor({ state: 'visible' })
-    return menu
+    await this.menuTrigger.click()
+    await this.menuLocator.waitFor({ state: 'visible' })
+    return this.menuLocator
+  }
+
+  /**
+   * Close the topbar menu by clicking outside
+   */
+  async closeTopbarMenu() {
+    await this.page.locator('body').click({ position: { x: 10, y: 10 } })
+    await expect(this.menuLocator).not.toBeVisible()
+  }
+
+  /**
+   * Navigate to a submenu by hovering over a menu item
+   */
+  async openSubmenu(menuItemLabel: string): Promise<Locator> {
+    const menuItem = this.getMenuItem(menuItemLabel)
+    await menuItem.hover()
+    const submenu = this.getVisibleSubmenu()
+    await submenu.waitFor({ state: 'visible' })
+    return submenu
+  }
+
+  /**
+   * Get theme menu items and interact with theme switching
+   */
+  async getThemeMenuItems() {
+    const themeSubmenu = await this.openSubmenu('Theme')
+    return {
+      submenu: themeSubmenu,
+      darkTheme: this.getMenuItem('Dark (Default)', themeSubmenu),
+      lightTheme: this.getMenuItem('Light', themeSubmenu)
+    }
+  }
+
+  /**
+   * Switch to a specific theme
+   */
+  async switchTheme(theme: 'dark' | 'light') {
+    const { darkTheme, lightTheme } = await this.getThemeMenuItems()
+    const themeItem = theme === 'dark' ? darkTheme : lightTheme
+    const themeLabel = themeItem.locator('.p-menubar-item-label')
+    await themeLabel.click()
   }
 
   async triggerTopbarCommand(path: string[]) {
@@ -79,9 +148,7 @@ export class Topbar {
 
     const menu = await this.openTopbarMenu()
     const tabName = path[0]
-    const topLevelMenuItem = this.page.locator(
-      `.p-menubar-item-label:text-is("${tabName}")`
-    )
+    const topLevelMenuItem = this.getMenuItem(tabName)
     const topLevelMenu = menu
       .locator('.p-tieredmenu-item')
       .filter({ has: topLevelMenuItem })

browser_tests/tests/menu.spec.ts

@@ -178,6 +178,72 @@ test.describe('Menu', () => {
       await comfyPage.menu.topbar.triggerTopbarCommand(['ext', 'foo-command'])
       expect(await comfyPage.getVisibleToastCount()).toBe(1)
     })
+
+    test('Can navigate Theme menu and switch between Dark and Light themes', async ({
+      comfyPage
+    }) => {
+      const { topbar } = comfyPage.menu
+
+      // Take initial screenshot with default theme
+      await comfyPage.attachScreenshot('theme-initial')
+
+      // Open the topbar menu
+      const menu = await topbar.openTopbarMenu()
+      await expect(menu).toBeVisible()
+
+      // Get theme menu items
+      const {
+        submenu: themeSubmenu,
+        darkTheme: darkThemeItem,
+        lightTheme: lightThemeItem
+      } = await topbar.getThemeMenuItems()
+
+      await expect(darkThemeItem).toBeVisible()
+      await expect(lightThemeItem).toBeVisible()
+
+      // Switch to Light theme
+      await topbar.switchTheme('light')
+
+      // Verify menu stays open and Light theme shows as active
+      await expect(menu).toBeVisible()
+      await expect(themeSubmenu).toBeVisible()
+
+      // Check that Light theme is active
+      expect(await topbar.isMenuItemActive(lightThemeItem)).toBe(true)
+
+      // Screenshot with light theme active
+      await comfyPage.attachScreenshot('theme-menu-light-active')
+
+      // Verify ColorPalette setting is set to "light"
+      expect(await comfyPage.getSetting('Comfy.ColorPalette')).toBe('light')
+
+      // Close menu to see theme change
+      await topbar.closeTopbarMenu()
+
+      // Re-open menu and get theme items again
+      await topbar.openTopbarMenu()
+      const themeItems2 = await topbar.getThemeMenuItems()
+
+      // Switch back to Dark theme
+      await topbar.switchTheme('dark')
+
+      // Verify menu stays open and Dark theme shows as active
+      await expect(menu).toBeVisible()
+      await expect(themeItems2.submenu).toBeVisible()
+
+      // Check that Dark theme is active and Light theme is not
+      expect(await topbar.isMenuItemActive(themeItems2.darkTheme)).toBe(true)
+      expect(await topbar.isMenuItemActive(themeItems2.lightTheme)).toBe(false)
+
+      // Screenshot with dark theme active
+      await comfyPage.attachScreenshot('theme-menu-dark-active')
+
+      // Verify ColorPalette setting is set to "dark"
+      expect(await comfyPage.getSetting('Comfy.ColorPalette')).toBe('dark')
+
+      // Close menu
+      await topbar.closeTopbarMenu()
+    })
   })
 
   // Only test 'Top' to reduce test time.

src/components/topbar/CommandMenubar.vue

@@ -28,29 +28,7 @@
     @show="onMenuShow"
   >
     <template #item="{ item, props }">
-      <div
-        v-if="item.key === 'theme'"
-        class="flex items-center gap-4 px-4 py-5"
-        @click.stop.prevent
-      >
-        {{ item.label }}
-        <SelectButton
-          :options="[darkLabel, lightLabel]"
-          :model-value="activeTheme"
-          @click.stop.prevent
-          @update:model-value="onThemeChange"
-        >
-          <template #option="{ option }">
-            <div class="flex items-center gap-2">
-              <i v-if="option === lightLabel" class="pi pi-sun" />
-              <i v-if="option === darkLabel" class="pi pi-moon" />
-              <span>{{ option }}</span>
-            </div>
-          </template>
-        </SelectButton>
-      </div>
       <a
-        v-else
         class="p-menubar-item-link px-4 py-2"
         v-bind="props.action"
         :href="item.url"
@@ -95,7 +73,6 @@
 
 <script setup lang="ts">
 import type { MenuItem } from 'primevue/menuitem'
-import SelectButton from 'primevue/selectbutton'
 import TieredMenu, {
   type TieredMenuMethods,
   type TieredMenuState
@@ -106,6 +83,7 @@ import { useI18n } from 'vue-i18n'
 import SubgraphBreadcrumb from '@/components/breadcrumb/SubgraphBreadcrumb.vue'
 import SettingDialogContent from '@/components/dialog/content/SettingDialogContent.vue'
 import SettingDialogHeader from '@/components/dialog/header/SettingDialogHeader.vue'
+import { useColorPaletteService } from '@/services/colorPaletteService'
 import { useDialogService } from '@/services/dialogService'
 import { useCommandStore } from '@/stores/commandStore'
 import { useDialogStore } from '@/stores/dialogStore'
@@ -121,6 +99,7 @@ import { normalizeI18nKey } from '@/utils/formatUtil'
 import { whileMouseDown } from '@/utils/mouseDownUtil'
 
 const colorPaletteStore = useColorPaletteStore()
+const colorPaletteService = useColorPaletteService()
 const menuItemsStore = useMenuItemStore()
 const commandStore = useCommandStore()
 const dialogStore = useDialogStore()
@@ -184,11 +163,26 @@ const showManageExtensions = async () => {
   }
 }
 
-const extraMenuItems = computed<MenuItem[]>(() => [
+const themeMenuItems = computed(() => {
+  return colorPaletteStore.palettes.map((palette) => ({
+    key: `theme-${palette.id}`,
+    label: palette.name,
+    parentPath: 'theme',
+    comfyCommand: {
+      active: () => colorPaletteStore.activePaletteId === palette.id
+    },
+    command: async () => {
+      await colorPaletteService.loadColorPalette(palette.id)
+    }
+  }))
+})
+
+const extraMenuItems = computed(() => [
   { separator: true },
   {
     key: 'theme',
-    label: t('menu.theme')
+    label: t('menu.theme'),
+    items: themeMenuItems.value
   },
   { separator: true },
   {
@@ -211,19 +205,6 @@ const extraMenuItems = computed<MenuItem[]>(() => [
   }
 ])
 
-const lightLabel = computed(() => t('menu.light'))
-const darkLabel = computed(() => t('menu.dark'))
-
-const activeTheme = computed(() => {
-  return colorPaletteStore.completedActivePalette.light_theme
-    ? lightLabel.value
-    : darkLabel.value
-})
-
-const onThemeChange = async () => {
-  await commandStore.execute('Comfy.ToggleTheme')
-}
-
 const translatedItems = computed(() => {
   const items = menuItemsStore.menuItems.map(translateMenuItem)
   let helpIndex = items.findIndex((item) => item.key === 'Help')
@@ -308,7 +289,12 @@ const handleItemClick = (item: MenuItem, event: MouseEvent) => {
 }
 
 const hasActiveStateSiblings = (item: MenuItem): boolean => {
-  return menuItemsStore.menuItemHasActiveStateChildren[item.parentPath]
+  // Check if this item has siblings with active state (either from store or theme items)
+  return (
+    item.parentPath &&
+    (item.parentPath === 'theme' ||
+      menuItemsStore.menuItemHasActiveStateChildren[item.parentPath])
+  )
 }
 </script>
 
@@ -332,6 +318,18 @@ const hasActiveStateSiblings = (item: MenuItem): boolean => {
 </style>
 
 <style>
+.comfy-command-menu {
+  --p-tieredmenu-item-focus-background: color-mix(
+    in srgb,
+    var(--fg-color) 15%,
+    transparent
+  );
+  --p-tieredmenu-item-active-background: color-mix(
+    in srgb,
+    var(--fg-color) 10%,
+    transparent
+  );
+}
 .comfy-command-menu ul {
   background-color: var(--comfy-menu-secondary-bg) !important;
 }

src/constants/coreMenuCommands.ts

@@ -23,6 +23,7 @@ export const CORE_MENU_COMMANDS = [
       'Comfy.Memory.UnloadModelsAndExecutionCache'
     ]
   ],
+  [['View'], []],
   [
     ['Help'],
     [

src/locales/ar/main.json

@@ -842,6 +842,7 @@
     "Ungroup selected group nodes": "فك تجميع عقد المجموعة المحددة",
     "Unlock Canvas": "فتح قفل اللوحة",
     "Unpack the selected Subgraph": "فك تجميع الرسم البياني الفرعي المحدد",
+    "View": "عرض",
     "Workflows": "سير العمل",
     "Zoom In": "تكبير",
     "Zoom Out": "تصغير",

src/locales/en/main.json

@@ -1004,6 +1004,7 @@
   "menuLabels": {
     "Workflow": "Workflow",
     "Edit": "Edit",
+    "View": "View",
     "Manager": "Manager",
     "Help": "Help",
     "Check for Updates": "Check for Updates",

src/locales/es/main.json

@@ -825,7 +825,11 @@
     "Ungroup selected group nodes": "Desagrupar nodos de grupo seleccionados",
     "Unload Models": "Descargar modelos",
     "Unload Models and Execution Cache": "Descargar modelos y caché de ejecución",
+    "Unlock Canvas": "Desbloquear lienzo",
+    "Unpack the selected Subgraph": "Desempaquetar el Subgrafo seleccionado",
+    "View": "Ver",
     "Workflow": "Flujo de trabajo",
+    "Workflows": "Flujos de trabajo",
     "Zoom In": "Acercar",
     "Zoom Out": "Alejar",
     "Zoom to fit": "Ajustar al tamaño"