Skip to content

FIX: inactive touch driving invalid synthetic click on state reset#2349

Open
MorganHoarau wants to merge 15 commits intodevelopfrom
fix/uum-100125/inactive-touch-drive-phantom-click-on-state-reset
Open

FIX: inactive touch driving invalid synthetic click on state reset#2349
MorganHoarau wants to merge 15 commits intodevelopfrom
fix/uum-100125/inactive-touch-drive-phantom-click-on-state-reset

Conversation

@MorganHoarau
Copy link
Collaborator

@MorganHoarau MorganHoarau commented Feb 18, 2026

Description

Addresses UUM-100125 (phantom click after device rotation).

Follow up PR about a UITKInputModule failing test locally: #2367

Issue:

  • After rotation, InputSystem queue a device configuration event which causes InputActionState to run initial-state catch-up in OnBeforeInitialUpdate. (See Comments To Reviewers section for overview)
  • Touch position state can still be non-default after touch end (because position is dontReset = true). So the stale state (touch position being non-default) was treated as fresh actuation and trigger unexpected callbacks.

Changes

  • Added a guard in initial-state check to skip replay for inactive touch state.
  • Added regression test: Actions_InitialStateCheckAfterConfigurationChange_DoesNotTriggerForInactiveTouch
  • Updated existing test Actions_WithMultipleCompositeBindings_WithoutEvaluateMagnitude_Works(true) to align the new expected behaviour. The test still preserves the original ISXB-98 guarantee: after enabling, real subsequent touches must still trigger the composite action correctly.

TODO:

  • Check existing documentation around the topic (Touch, device configuration event, dontReset) - need update?

Also, unlike both Button and PassThrough actions, Value actions perform what's called "initial state check" on the first input update after the action was enabled. What this does is check controls bound to the action and if they are already actuated (that is, at non-default value), the action will immediately be started and performed. What this means in practice is that when a value action is bound to, say, the left stick on a gamepad and the stick is already moved out of its resting position, then the action will immediately trigger instead of first requiring the stick to be moved slightly.
From InputActionType documentation.

  • Changelog

Testing status & QA

  • Reproduced issue in repro project.
  • Added new regression test for this bug path.
  • Ran existing tests and identified conflict with: Actions_WithMultipleCompositeBindings_WithoutEvaluateMagnitude_Works(true)

Overall Product Risks

  • Complexity: Low
  • Halo Effect: Medium - affects TouchControl that supports initial state check.

Comments to reviewers

Tracing the whole flow can help understand the broader context. DeviceConfigurationEvent flow diagram:

flowchart TD
    subgraph Native["Native / Backend"]
        A[Device rotates]
        B[Queue DeviceConfigurationEvent]
    end

    subgraph IM["InputManager"]
        C[Process DeviceConfigurationEvent]
        D[NotifyConfigurationChanged]
        E[InputActionState.OnDeviceChange ConfigurationChanged]
    end

    subgraph IASResolve["InputActionState.Resolve phase"]
        F[Full binding re-resolve]
        G[Set initialStateCheckPending = true]
    end

    subgraph IASInitial["InputActionState.OnBeforeInitialUpdate"]
        H[Loop bindings and controls]
        I{Control belongs to TouchControl?}
        J{touch.isInProgress?}
        K[Default CheckStateIsAtDefault path]
    end

    subgraph OldPath["Old behavior"]
        L[SignalStateChangeMonitor for inactive touch]
        M[Started/Performed from stale touch state]
        N[Phantom UI click]
    end

    subgraph NewPath["Current proposed fix"]
        O[Skip inactive touch control]
        P[No synthetic Started/Performed]
        Q[No phantom UI click]
    end

    A --> B
    B --> C
    C --> D
    D --> E
    E --> F
    F --> G
    G --> H
    H --> I
    I -- no --> K
    I -- yes --> J
    J -- yes --> K
    J -- no old --> L
    L --> M
    M --> N
    J -- no new --> O
    O --> P
    P --> Q
Loading

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
    • Jira port for the next release set as "Resolved".
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

Add ShouldSkipInitialStateCheck and use it in InputActionState's initial-state loop to avoid treating preserved touch data as actuated during binding re-resolution.
@MorganHoarau MorganHoarau changed the title Fixes inactive touch driving invalid synthetic click on state reset FIX: inactive touch driving invalid synthetic click on state reset Feb 18, 2026
@MorganHoarau MorganHoarau marked this pull request as ready for review March 3, 2026 09:40
@u-pr
Copy link
Contributor

