Conversation
This is a prposed solution, based on logical deduction. It is hard for me to test this.
|
Thanks for your PR, @peereflits. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
Reviewer's GuideAdds an optional behavior to hide the context menu while a touch scroll is in progress and wires a new touchmove handler into the ContextMenuTrigger rendering logic, including refactoring of RenderTree sequence numbers. Sequence diagram for context menu touch scroll invisibilitysequenceDiagram
actor User
participant BrowserDom
participant ContextMenuTrigger
User->>BrowserDom: touchstart
BrowserDom->>ContextMenuTrigger: OnTouchStart(TouchEventArgs)
activate ContextMenuTrigger
ContextMenuTrigger-->>ContextMenuTrigger: IsTouchStarted = true
ContextMenuTrigger-->>ContextMenuTrigger: IsBusy = true
deactivate ContextMenuTrigger
User->>BrowserDom: touchmove (scroll)
BrowserDom->>ContextMenuTrigger: OnTouchMove()
activate ContextMenuTrigger
alt IsInvisibleOnTouchMove == true
ContextMenuTrigger-->>ContextMenuTrigger: IsBusy = false
ContextMenuTrigger-->>ContextMenuTrigger: IsTouchStarted = false
ContextMenuTrigger-->>BrowserDom: hide context menu
else IsInvisibleOnTouchMove == false
ContextMenuTrigger-->>BrowserDom: no visibility change
end
deactivate ContextMenuTrigger
User->>BrowserDom: touchend
BrowserDom->>ContextMenuTrigger: OnTouchEnd()
activate ContextMenuTrigger
ContextMenuTrigger-->>ContextMenuTrigger: IsTouchStarted = false
deactivate ContextMenuTrigger
Class diagram for updated ContextMenuTrigger touch handlingclassDiagram
class ContextMenuTrigger {
+bool IsInvisibleOnTouchMove
-bool IsBusy
-bool IsTouchStarted
+void BuildRenderTree(RenderTreeBuilder builder)
-Task OnTouchStart(TouchEventArgs e)
-void OnTouchMove()
-void OnTouchEnd()
-Task OnContextMenu(MouseEventArgs e)
}
class RenderTreeBuilder
class TouchEventArgs
class MouseEventArgs
ContextMenuTrigger ..> RenderTreeBuilder : builds
ContextMenuTrigger ..> TouchEventArgs : handles
ContextMenuTrigger ..> MouseEventArgs : handles
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7822 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 764 764
Lines 34131 34142 +11
Branches 4699 4701 +2
=========================================
+ Hits 34131 34142 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
OnTouchMovehandler is registered asEventCallback.Factory.Create<TouchEventArgs>(this, OnTouchMove)but the method signatureprivate void OnTouchMove()does not accept aTouchEventArgsparameter, which will cause a mismatch; either add aTouchEventArgsparameter or adjust theEventCallbackcreation. - When cancelling a touch invocation on
OnTouchMoveby settingIsBusy = falseandIsTouchStarted = false, consider also cancelling any pending delay logic associated withOnTouchDelay(if present elsewhere) to avoid a delayed menu opening after a scroll.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `OnTouchMove` handler is registered as `EventCallback.Factory.Create<TouchEventArgs>(this, OnTouchMove)` but the method signature `private void OnTouchMove()` does not accept a `TouchEventArgs` parameter, which will cause a mismatch; either add a `TouchEventArgs` parameter or adjust the `EventCallback` creation.
- When cancelling a touch invocation on `OnTouchMove` by setting `IsBusy = false` and `IsTouchStarted = false`, consider also cancelling any pending delay logic associated with `OnTouchDelay` (if present elsewhere) to avoid a delayed menu opening after a scroll.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
Adjusts ContextMenuTrigger touch handling so a long-press context menu can be suppressed when the user starts scrolling (touch-move), addressing #7821 for touch devices (e.g., MAUI).
Changes:
- Added
IsInvisibleOnTouchMove[Parameter]to opt into canceling the long-press menu when a touch-move occurs. - Wired up an
ontouchmovehandler to reset internal touch state during scrolling. - Refactored
BuildRenderTreesequence numbers to use an incrementing index to accommodate the new attribute.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This is a proposed solution, based on logical deduction.
It is hard for me to test this.
Link issues
fixes #7821
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Ensure the context menu can be configured to hide while the user is scrolling on touch devices.
New Features:
Bug Fixes:
Enhancements: