fix: only treat blocking overlays as open modals - #15455
Draft
DrJKL wants to merge 1 commit into
Draft
Conversation
isModalOpen matched [role="dialog"][aria-modal="true"] by existence alone. PrimeVue popovers set that attribute while being non-blocking hover previews, and extensions that hide only a dialog's mask leave the node in the DOM for the rest of the session. Either one made the keybinding gate consider the page permanently modal, so Ctrl+S was handed back to the browser instead of saving the workflow. Require a matched node to be neither popover content nor hidden by itself or an ancestor, and apply the same rule to the Reka check so both dialog queries agree. - Fixes #15639
🎨 Storybook: ✅ Built — View Storybook🎭 Playwright: ✅ 1832 passed, 0 failed · 4 flaky📊 Browser Reports
📦 Bundle: 9.12 MB gzip 🔴 +388 BDetailsSummary
Category Glance App Entry Points — 3.71 kB (baseline 3.71 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.37 MB (baseline 1.37 MB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 2 added / 2 removed / 1 unchanged Views & Navigation — 124 kB (baseline 124 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 13 added / 13 removed / 4 unchanged Panels & Settings — 566 kB (baseline 566 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed / 16 unchanged User & Accounts — 27.5 kB (baseline 27.5 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 6 added / 6 removed / 5 unchanged Editors & Dialogs — 125 kB (baseline 125 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 7 added / 7 removed / 1 unchanged UI Components — 67.1 kB (baseline 67.1 kB) • ⚪ 0 BReusable component library chunks
Status: 6 added / 6 removed / 8 unchanged Data & Services — 3.53 MB (baseline 3.53 MB) • 🔴 +911 BStores, services, APIs, and repositories
Status: 14 added / 14 removed / 3 unchanged Utilities & Hooks — 549 kB (baseline 549 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 18 added / 18 removed / 19 unchanged Vendor & Third-Party — 18.1 MB (baseline 18.1 MB) • ⚪ 0 BExternal libraries and shared vendor chunks Status: 18 unchanged Other — 14.2 MB (baseline 14.2 MB) • ⚪ 0 BBundles that do not match a named category
Status: 68 added / 68 removed / 219 unchanged ⚡ Performance
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: 2 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour. 📝 WalkthroughWalkthroughThe modal utility now detects visible blocking dialogs and excludes hidden or non-blocking overlays. Tests cover modal detection and ChangesModal detection and shortcut handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change makes global shortcuts ignore hidden or non-blocking overlays while continuing to suppress them for genuine modals; no actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
Suggested labels: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
github-actions
Bot
added
the
risk:R2
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #15455 +/- ##
==========================================
+ Coverage 79.16% 81.76% +2.59%
==========================================
Files 2213 1886 -327
Lines 112075 107190 -4885
Branches 35684 33930 -1754
==========================================
- Hits 88725 87642 -1083
+ Misses 22898 19196 -3702
+ Partials 452 352 -100
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 336 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Thanks — noting for human reviewers that CodeRabbit's pass generated no actionable comments, so no changes were made in response. CI is green on For anyone picking this up, two things in the diff are deliberate rather than oversights, and are the parts worth a human eye:
The one thing knowingly left out of scope: |
DrJKL
added
area:testing
area:ui
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Created by the Glary-Bot Agent
Follow-up to #15066, which stopped Ctrl+S being handed to the browser but left it a silent no-op in the two scenarios users actually reported.
Summary
isModalOpenmatched[role="dialog"][aria-modal="true"]by existence alone — no visibility check, and no exclusion for overlays that declare modality without blocking the page. Two very common things tripped it:Popoverrendersrole="dialog"witharia-modal="true"(popover/index.mjs), so hovering a workflow tab made the app consider itself modal for as long as the preview was up.role='dialog'+ariaModal: 'true'and an inlinedisplay: flex, parented to a mask (comfyui-gui-builder.js:104-110).close()hides only the mask, so the node stays in the DOM atdisplay: flexfor the rest of the session — one click on "Manager" left Ctrl+S dead until reload.With #15066 merged the browser no longer hijacks the combo, but the gate still declines to run the command, so Ctrl+S silently does nothing in both cases.
Changes
A matched node now has to be neither popover content nor hidden (by itself or an ancestor) before it counts as an open modal. The same predicate is applied to the Reka query, so both
role="dialog"checks agree — previously only the Reka one excluded popper content.Hiding is detected by walking ancestors for
display: none, because the mechanism in the wild is a hidden wrapper around a still-display: flexdialog.checkVisibility()would read better but is not implemented in happy-dom, so it is untestable here.Review Focus
managedDialogCount > 0still short-circuits first, so nothing about the app's own dialog stack changes..p-popoveris a deny-list entry, so it un-gates bare-key shortcuts for all 15 PrimeVue Popover call sites, not just the workflow tab. Focus in anINPUT/TEXTAREA/contenteditablealready returns earlier, so text entry is unaffected, and this matches how Reka popovers already behaved onmain.Tests
modalUtil.test.tsis new and enumerates every overlay flavour the predicate claims to support — managed stack, ARIA modal, Reka dialog, native<dialog>, legacy.comfy-modal— plus their non-blocking and left-in-the-DOM counterparts. That structure is a direct answer to the point that a change like #12184 should have come with coverage across all dialog stories.The 6 behavioural tests fail on
mainand pass here; 189 tests pass acrossmodalUtil, allplatform/keybindings, andchangeTracker.Validation
Verified in a browser against a real ComfyUI backend, reading
event.defaultPreventedfrom akeydownlistener:defaultPreventedtruetruetruetrueBare
wwith a genuine modal open stays unprevented and does not toggle the sidebar.pnpm typecheck, ESLint, oxlint and oxfmt clean.Known follow-up (not in this PR)
src/platform/assets/composables/useAssetGridSelection.ts:215still uses the rawdocument.querySelector('[role="dialog"][aria-modal="true"]')check, so Ctrl+A in the asset grid remains broken by both of the same repros. Routing it through this predicate is a small, separate change.Screenshots