u-pr bot commented Mar 3, 2026

PR Reviewer Guide 🔍

(Review updated until commit ea8b067)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

Moderate effort: small but behavior-changing runtime logic in action initial-state processing plus new/updated tests that need careful validation for determinism and intended semantics.

🏅 Score: 88

Solid, targeted fix with regression coverage, but the change alters initial-state behavior for touch bindings and one new test assertion looks potentially brittle across internal action-state variations.

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Test Robustness

The new regression test assumes exactly one trace event and that it is always Canceled after a configuration change; depending on whether the re-resolve actually results in a cancel for this action in a given runtime/path, this may be flaky (could be zero events or additional events). Consider asserting the absence of Started/Performed rather than requiring an exact count/phase, or otherwise tightening the setup to guarantee a cancel.

// Configuration change causes full re-resolve and schedules initial state check.
InputSystem.QueueConfigChangeEvent(touchscreen);
InputSystem.Update();
InputSystem.Update();

// Full re-resolve may cancel the current action state. What must NOT happen is a synthetic
// Started/Performed pair from persisted inactive touch state.
Assert.AreEqual(1, trace.count);
foreach (var eventPtr in trace)
{
    // The trace should only contain a Canceled event for the action.
    Assert.AreEqual(InputActionPhase.Canceled, eventPtr.phase, 
        $"inactive touch state should not produce action callbacks, but received {eventPtr.phase}.");
}
Behavior Change

Skipping initial-state catch-up for any control under a TouchControl when isInProgress is false prevents replay from persisted dontReset state (desired), but it also changes semantics for value bindings that may intentionally want “last known touch position/touchId” even when not in progress; confirm this is acceptable for all touch sub-controls and doesn’t break existing user expectations beyond the intended fix.

            if (ShouldSkipInitialStateCheck(control))
                continue;

            if (!control.CheckStateIsAtDefault())
            {
                // Update press times.
                if (control.IsValueConsideredPressed(control.magnitude))
                {
                    // ReSharper disable once CompareOfFloatsByEqualityOperator
                    if (bindingState.pressTime == default || bindingState.pressTime > time)
                        bindingState.pressTime = time;
                }

                // For composites, any one actuated control will lead to the composite being
                // processed as a whole so we can stop here. This also ensures that we are
                // not triggering the composite repeatedly if there are multiple actuated
                // controls bound to its parts.
                if (isComposite && didFindControlToSignal)
                    continue;

                manager.SignalStateChangeMonitor(control, this);
                didFindControlToSignal = true;
            }
        }
    }
    manager.FireStateChangeNotifications();

    k_InputInitialActionStateCheckMarker.End();
}

private static bool ShouldSkipInitialStateCheck(InputControl control)
{
    // UUM-100125
    // Touch controls intentionally preserve state such as position even when no touch is currently active.
    // During binding re-resolution this can make inactive touches look actuated and cause invalid triggers.
    for (var current = control; current != null; current = current.parent)
    {
        if (current is TouchControl touchControl)
        {
            return !touchControl.isInProgress;
        }
    }

    return false;
}
Performance/Scope

The parent-walk in ShouldSkipInitialStateCheck runs for every bound control during initial-state checks; it’s likely fine given the frequency, but validate it doesn’t become a hot spot with many bindings/controls, and consider a cheaper predicate (e.g., fast-path for control.device is Touchscreen) if needed.

private static bool ShouldSkipInitialStateCheck(InputControl control)
{
    // UUM-100125
    // Touch controls intentionally preserve state such as position even when no touch is currently active.
    // During binding re-resolution this can make inactive touches look actuated and cause invalid triggers.
    for (var current = control; current != null; current = current.parent)
    {
        if (current is TouchControl touchControl)
        {
            return !touchControl.isInProgress;
        }
    }

    return false;
}
  • Update review

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr

@u-pr
Copy link
Contributor

u-pr bot commented Mar 3, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Skip check for nested touch children

This only checks control.parent, so bindings to deeper children (e.g.
/primaryTouch/position/x) can still trigger from persisted inactive touch state.
Walk up the full parent chain until a TouchControl is found, and base the skip
decision on that.

Packages/com.unity.inputsystem/InputSystem/Actions/InputActionState.cs [1354-1370]

 private static bool ShouldSkipInitialStateCheck(InputControl control)
 {
     // UUM-100125
     // Touch controls intentionally preserve state such as position even when no touch is currently active.
     // During binding re-resolution this can make inactive touches look actuated and cause invalid triggers.
-    if (control is TouchControl touchControl)
+    for (var current = control; current != null; current = current.parent)
     {
-        return !touchControl.isInProgress;
-    }
-
-    if (control.parent is TouchControl parentTouchControl)
-    {
-        return !parentTouchControl.isInProgress;
+        if (current is TouchControl touchControl)
+            return !touchControl.isInProgress;
     }
 
     return false;
 }
Suggestion importance[1-10]: 8

__

Why: The current implementation only checks the control and its direct parent for a TouchControl. For bindings to nested controls, such as <Touchscreen>/primaryTouch/position/x, the check would fail to find the TouchControl and incorrectly skip the suppression logic.

Medium
  • More suggestions

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr

@codecov-github-com
Copy link

codecov-github-com bot commented Mar 3, 2026

Codecov Report

All modified and coverable lines are covered by tests ✅

@@             Coverage Diff             @@
##           develop    #2349      +/-   ##
===========================================
+ Coverage    77.90%   77.92%   +0.01%     
===========================================
  Files          476      482       +6     
  Lines        97613    97789     +176     
===========================================
+ Hits         76048    76204     +156     
- Misses       21565    21585      +20     
Flag Coverage Δ
inputsystem_MacOS_2022.3 5.30% <0.00%> (-0.23%) ⬇️
inputsystem_MacOS_2022.3_project 75.43% <100.00%> (+0.03%) ⬆️
inputsystem_MacOS_6000.0 5.27% <0.00%> (-0.04%) ⬇️
inputsystem_MacOS_6000.0_project 77.33% <100.00%> (+0.04%) ⬆️
inputsystem_MacOS_6000.3 5.27% <0.00%> (-0.04%) ⬇️
inputsystem_MacOS_6000.3_project 77.33% <100.00%> (+0.04%) ⬆️
inputsystem_MacOS_6000.4 5.28% <0.00%> (-0.04%) ⬇️
inputsystem_MacOS_6000.4_project 77.34% <100.00%> (+0.04%) ⬆️
inputsystem_MacOS_6000.5 5.28% <0.00%> (-0.04%) ⬇️
inputsystem_MacOS_6000.5_project 77.32% <100.00%> (+0.02%) ⬆️
inputsystem_MacOS_6000.6 5.28% <0.00%> (-0.04%) ⬇️
inputsystem_MacOS_6000.6_project 77.34% <100.00%> (+0.03%) ⬆️
inputsystem_Ubuntu_2022.3_project 75.23% <100.00%> (+0.03%) ⬆️
inputsystem_Ubuntu_6000.0 5.28% <0.00%> (-0.04%) ⬇️
inputsystem_Ubuntu_6000.0_project 77.14% <100.00%> (+0.04%) ⬆️
inputsystem_Ubuntu_6000.3 5.28% <0.00%> (-0.04%) ⬇️
inputsystem_Ubuntu_6000.3_project 77.13% <100.00%> (+0.03%) ⬆️
inputsystem_Ubuntu_6000.4 5.28% <0.00%> (-0.04%) ⬇️
inputsystem_Ubuntu_6000.4_project 77.16% <100.00%> (+0.04%) ⬆️
inputsystem_Ubuntu_6000.5 5.28% <0.00%> (-0.04%) ⬇️
inputsystem_Ubuntu_6000.5_project 77.13% <100.00%> (+0.01%) ⬆️
inputsystem_Ubuntu_6000.6 5.28% <0.00%> (-0.04%) ⬇️
inputsystem_Ubuntu_6000.6_project 77.14% <100.00%> (+0.03%) ⬆️
inputsystem_Windows_2022.3 5.30% <0.00%> (-0.23%) ⬇️
inputsystem_Windows_2022.3_project 75.56% <100.00%> (+0.03%) ⬆️
inputsystem_Windows_6000.0 5.27% <0.00%> (-0.04%) ⬇️
inputsystem_Windows_6000.0_project 77.46% <100.00%> (+0.03%) ⬆️
inputsystem_Windows_6000.3 5.27% <0.00%> (-0.04%) ⬇️
inputsystem_Windows_6000.3_project 77.46% <100.00%> (+0.03%) ⬆️
inputsystem_Windows_6000.4 5.28% <0.00%> (-0.04%) ⬇️
inputsystem_Windows_6000.5 5.28% <0.00%> (-0.04%) ⬇️
inputsystem_Windows_6000.5_project 77.45% <100.00%> (+0.02%) ⬆️
inputsystem_Windows_6000.6 5.28% <0.00%> (-0.04%) ⬇️
inputsystem_Windows_6000.6_project 77.46% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
Assets/Tests/InputSystem/CoreTests_Actions.cs 98.41% <100.00%> (+0.26%) ⬆️
...nputsystem/InputSystem/Actions/InputActionState.cs 92.88% <100.00%> (-0.12%) ⬇️

