feat(ContextMenu): make the context menu appearance speed configurable when touched#7521
Merged
feat(ContextMenu): make the context menu appearance speed configurable when touched#7521
Conversation
…ouched Added parameter OnTouchDelay to ContextMenuTrigger, improve touch logic ContextMenuTrigger now supports configurable OnTouchDelay for touch event handling, using options from BootstrapBlazorOptions. Replaces hardcoded delays with parameter value, renames TouchStart to IsTouchStarted for clarity, and exposes it for testing. Adds a new test to verify touch delay behavior and state transitions. Note that ContextMenuOptions are already used in Table and TreeViewRow.
|
Thanks for your PR, @peereflits. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
Contributor
Reviewer's GuideMakes the touch-triggered context menu appearance delay configurable via a new OnTouchDelay parameter on ContextMenuTrigger, wired to BootstrapBlazorOptions.ContextMenuOptions, and updates touch handling/state plus tests to reflect the new behavior and default delay. Sequence diagram for touch-triggered context menu with configurable delaysequenceDiagram
actor User
participant Trigger as ContextMenuTrigger
participant Zone as ContextMenuZone
participant Menu as ContextMenu
User->>Trigger: TouchStart
alt Trigger not busy
Trigger->>Trigger: IsBusy = true
Trigger->>Trigger: IsTouchStarted = true
Trigger->>Trigger: Task.Delay(OnTouchDelay)
alt IsTouchStarted still true
Trigger->>Trigger: Create MouseEventArgs
Trigger->>Zone: OnContextMenu(MouseEventArgs)
Zone->>Menu: Show
Trigger->>Trigger: Task.Delay(OnTouchDelay)
end
Trigger->>Trigger: IsBusy = false
end
User->>Trigger: TouchEnd
Trigger->>Trigger: IsTouchStarted = false
Class diagram for updated ContextMenuTrigger touch configurationclassDiagram
class BootstrapComponentBase
class ContextMenuTrigger {
+ContextMenuTrigger(options IOptionsMonitor_BootstrapBlazorOptions)
+int OnTouchDelay
+bool IsTouchStarted
-bool IsBusy
-ContextMenuZone ContextMenuZone
-Task OnTouchStart(e TouchEventArgs)
-void OnTouchEnd()
-Task OnContextMenu(e MouseEventArgs)
}
class IOptionsMonitor_BootstrapBlazorOptions {
+BootstrapBlazorOptions CurrentValue
}
class BootstrapBlazorOptions {
+ContextMenuOptions ContextMenuOptions
}
class ContextMenuOptions {
+int OnTouchDelay
}
BootstrapComponentBase <|-- ContextMenuTrigger
ContextMenuTrigger --> IOptionsMonitor_BootstrapBlazorOptions : constructor_injection
IOptionsMonitor_BootstrapBlazorOptions --> BootstrapBlazorOptions : provides
BootstrapBlazorOptions --> ContextMenuOptions : has
ContextMenuTrigger --> ContextMenuOptions : reads_default_OnTouchDelay
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
ContextMenu_TouchWithTimeout_Oktest relies on real elapsed wall-clock time (StopwatchandElapsedMilliseconds >= Delay * 2), which is likely to be flaky on slower CI or under load; consider restructuring it to assert on observable state changes (e.g., via fake timers or injected delay abstraction) instead of timing thresholds. - Exposing
IsTouchStartedas a public property purely to support the new test leaks internal state into the public API; consider keeping it private/internal and validating behavior through the component’s visible effects (e.g., menu visibility) or using test-only access patterns instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `ContextMenu_TouchWithTimeout_Ok` test relies on real elapsed wall-clock time (`Stopwatch` and `ElapsedMilliseconds >= Delay * 2`), which is likely to be flaky on slower CI or under load; consider restructuring it to assert on observable state changes (e.g., via fake timers or injected delay abstraction) instead of timing thresholds.
- Exposing `IsTouchStarted` as a public property purely to support the new test leaks internal state into the public API; consider keeping it private/internal and validating behavior through the component’s visible effects (e.g., menu visibility) or using test-only access patterns instead.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/ContextMenu/ContextMenuTrigger.cs:43-46` </location>
<code_context>
+ /// Default is <see cref="ContextMenuOptions.OnTouchDelay"/> milliseconds. Must be greater than 0.
+ /// </summary>
+ [Parameter]
+ public int OnTouchDelay
+ {
+ get;
+ set { if(value > 0) field = value; }
+ } = options.CurrentValue.ContextMenuOptions.OnTouchDelay;
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Silent rejection of non-positive OnTouchDelay values may be confusing for consumers.
The setter ignores non-positive values and keeps the previous value, which can be surprising since any `int` is accepted in markup without feedback. Consider either clamping to a minimum (e.g. `field = Math.Max(1, value);`), throwing an exception for invalid values, or validating and enforcing the minimum before setting this property so the behavior is explicit and predictable.
</issue_to_address>
### Comment 2
<location> `test/UnitTest/Components/ContextMenuTest.cs:111-120` </location>
<code_context>
+ public async Task ContextMenu_TouchWithTimeout_Ok()
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid using real-time delays and Stopwatch in unit tests to reduce flakiness and improve performance.
This test uses real `Task.Delay` and `Stopwatch` and asserts `sw.ElapsedMilliseconds >= Delay * 2`, making it slow (~600ms per run for Delay=300) and prone to flakiness on slower or loaded CI. It also couples the test to the implementation detail that there are exactly two `OnTouchDelay` intervals.
Please refactor to avoid wall‑clock timing by using a much smaller delay and/or asserting on observable behavior (e.g., menu visibility or a callback invocation) rather than elapsed time. Ideally, abstract the timer behind an injectable clock/timer so tests can advance time deterministically while still verifying that `OnTouchDelay` controls when the menu appears.
Suggested implementation:
```csharp
[Fact]
public async Task ContextMenu_TouchWithTimeout_Ok()
{
// Use a much smaller delay in tests to reduce runtime and flakiness
const int Delay = 10; // ms
```
Based on your comment, more changes are needed in the same test method (and possibly in the production code) that are not visible in the snippet:
1. **Remove dependence on wall‑clock time in the test:**
- Find any usage of `Stopwatch` and `Assert.True(sw.ElapsedMilliseconds >= Delay * 2)` (or similar) inside `ContextMenu_TouchWithTimeout_Ok`.
- Replace those assertions with checks on observable behavior, such as:
- Asserting that the context menu is not visible before the "delay period" has been simulated.
- Asserting that the context menu becomes visible (or a callback is invoked) after simulating the appropriate number of touch delay intervals.
2. **Avoid real `Task.Delay` where possible:**
- If the test explicitly calls `await Task.Delay(Delay * 2)` (or similar) merely to wait out the timer, refactor to:
- Either use a much smaller delay (matching the new `Delay = 10`) and assert behavior without relying on tight timing thresholds, or
- Preferably, abstract the timing behavior in the component under test behind an injectable timer/clock service (e.g., `ITimer` or `IClock`) and:
- In production, use a real implementation (`System.Threading.Timer` / `Task.Delay`).
- In tests, use a fake implementation that allows you to advance "logical" time deterministically (e.g., `AdvanceBy(Delay)`), then assert on menu visibility or callbacks without calling `Task.Delay` at all.
3. **Clean up `using System.Diagnostics;` if no longer needed:**
- After removing `Stopwatch` from the tests, remove the `using System.Diagnostics;` directive at the top of `ContextMenuTest.cs` if nothing else in the file uses it.
4. **Keep test intent but decouple from implementation detail:**
- The test currently couples to the implementation detail that there are exactly two `OnTouchDelay` intervals by asserting `ElapsedMilliseconds >= Delay * 2`.
- Instead, drive the component through its public API:
- Simulate the touch start event.
- Simulate passage of the first delay interval using your fake timer.
- Assert that the menu is still hidden (if that’s the intended behavior).
- Simulate passage of the second delay interval.
- Assert that the menu is now visible or the expected callback has been invoked.
Because I can’t see the rest of `ContextMenu_TouchWithTimeout_Ok` or the `ContextMenuZone` implementation, you’ll need to wire the above suggestions into your existing test and component abstractions (or introduce an injectable timer) to fully eliminate real‑time waiting and flaky timing assertions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7521 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 748 748
Lines 32986 32998 +12
Branches 4585 4588 +3
=========================================
+ Hits 32986 32998 +12
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:
|
Collaborator
Author
|
@microsoft-github-policy-service agree |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes the context menu touch delay configurable by adding an OnTouchDelay parameter to ContextMenuTrigger, aligning it with the existing configuration pattern used in Table and TreeViewRow components. The default delay is changed from 200ms to 500ms, matching the global ContextMenuOptions.OnTouchDelay configuration.
Changes:
- Added
OnTouchDelayparameter toContextMenuTriggerwith dependency injection ofBootstrapBlazorOptions - Renamed
TouchStartproperty toIsTouchStartedand made it public for testing - Added unit test to verify configurable touch delay behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/BootstrapBlazor/Components/ContextMenu/ContextMenuTrigger.cs | Added constructor with DI, new OnTouchDelay parameter, renamed TouchStart to IsTouchStarted, and updated touch handlers to use configurable delay |
| test/UnitTest/Components/ContextMenuTest.cs | Added test ContextMenu_TouchWithTimeout_Ok to verify touch delay functionality and state management |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Marcel Peereboom <40403094+peereflits@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Marcel Peereboom <40403094+peereflits@users.noreply.github.com>
ArgoZhang
changed the title
ArgoZhang
approved these changes
Jan 16, 2026
This was referenced Jan 22, 2026
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
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.
The context menu appearance speed is made configurable when touched.
Link issues
fixes #7512
Summary By Copilot
Added parameter OnTouchDelay to ContextMenuTrigger, improve touch logic.
ContextMenuTrigger now supports configurable OnTouchDelay for touch event handling, using options from BootstrapBlazorOptions. Replaces hardcoded delays with parameter value, renames TouchStart to IsTouchStarted for clarity, and exposes it for testing. Adds a new test to verify touch delay behavior and state transitions.
Note that ContextMenuOptions are already used in Table and TreeViewRow.
Note that the default timeout is changed from 200ms (hardcoded) to 500ms (configurable and used in Table and TreeViewRow).
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Make the context menu touch-trigger delay configurable and align it with shared ContextMenu options.
New Features:
Enhancements:
Tests: