FIX: Touchscreen losing focus then input fails on UI afterwards#2431
FIX: Touchscreen losing focus then input fails on UI afterwards#2431Darren-Kelly-Unity wants to merge 8 commits into
Conversation
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2431 +/- ##
============================================
+ Coverage 58.58% 79.02% +20.44%
============================================
Files 738 766 +28
Lines 135831 140408 +4577
============================================
+ Hits 79570 110962 +31392
+ Misses 56261 29446 -26815 Flags with carried forward coverage won't be shown. Click here to find out more.
|
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
There was a problem hiding this comment.
This fix for touchscreen focus loss correctly addresses the reported issue, but clearing the mouse state completely introduces a regression where initial clicks after regaining focus may default to (0,0). Additionally, resetting the repeat helper on focus loss would ensure consistent navigation state.
🤖 Helpful? 👍/👎
|
/review |
|
/test_plan |
Test Plan(Custom_review updated until commit 5d5c193)Updated: 2024-06-11 - Added test cases for CI failure investigation and state restoration logic.
Summary of Changes & Risk AssessmentSummary of ChangesThis PR fixes UUM-139662 where UI elements become unresponsive to touchscreen input after an Alt+Tab cycle on Windows. The fix resets Risk Assessment
Test ScenariosFunctional Testing
Regression Testing
🔍 Regression Deep Dive (additional risks identified)
Edge Cases
💡 This test plan updates automatically when 🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr |
|
Your new unit test is failing on CI |
MorganHoarau
left a comment
MorganHoarau
left a comment
There was a problem hiding this comment.
PR description is different from actual diff after you applied bot suggestion. Please update accordingly.
| m_SeenTouchEvents = false; | ||
| m_SeenPenEvents = false; | ||
| m_ResetSeenEventsOnUpdate = false; | ||
| m_RepeatHelper.Reset(); |
There was a problem hiding this comment.
Why do you reset navigation repeat state here? Looks unrelated to pointer clickCount bug and no mentioned in PR description. Is there a separate bug for it maybe?
There was a problem hiding this comment.
It was a suggestion that seemed to make sense from u-pr below!
https://github.com/Unity-Technologies/InputSystem/pull/2431/changes#r3388665182
There was a problem hiding this comment.
If that was not a known issue, we should at least test it first before applying to know if there is an issue in the first place. It doesn't look like it could introduce any regression, but I'm just considering that any line of code not part of fixing existing a bug might be a potential liability for subtle regression.
I haven't tested locally so I'll let QA confirm
There was a problem hiding this comment.
reverted it for now!
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Persistent custom_review updated to latest commit 5d5c193 |
This comment was marked as outdated.
This comment was marked as outdated.
Pauliusd01
left a comment
Pauliusd01
left a comment
There was a problem hiding this comment.
Sorry for taking a while, I was battling my cheap touchscreen monitor and finally got it to work in 480p. I've checked the /test_plan tests and the original bug repro with a windows touch screen, mouse and pen device.
Test scene can be found here if you ever want to double check:
PR_2431_V2.zip
3 participants
Purpose of this PR
Fixes a bug where UI Toolkit elements become unresponsive to touchscreen input after an Alt+Tab cycle on Windows. The root cause was that
InputSystemProvider.OnFocusChanged()did not reset pointer state on focus loss, leavingPointerState.ClickCount(and other fields) stale across the focus boundary. The fix assigns= defaultto all three pointer states inOnFocusChanged(false), which zeroes every field includingClickCount— a full reset that.Reset()alone does not provide.Release Notes
Functional Testing status
AfterAltTab_FirstTouchPress_GeneratesCleanButtonPressedEventinInputForUIFocusRegressionTests— fails on the unfixed code (clickCount=2), passes after the fix (clickCount=1).m_Provider.OnFocusChangedis called directly sinceScheduleFocusChangedEventdrives only the InputManagerBackgroundBehaviorpath, not theIEventProviderImplpath).ResetAndDisableNonBackgroundDevices.Performance Testing Status
No performance impact. The change adds a small number of field assignments inside
OnFocusChanged, which is called at most twice per Alt+Tab cycle.Overall Product Risks
OnFocusChanged, a method that only fires on application focus transitions. No hot-path code affected.InputManagerProvideralready behaves (it polls fresh state every frame so stale state is never possible there).Class diagram
Analyzed range:
origin/main...HEADInputSystemProvider — OnFocusChanged pointer-state reset
classDiagram class IEventProviderImpl { <<interface>> } class PointerState { } class InputSystemProvider { -PointerState m_MouseState -PointerState m_TouchState -PointerState m_PenState +OnFocusChanged(bool focus) } InputSystemProvider ..|> IEventProviderImpl InputSystemProvider "1" *-- "3" PointerState note for InputSystemProvider "OnFocusChanged now resets\nm_MouseState, m_TouchState,\nand m_PenState on focus loss"