... and 7 files with indirect coverage changes

Addresses U-PR feedback
Previously the code only checked the control and its immediate parent for a TouchControl, which could miss deeper nested touch controls and cause inactive touches to appear actuated during binding re-resolution. Replace the checks with a loop that walks up the control hierarchy and returns based on the first ancestor TouchControl's isInProgress value, preventing invalid triggers from preserved touch state.
…ate-reset' of https://github.com/Unity-Technologies/InputSystem into fix/uum-100125/inactive-touch-drive-phantom-click-on-state-reset
@u-pr
Copy link
Contributor

u-pr bot commented Mar 5, 2026

Persistent review updated to latest commit ea8b067

@u-pr
Copy link
Contributor

u-pr bot commented Mar 8, 2026

PR Code Suggestions ✨

No code suggestions found for the PR.

Copy link
Collaborator

@ekcoh ekcoh left a comment

Choose a reason for hiding this comment

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

I think this fix is cleanly executed and really like that tests have been added for this specific scenario. I still wonder if a touch-specific solution is really needed, but see no reason to not land this now. We can always revisit this together with later interaction robustness improvements if needed since all is internal. Good job on sorting this out @MorganHoarau!

@MorganHoarau MorganHoarau requested a review from Pauliusd01 March 16, 2026 10:19
@Pauliusd01
Copy link
Collaborator

I will probably only be able to take a look tomorrow

@Pauliusd01

This comment was marked as off-topic.

@u-pr
Copy link
Contributor

u-pr bot commented Mar 17, 2026

Test Plan

  • Test plan approved by PR author
  • Test execution in progress
  • Test execution complete
  • Ready for merge

Summary of Changes & Risk Assessment

Summary of Changes

This PR addresses UUM-100125 where device rotation (triggering a configuration change) caused "phantom" clicks or action triggers. This happened because InputActionState would replay the "initial state" of controls after re-resolving bindings; since touch positions are preserved even after a touch ends (dontReset = true), the system incorrectly treated these stale values as fresh actuations. The fix introduces a guard in InputActionState.ShouldSkipInitialStateCheck to ignore controls belonging to a TouchControl that is not currently in progress.

Risk Assessment

  • High Risk Areas: None identified.
  • Medium Risk Areas: Input Action lifecycle (specifically OnBeforeInitialUpdate), Touch-based interactions in UIToolkit/IM GUI.
  • Low Risk Areas: Performance of the parent-walk check in ShouldSkipInitialStateCheck.

Test Scenarios

Functional Testing

  • Verify Regression Test: Run the new test Actions_InitialStateCheckAfterConfigurationChange_DoesNotTriggerForInactiveTouch in CoreTests_Actions.cs:L1407.
  • UIToolkit Click Verification: Create a simple UI with a Button. Perform a touch, release it, then rotate the device (or simulate QueueConfigChangeEvent). Verify no ClickEvent or PointerDown is fired unexpectedly.
  • Active Touch Continuity: Verify that if a touch is in progress during a configuration change (e.g., rotating while holding a finger down), the action state is maintained or correctly re-evaluated without losing the "Pressed" state.

Regression Testing

  • Composite Bindings: Run Actions_WithMultipleCompositeBindings_WithoutEvaluateMagnitude_Works in CoreTests_Actions.cs:L12489 to ensure the updated behavior for prepopulated touches is correct.
  • Non-Touch Devices: Verify that Mouse and Gamepad initial state checks still work correctly (e.g., a stick held in a non-default position when an action is enabled should still trigger the action).
  • Nested Touch Controls: Verify that bindings to deep children like <Touchscreen>/touch0/position/x are correctly skipped when inactive, as implemented in InputActionState.cs:L1354.

Edge Cases

  • Multi-Touch Mix: Perform two touches; end one, keep the other active. Trigger a config change. Verify only the active touch continues to drive actions.
  • Fast Taps: Ensure that a very fast tap immediately followed by a rotation doesn't result in a dropped "End" phase or a stuck "Started" phase.

💡 This test plan updates automatically when /test_plan is run on new commits. If you have any feedback, please reach out in #ai-qa


🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants