Skip to content

Conversation

davvid-comfy
Copy link

@davvid-comfy davvid-comfy commented Oct 9, 2025

Summary

Make it easier to change the mask brush size. These keybindings are familiar to users of other image editors.

Changes

  • What: Added [ to decrease and ] to increase the brush size.

Review Focus

Is this generically useful enough to include the default keybindings?

┆Issue is synchronized with this Notion page by Unito

References

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Oct 9, 2025
Copy link

github-actions bot commented Oct 9, 2025

🎨 Storybook Build Status

loading Build is starting...

⏰ Started at: 10/20/2025, 06:57:03 AM UTC

🚀 Building Storybook

  • 📦 Installing dependencies...
  • 🔧 Building Storybook components...
  • 🌐 Preparing deployment to Cloudflare Pages...

⏱️ Please wait while the Storybook build is in progress...

Copy link

github-actions bot commented Oct 9, 2025

🎭 Playwright Test Results

⚠️ Tests passed with flaky tests

⏰ Completed at: 10/20/2025, 07:11:04 AM UTC

📈 Summary

  • Total Tests: 498
  • Passed: 464 ✅
  • Failed: 0
  • Flaky: 3 ⚠️
  • Skipped: 31 ⏭️

📊 Test Reports by Browser

  • chromium: View Report • ✅ 455 / ❌ 0 / ⚠️ 3 / ⏭️ 31
  • chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • mobile-chrome: View Report • ✅ 6 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

jtydhr88
jtydhr88 previously approved these changes Oct 9, 2025
Copy link
Collaborator

@jtydhr88 jtydhr88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on my local, worked as expected

Copy link
Contributor

@christian-byrne christian-byrne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mask editor has an internal Keybinding manager because we don't have a keybinding context system yet and adding a keybinding globally will trigger whether or not the mask editor is initialized or opened.

@davvid-comfy
Copy link
Author

I see what you mean about the local keybinding manager. I added a 2nd commit that handles this behavior using the scoped keyDown callback instead of with a global keybinding. If you think the end result looks better I'll squash the two commits together.

@brucew4yn3rp
Copy link
Collaborator

Hi @davvid-comfy this feature already exists as a keybinding setting.

image

Copy link
Contributor

@christian-byrne christian-byrne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Make it easier to change the mask brush size. These keybindings are
familiar to users of other image editors.
@davvid-comfy davvid-comfy force-pushed the maskeditor-keyboard-shortcuts branch from 56d2f0f to 6547af4 Compare October 12, 2025 02:13
@davvid-comfy
Copy link
Author

I just squashed the commits together.

@brucew4yn3rp the commit isn't here anymore because I squashed them together, but this PR was originally just an addition to the core keybindings to make this behavior preset. At least in my usage, that keybinding wasn't present by default.

What's here now is scoped behavior to just the editor and it's unconditional which is kinda nice but it does beg the question as to what happens w/ this keyDown approach if someone were to have the same hotkey set in their settings.

Maybe the original corekeybindings approach was better in that it avoids that issue, despite the concerns raised earlier? Here's a link for comparison:

davvid-comfy@200fbb7

I can always swap that back in here instead if we think that's a cleaner implementation. That's the maskeditor-keyboard-shortcuts-v0 branch in my fork.

@brucew4yn3rp
Copy link
Collaborator

@davvid-comfy we had initially incorporated default keybindings but got this feedback from the team, so I’ll defer to @webfiltered and others if we now want to incorporate default keys for brush re-sizing.

In my opinion, the existence of the setting is more versatile and serves the purpose well.

@davvid-comfy
Copy link
Author

Thanks for the link to that feedback. (heads-up @webfiltered)

The non-us keyboard concern makes sense, but I think a better solution for that would be to allow multiple keyboard shortcuts to bind to the same action. That way a non-US shortcut could be provided as well, but.. honestly.. is that really a problem? That didn't seem to prevent this shortcut from being adopted elsewhere.

Providing it by default is really the better approach in general. All of the popular image editing programs use this same shortcut and it can be argued that it's on the same level as ctrl-z for undo -- it really is that ubiquitous and it's a very common operation.

Consider for a moment that it would take an average user 30 seconds to configure this, and that's assuming they already know that it exists and is configurable. Multiply that by even 10k users and we're already talking about 5k wasted minutes collectively.

Those are user-side arguments in favor of it, at least. It's not a hill I'm going to die on, though, so if you don't want it let's close this PR.

@brucew4yn3rp
Copy link
Collaborator

brucew4yn3rp commented Oct 14, 2025

I appreciate your preference for the [ and ] keybindings. As mentioned, I’d defer to @webfiltered since we removed the default assignment based on earlier feedback.

That said, perhaps the right approach isn’t to hardcode a message broker command, but instead to have the keybinding settings preset to [ and ] for brush size decrease and increase (respectively). This would mitigate the concern of every user having to set a somewhat standard keybinding but would still allow the versatility for users who want to set non-standard bindings.

Copy link

Bundle Size Report

Summary

  • Raw size: 12.5 MB baseline 12.5 MB — 🔴 +194 B
  • Gzip: 2.54 MB baseline 2.54 MB — 🔴 +37 B
  • Brotli: 2 MB baseline 2 MB — 🔴 +21 B
  • Bundles: 56 current • 56 baseline • 12 added / 12 removed

Category Glance
Data & Services 🔴 +194 B (10.2 kB) · Vendor & Third-Party ⚪ 0 B (5.36 MB) · App Entry Points ⚪ 0 B (3.31 MB) · Other ⚪ 0 B (2.79 MB) · Graph Workspace ⚪ 0 B (707 kB) · Panels & Settings ⚪ 0 B (294 kB) · + 3 more

Per-category breakdown
App Entry Points — 3.31 MB (baseline 3.31 MB) • ⚪ 0 B _Main entry bundles and manifests_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | ---------------------------------------- | ------- | ------- | ----------------------- | ---------------------- | ----------------------- | | ~~assets/index-BF98DELv.js~~ _(removed)_ | 2.69 MB | — | 🟢 -2.69 MB | 🟢 -561 kB | 🟢 -424 kB | | **assets/index-D5D0_ixA.js** _(new)_ | — | 2.69 MB | 🔴 +2.69 MB | 🔴 +561 kB | 🔴 +424 kB | | ~~assets/index-Oc6FsrEY.js~~ _(removed)_ | 616 kB | — | 🟢 -616 kB | 🟢 -114 kB | 🟢 -90.3 kB | | **assets/index-qQ-LWSM4.js** _(new)_ | — | 616 kB | 🔴 +616 kB | 🔴 +114 kB | 🔴 +90.3 kB |

Status: 2 added / 2 removed

Graph Workspace — 707 kB (baseline 707 kB) • ⚪ 0 B _Graph editor runtime, canvas, workflow orchestration_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | -------------------------------------------- | ------ | ------ | ---------------------- | ---------------------- | ---------------------- | | **assets/GraphView-D9tVhV-D.js** _(new)_ | — | 707 kB | 🔴 +707 kB | 🔴 +138 kB | 🔴 +107 kB | | ~~assets/GraphView-VsdP2Gw4.js~~ _(removed)_ | 707 kB | — | 🟢 -707 kB | 🟢 -138 kB | 🟢 -107 kB |

Status: 1 added / 1 removed

Views & Navigation — 8.15 kB (baseline 8.15 kB) • ⚪ 0 B _Top-level views, pages, and routed surfaces_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | ------------------------------------------------- | ------- | ------- | ----------------------- | ----------------------- | ----------------------- | | ~~assets/UserSelectView-DMJFvUHL.js~~ _(removed)_ | 8.15 kB | — | 🟢 -8.15 kB | 🟢 -2.46 kB | 🟢 -2.16 kB | | **assets/UserSelectView-DTbQpuOa.js** _(new)_ | — | 8.15 kB | 🔴 +8.15 kB | 🔴 +2.46 kB | 🔴 +2.16 kB |

Status: 1 added / 1 removed

Panels & Settings — 294 kB (baseline 294 kB) • ⚪ 0 B _Configuration panels, inspectors, and settings screens_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | ---------------------------------------------------- | ------- | ------- | ----------------------- | ----------------------- | ----------------------- | | **assets/CreditsPanel-BT0R2oJ-.js** _(new)_ | — | 22.1 kB | 🔴 +22.1 kB | 🔴 +5.28 kB | 🔴 +4.61 kB | | ~~assets/CreditsPanel-Ph2eVkIo.js~~ _(removed)_ | 22.1 kB | — | 🟢 -22.1 kB | 🟢 -5.28 kB | 🟢 -4.61 kB | | **assets/KeybindingPanel-CS1szTxl.js** _(new)_ | — | 15.2 kB | 🔴 +15.2 kB | 🔴 +3.75 kB | 🔴 +3.31 kB | | ~~assets/KeybindingPanel-Cy-10Gme.js~~ _(removed)_ | 15.2 kB | — | 🟢 -15.2 kB | 🟢 -3.75 kB | 🟢 -3.32 kB | | **assets/ExtensionPanel-BbbPc-dF.js** _(new)_ | — | 12.1 kB | 🔴 +12.1 kB | 🔴 +2.82 kB | 🔴 +2.47 kB | | ~~assets/ExtensionPanel-CR6D8lYs.js~~ _(removed)_ | 12.1 kB | — | 🟢 -12.1 kB | 🟢 -2.82 kB | 🟢 -2.47 kB | | **assets/AboutPanel-DY8KuJKT.js** _(new)_ | — | 10.3 kB | 🔴 +10.3 kB | 🔴 +2.66 kB | 🔴 +2.33 kB | | ~~assets/AboutPanel-R8soUVEk.js~~ _(removed)_ | 10.3 kB | — | 🟢 -10.3 kB | 🟢 -2.66 kB | 🟢 -2.33 kB | | **assets/ServerConfigPanel-BQKLA7iX.js** _(new)_ | — | 8.2 kB | 🔴 +8.2 kB | 🔴 +2.16 kB | 🔴 +1.9 kB | | ~~assets/ServerConfigPanel-W3IQZs4m.js~~ _(removed)_ | 8.2 kB | — | 🟢 -8.2 kB | 🟢 -2.16 kB | 🟢 -1.91 kB | | **assets/UserPanel-B5RDdVwB.js** _(new)_ | — | 7.91 kB | 🔴 +7.91 kB | 🔴 +2.05 kB | 🔴 +1.79 kB | | ~~assets/UserPanel-CTlUle_0.js~~ _(removed)_ | 7.91 kB | — | 🟢 -7.91 kB | 🟢 -2.05 kB | 🟢 -1.79 kB | | assets/settings-B-df0dZe.js | 20.7 kB | 20.7 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-CI6OKvJn.js | 22.9 kB | 22.9 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-CXGVj_nD.js | 24.5 kB | 24.5 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-DfQ6dSJj.js | 31.6 kB | 31.6 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-DJ2QgDzm.js | 25.2 kB | 25.2 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-DRNLPMG6.js | 23.7 kB | 23.7 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-DVVycxDc.js | 19.9 kB | 19.9 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-G6Dybj1b.js | 24.1 kB | 24.1 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-M6_GZccG.js | 26 kB | 26 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B |

Status: 6 added / 6 removed

UI Components — 12.3 kB (baseline 12.3 kB) • ⚪ 0 B _Reusable component library chunks_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | ----------------------------------------------------------------- | ------- | ------- | ----------------------- | ----------------------- | ----------------------- | | **assets/ComfyQueueButton-BLYniyIF.js** _(new)_ | — | 11.1 kB | 🔴 +11.1 kB | 🔴 +2.76 kB | 🔴 +2.43 kB | | ~~assets/ComfyQueueButton-Bm-0vk2R.js~~ _(removed)_ | 11.1 kB | — | 🟢 -11.1 kB | 🟢 -2.76 kB | 🟢 -2.43 kB | | assets/UserAvatar.vue_vue_type_script_setup_true_lang-C9bSkTC5.js | 1.12 kB | 1.12 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B |

Status: 1 added / 1 removed

