fix(ui): block click pass-through on modal overlays#732
fix(ui): block click pass-through on modal overlays#732thepastaclaw wants to merge 4 commits intodashpay:v1.0-devfrom
Conversation
|
@coderabbitai review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughCentralizes modal overlay rendering by adding Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/tokens/tokens_screen/my_tokens.rs (1)
177-216:⚠️ Potential issue | 🟠 Major
Token Configuration Detailsstill has click-through dismissal.This popup still closes on outside clicks without drawing the shared overlay first, so the same click can also hit the token list underneath. That leaves the original pass-through bug reproducible on this screen.
🛠️ Proposed fix
if let Some(token_info) = self.all_known_tokens.get(&token_id).cloned() { let mut is_open = true; let mut close_popup = false; let dark_mode = ui.ctx().style().visuals.dark_mode; + draw_modal_overlay(ui.ctx(), "token_info_popup_overlay"); + let window_response = egui::Window::new("Token Configuration Details")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/tokens/tokens_screen/my_tokens.rs` around lines 177 - 216, The popup still allows click-through because the shared modal overlay isn't painted before the window is shown and the outside-click check runs; to fix, when self.show_token_info_popup.is_some() (the same condition that drives Token Configuration Details / is_open / close_popup), paint the modal overlay on the UI context before creating/showing the egui::Window (i.e. before calling egui::Window::new(...).show) so the overlay consumes the outside click; you can do this by painting a fullscreen translucent rect via ui.ctx().layer_painter or an appropriate LayerId/screen rect paint call using ui.ctx().input().screen_rect (or ctx.screen_rect()) and DashColors::overlay(dark_mode) so clicked_outside_window(ui.ctx(), wr.response.rect) no longer allows the original click to pass through to the token list underneath.
🧹 Nitpick comments (1)
src/ui/components/modal_overlay.rs (1)
9-21: Add a regression test for the shared overlay path.This helper is now the single choke point for the pass-through fix. A UI test that opens a modal, clicks outside it, and asserts the background widget does not fire would protect every call site that now depends on this behavior.
As per coding guidelines, "UI integration tests should be located in
tests/kittest/."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/modal_overlay.rs` around lines 9 - 21, Add a UI integration test under tests/kittest that exercises the shared overlay path by opening a modal that uses draw_modal_overlay (ensure the code path uses DashColors::modal_overlay and ui.allocate_response with egui::Sense::click_and_drag), simulate a click outside the modal, and assert the background widget's click handler was not invoked; the test should construct the UI to call draw_modal_overlay, trigger a pointer event outside the modal bounds, and verify that only the modal consumed the event (i.e., the background widget's state/callback was unchanged) to prevent regressions for the pass-through fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/components/selection_dialog.rs`:
- Around line 180-181: The shared modal overlay used by selection_dialog (called
via draw_modal_overlay) is drawn with egui::Order::Background which doesn't
block hit-testing for higher-order windows; update the modal overlay helper in
modal_overlay.rs to use egui::Order::Foreground (matching egui::Modal) so it
reliably blocks interactions, and ensure the overlay still fills
ctx.content_rect() and uses the same response/hit area logic as before (preserve
the existing draw_modal_overlay function name and its call sites like in
selection_dialog.rs).
---
Outside diff comments:
In `@src/ui/tokens/tokens_screen/my_tokens.rs`:
- Around line 177-216: The popup still allows click-through because the shared
modal overlay isn't painted before the window is shown and the outside-click
check runs; to fix, when self.show_token_info_popup.is_some() (the same
condition that drives Token Configuration Details / is_open / close_popup),
paint the modal overlay on the UI context before creating/showing the
egui::Window (i.e. before calling egui::Window::new(...).show) so the overlay
consumes the outside click; you can do this by painting a fullscreen translucent
rect via ui.ctx().layer_painter or an appropriate LayerId/screen rect paint call
using ui.ctx().input().screen_rect (or ctx.screen_rect()) and
DashColors::overlay(dark_mode) so clicked_outside_window(ui.ctx(),
wr.response.rect) no longer allows the original click to pass through to the
token list underneath.
---
Nitpick comments:
In `@src/ui/components/modal_overlay.rs`:
- Around line 9-21: Add a UI integration test under tests/kittest that exercises
the shared overlay path by opening a modal that uses draw_modal_overlay (ensure
the code path uses DashColors::modal_overlay and ui.allocate_response with
egui::Sense::click_and_drag), simulate a click outside the modal, and assert the
background widget's click handler was not invoked; the test should construct the
UI to call draw_modal_overlay, trigger a pointer event outside the modal bounds,
and verify that only the modal consumed the event (i.e., the background widget's
state/callback was unchanged) to prevent regressions for the pass-through fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 133d707e-3345-422e-a436-1c434a3d5cfa
📒 Files selected for processing (13)
src/ui/components/confirmation_dialog.rssrc/ui/components/info_popup.rssrc/ui/components/mod.rssrc/ui/components/modal_overlay.rssrc/ui/components/selection_dialog.rssrc/ui/components/wallet_unlock_popup.rssrc/ui/dashpay/profile_screen.rssrc/ui/identities/identities_screen.rssrc/ui/tokens/tokens_screen/data_contract_json_pop_up.rssrc/ui/tokens/tokens_screen/my_tokens.rssrc/ui/wallets/add_new_wallet_screen.rssrc/ui/wallets/asset_lock_detail_screen.rssrc/ui/wallets/wallets_screen/dialogs.rs
|
it doesn't work, when on Wallet I click "receive", the overlay is above both the main window and dialog. Expected: only below dialog. |
|
@lklimek Good catch — the overlay was using Added doc comments explaining the ordering rationale and warning against |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
claw resolve conflicts |
Centralize modal overlay painting into a shared helper that both renders the semi-transparent background and consumes pointer events, preventing clicks from reaching background widgets. Uses egui::Area with allocate_response(Sense::click_and_drag()) to intercept all pointer events on the overlay rect. Closes dashpay#704
- Use Order::Foreground instead of Order::Background for the modal overlay Area, matching egui's own Modal implementation. Higher-order layers win hit-testing, so Background couldn't reliably block interactions with windows/popups above it. - Add draw_modal_overlay() call before Token Configuration Details popup in my_tokens.rs, which was missing the shared overlay and still allowed click-through to the token list underneath.
Order::Foreground places the overlay above egui::Window dialogs (which default to Order::Middle), blocking interaction with the dialog itself. This was reported by lklimek: clicking 'receive' on the Wallet screen showed the overlay above both the background and the dialog. Revert to Order::Background — the Area with allocate_response (Sense::click_and_drag) still blocks click-through to background panel widgets because, within the same order, this later-drawn Area wins hit-testing over earlier-drawn panel layers. Add doc comments explaining the ordering rationale and warning against Order::Foreground.
Head branch was pushed to by a user without write access
thepastaclaw
force-pushed
the
fix/modal-overlay-click-through
branch
from
5d5b411 to
7957db7
Compare
|
✅ Review complete (commit 547e854) |
|
@lklimek Conflicts resolved — rebased onto latest v1.0-dev and force-pushed. Note: upstream v1.0-dev currently has a pre-existing build failure ( |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ui/components/selection_dialog.rs (1)
189-189: Consider consistent window ordering across dialog components.
SelectionDialogusesOrder::Foregroundfor its window, whileInfoPopupand other dialogs use the defaultOrder::Middle. Both work correctly with theOrder::Backgroundoverlay, but the inconsistency could cause unexpected z-ordering if dialogs ever overlap. Consider aligning all dialogs to use the same window order for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/selection_dialog.rs` at line 189, SelectionDialog sets its window order to egui::Order::Foreground which is inconsistent with InfoPopup and other dialogs that use the default/egui::Order::Middle; update the window creation in SelectionDialog (where .order(egui::Order::Foreground) is called—likely in the SelectionDialog show/build method) to use egui::Order::Middle (or otherwise match the shared ordering used by InfoPopup) so all dialog components have consistent z-ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ui/components/selection_dialog.rs`:
- Line 189: SelectionDialog sets its window order to egui::Order::Foreground
which is inconsistent with InfoPopup and other dialogs that use the
default/egui::Order::Middle; update the window creation in SelectionDialog
(where .order(egui::Order::Foreground) is called—likely in the SelectionDialog
show/build method) to use egui::Order::Middle (or otherwise match the shared
ordering used by InfoPopup) so all dialog components have consistent z-ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6784ec3b-4df1-40ac-9f48-5778a84022ee
📒 Files selected for processing (12)
src/ui/components/confirmation_dialog.rssrc/ui/components/info_popup.rssrc/ui/components/selection_dialog.rssrc/ui/components/wallet_unlock_popup.rssrc/ui/dashpay/profile_screen.rssrc/ui/helpers.rssrc/ui/identities/identities_screen.rssrc/ui/tokens/tokens_screen/data_contract_json_pop_up.rssrc/ui/tokens/tokens_screen/my_tokens.rssrc/ui/wallets/add_new_wallet_screen.rssrc/ui/wallets/asset_lock_detail_screen.rssrc/ui/wallets/wallets_screen/dialogs.rs
✅ Files skipped from review due to trivial changes (3)
- src/ui/components/confirmation_dialog.rs
- src/ui/components/wallet_unlock_popup.rs
- src/ui/wallets/wallets_screen/dialogs.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/ui/identities/identities_screen.rs
- src/ui/dashpay/profile_screen.rs
- src/ui/tokens/tokens_screen/my_tokens.rs
- src/ui/wallets/add_new_wallet_screen.rs
- src/ui/tokens/tokens_screen/data_contract_json_pop_up.rs
Per review feedback from lklimek: no need for a new file, move the function to an existing file. helpers.rs is the best fit since it already contains the related clicked_outside_window() helper. - Moved draw_modal_overlay() to src/ui/helpers.rs - Updated 11 import paths across the codebase - Removed src/ui/components/modal_overlay.rs - Removed pub mod modal_overlay from components/mod.rs
thepastaclaw
force-pushed
the
fix/modal-overlay-click-through
branch
from
7957db7 to
547e854
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR #732 introduces a draw_modal_overlay() helper that creates an egui::Area with Order::Background and Sense::click_and_drag() to block pointer events from reaching background widgets. It replaces inline overlay painting across 11 callsites. 4 commits: initial fix, CodeRabbit feedback, Order::Background fix, and refactor moving the function to helpers.rs. The wallet_lifecycle.rs change (FeatureGate refactor) is unrelated scope creep but harmless.
Reviewed commit: 547e854
💬 2 nitpick(s)
1 additional finding
💬 nitpick: Unrelated FeatureGate change in wallet_lifecycle.rs
src/context/wallet_lifecycle.rs (line 1)
Commit 547e854 (refactor: move draw_modal_overlay) includes a 3-line change in src/context/wallet_lifecycle.rs replacing self.supports_shielded() with FeatureGate::Shielded.is_available(self). This is unrelated to the modal overlay fix — it's a FeatureGate API migration that was bundled into the refactor commit. The change is correct and harmless, but strictly out of PR scope.
|
@lklimek quick status check from my side: the requested changes on this PR were already addressed in code — the overlay ordering fix landed in I re-checked the current head and reran verification on
I am not making additional code changes here because the only remaining open note was a non-blocking self-review nitpick about explicit If you still want anything else changed on top of the current head, I can pick it up — otherwise this should be ready for re-review. |
|
Fresh pass on current head
So from my side there is not a new code change to make here — the requested code move is already present on the PR branch. If there is still something blocking on the current head, please point me at the specific current-head issue/thread and I'll pick it up. |
2 participants
Issue
Closes #704
Summary
All modal overlays used
painter.rect_filled()vialayer_painter(Order::Background)which is purely visual — clicks pass through the overlay to background widgets in the same frame.Changes
src/ui/components/modal_overlay.rs— centralizeddraw_modal_overlay(ctx, id)helper that usesegui::Areawithallocate_response(Sense::click_and_drag())to both paint the semi-transparent overlay and consume all pointer eventsWalletsBalancesScreen::draw_modal_overlaymethod fromdialogs.rsNet: -29 lines (59 added, 88 removed across 13 files).
Validation
cargo check— compiles cleancargo clippy -- -D warnings— no warningscargo fmt --all -- --check— formattedmodal_overlay()references now go through the centralized helper (grep confirms no remaining inlinerect_filled+modal_overlaypatterns)Summary by CodeRabbit