Repository navigation
FIX: [Port] [UUM-155740] [Input System] Fixed UI Toolkit receiving an extra PointerDownEvent for touch drags - #2502
benoitalain wants to merge 8 commits into
Conversation
… extra PointerDownEvent for touch drags A touch's press control reads its state from the touch phase, so the pass-through UI/Click action is performed again when the phase changes from Began to Moved. InputSystemProvider dispatched a ButtonPressed on every callback, so a single-finger drag produced two PointerDownEvents. The provider now tracks pressed inputs per binding and only dispatches a button event when the pressed state changes. Presses whose release was missed while the click action was disabled are dropped in Update. Unity.InputSystem.ForUI now allows unsafe code, to read the binding state of the callback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2502 +/- ##
===========================================
+ Coverage 78.96% 79.26% +0.29%
===========================================
Files 767 769 +2
Lines 140949 141665 +716
===========================================
+ Hits 111305 112293 +988
+ Misses 29644 29372 -272
Flags with carried forward coverage won't be shown. Click here to find out more.
|
A UI Toolkit Button pressed with the mouse stayed pressed when the application lost focus while the button was held. OnFocusChanged(false) cleared m_PressedInputs, so the release callback that follows the Input System's device reset found no record and its ButtonReleased was suppressed; the Button never received a PointerUp. The pressed records are now kept across focus loss, and ForgetReleasedInputs dispatches the ButtonReleased of each press it forgets instead of dropping it silently. This restores the 6.7 behaviour: the release reaches UI when focus returns, at the pointer's last known position. The event construction is shared between OnClickPerformed and the new release path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Disabling and re-enabling the UI action map while a pointer is held must not dispatch a ButtonReleased: IsStillPressed reads the control's device state, which a map swap does not change. The release is dispatched only once the pointer is physically released. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A press reaches OnClickPerformed and is buffered until the provider update. If the UI map is disabled and the pointer released before that update, the release only comes from ForgetReleasedInputs, which dispatched it immediately, ahead of the buffered press. UI Toolkit then ignored the unmatched release and the press captured the pointer after it was physically released. The fallback release now goes through the same queue as callback events, so it follows the press and goes through the same sorting and touch/pen priority filtering. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dictionary.Remove(key, out value) is missing from the .NET Framework API compatibility level that package users can select. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/review Written by Claude Code (AI agent) on behalf of @benoitalain. |
Touch and pen events make the provider drop mouse events until a click callback schedules a reset of the seen-event flags. A release dispatched by ForgetReleasedInputs or a focus loss skipped that step, so after a touch drag ended while the UI map was disabled, mouse events stayed filtered until the next touch click. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/review Written by Claude Code (AI agent) on behalf of @benoitalain. |
There was a problem hiding this comment.
💡 Harness Review
The change adds transition-based press filtering and fallback release handling, with broad regression coverage, but composite press tracking has a remaining lifecycle defect.
Reviewed commit 01a24a1
🤖 Helpful? 👍/👎
A fallback release used the pointer's last position from a Point callback. When the pointer moved while the UI map was disabled, the release landed on the press position and could click the element under the press. The release now reads the pointer's current position from its device. A press through a composite was released when the device of its triggering control was removed, even while the composite stayed pressed through another device. Composite presses are now checked through any control of their part, since reading a part evaluates the whole composite. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/review Written by Claude Code (AI agent) on behalf of @benoitalain. |
There was a problem hiding this comment.
💡 Harness Review
The change deduplicates pointer button events and synthesizes releases missed by callbacks. I examined press identity, action-state lifetime, pointer positioning, focus handling, and the added regression coverage, with no new findings beyond those previously reported.
Reviewed commit c41fc10
🤖 Helpful? 👍/👎
# Conflicts: # Packages/com.unity.inputsystem/CHANGELOG.md
|
|
||
| DispatchFromCallback(Event.From(new PointerEvent | ||
| // The pointer can move without Point callbacks, such as while the UI map is disabled | ||
| if (pressedInput.control.device is Pointer pointer && pointer.added) |
There was a problem hiding this comment.
A modifier composite’s fallback release uses the old mouse position when the mouse moves while the UI map is disabled. Press the mouse first, then Ctrl with ButtonWithOneModifier(modifiersOrder=2), and PressedInput.control records the keyboard. If the mouse then moves and releases with the map disabled, DispatchButtonReleased skips its position refresh because the recorded device is a keyboard. The emitted ButtonReleased therefore carries the press position instead of the release position. Resolve the composite’s pointer control for fallback positioning. Add a disabled-map regression test with the modifier pressed last.
🤖 Helpful? 👍/👎
There was a problem hiding this comment.
Declining. The fallback release exists to reset the pressed state on the UI that saw the press, not to deliver an accurate click. This case needs a click bound through a modifier composite, with the modifier pressed after the pointer button, and the UI map disabled while it is held. The release then lands at the press position, the last position the UI saw, so the pressed state still resets correctly. Resolving the composite's pointer control would add another rule to an already long list of edge cases, for a configuration the default UI actions don't use.
The code matches the trunk and 6000.7 versions of this fix. How these fallback releases should be delivered, including their position, is left for a separate change.
Written by Claude Code (AI agent) on behalf of @benoitalain.
Description
Port of the Editor fix for UUM-155740. On iOS and Android, a single-finger drag on a UI Toolkit runtime UI dispatched
PointerDownEventtwice: Down, Move, Down, Move, Up.UI/Clickis a pass-through action bound to<Touchscreen>/touch*/press, and a touch'spresscontrol reads its state from the touch phase. When the phase changes fromBegantoMoved, the press value stays 1 but the action is performed again, andInputSystemProviderdispatched a secondButtonPressed.The provider now only dispatches a button event when the pressed state of an input changes:
Updatedrops tracked presses that the action no longer evaluates as pressed, or whose device was removed, so they don't swallow the next press.The PR also includes the fix for UUM-160160, a regression of the first change found in the Editor before it shipped: a UI Toolkit Button pressed with the mouse stayed pressed when the application lost focus while the button was held (Alt+Tab with the mouse button down). Press records now survive focus loss, and when
Updatedrops a press it dispatches the matchingButtonReleased, queued behind any pending callback events so a release never reaches UI before its press. This PR stays in draft until that fix lands in the Editor.Testing status & QA
Added
InputForUITestsfor touch drags, two fingers, two touchscreens, releases while the UI map is disabled, and click bindings with processors and composites. The UUM-160160 commits add tests for focus loss while a pointer is held, a UI map swap while held, and the order of a release dropped while the UI map was disabled. Ran locally in 6000.0.51f1 batchmode, PlayMode,InputForUITests:TouchDrag_DispatchesOneButtonPressedAndOneButtonReleasedandTwoFingerDrag_DispatchesButtonEventsForEachFinger.I haven't run the full suites locally.
Overall Product Risks
Comments to reviewers
TryGetValueandRemoveinstead ofDictionary.Remove(key, out value), which the .NET Framework API compatibility level lacks. AndUnity.InputSystem.ForUInow allows unsafe code. The provider reads the callback's binding state, and in the Editor the package is built with unsafe code allowed, so the fix didn't need it there.presscontrol notifies listeners on any phase change even when the press value doesn't change. That could be worth a look on its own, but this PR doesn't change it.Written by Claude Code (AI agent) on behalf of @benoitalain.
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.