Data & Services — 10.2 kB (baseline 10 kB) • 🔴 +194 B _Stores, services, APIs, and repositories_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | ---------------------------------------------------- | ------- | ------- | ----------------------- | ----------------------- | ---------------------- | | **assets/keybindingService-CMEKzWFF.js** _(new)_ | — | 7.4 kB | 🔴 +7.4 kB | 🔴 +1.78 kB | 🔴 +1.54 kB | | ~~assets/keybindingService-BC_WyRxV.js~~ _(removed)_ | 7.21 kB | — | 🟢 -7.21 kB | 🟢 -1.75 kB | 🟢 -1.5 kB | | assets/serverConfigStore-B1bw4Pe0.js | 2.79 kB | 2.79 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B |

Status: 1 added / 1 removed

Utilities & Hooks — 1.07 kB (baseline 1.07 kB) • ⚪ 0 B _Helpers, composables, and utility bundles_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | --------------------------- | ------- | ------- | ------------------ | ------------------ | ------------------ | | assets/mathUtil-CTARWQ-l.js | 1.07 kB | 1.07 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B |
Vendor & Third-Party — 5.36 MB (baseline 5.36 MB) • ⚪ 0 B _External libraries and shared vendor chunks_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | --------------------------------------- | ------- | ------- | ------------------ | ------------------ | ------------------ | | assets/vendor-other-kaNE-JGc.js | 3.22 MB | 3.22 MB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/vendor-primevue-PESgPnbc.js | 517 B | 517 B | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/vendor-tiptap-DKA7Hxfn.js | 232 kB | 232 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/vendor-visualization-BEfdbjRw.js | 1.82 MB | 1.82 MB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/vendor-vue-QImF2beP.js | 92.4 kB | 92.4 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B |
Other — 2.79 MB (baseline 2.79 MB) • ⚪ 0 B _Bundles that do not match a named category_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | --------------------------- | ------- | ------- | ------------------ | ------------------ | ------------------ | | assets/commands-B2KZRBmX.js | 15.1 kB | 15.1 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-Bw-ckyga.js | 13.9 kB | 13.9 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-C_NmM85I.js | 13.8 kB | 13.8 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-CuozCW4W.js | 14 kB | 14 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-DGfVUJCR.js | 16.2 kB | 16.2 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-dOJNDogK.js | 14.5 kB | 14.5 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-DwiE551e.js | 14.7 kB | 14.7 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-Fw7mvqSy.js | 13.1 kB | 13.1 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-FXnO1W4Q.js | 13.2 kB | 13.2 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-6UgCUkrV.js | 108 kB | 108 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-BfHN1fzx.js | 125 kB | 125 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-BhulUfFD.js | 77.5 kB | 77.5 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-BPHe683n.js | 92.4 kB | 92.4 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-C75C4LWt.js | 90.9 kB | 90.9 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-CEhf19j2.js | 99.4 kB | 99.4 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-CipazGd8.js | 79.3 kB | 79.3 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-CKz_lTAz.js | 94.3 kB | 94.3 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-Dzm38Va4.js | 90.3 kB | 90.3 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-BePSqkA4.js | 195 kB | 195 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-BfT7dJcF.js | 204 kB | 204 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-BiAtoiXc.js | 194 kB | 194 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-CDfbduPY.js | 219 kB | 219 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-CDurg_KW.js | 197 kB | 197 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-CE-vG3RG.js | 182 kB | 182 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-DAwVV156.js | 200 kB | 200 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-DexhCMEi.js | 233 kB | 233 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-kTrYLFPK.js | 184 kB | 184 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B |

@davvid-comfy
Copy link
Author

That said, perhaps the right approach isn’t to hardcode a message broker command, but instead to have the keybinding settings preset to [ and ] for brush size decrease and increase (respectively). This would mitigate the concern of every user having to set a somewhat standard keybinding but would still allow the versatility for users who want to set non-standard bindings.

Makes sense. I agree. That's effectively what I had in the original version of this PR (where it just edited the core keybindings and nothing else) so I've restored those changes.

Users are able to override these presets so this PR works the way you describe now. cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:hotkeys area:mask-editor size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants