Closed Bug 1406446 Opened 2 years ago Closed 2 years ago

Expose EventStateManager::IsHandlingUserInput() (or equivalent) in InputContextAction

Categories

(Core :: Widget, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: snorp, Assigned: masayuki)

References

Details

Attachments

(2 files)

I changed Android to ignore input focus unless we have EventStateManager::IsUserAction() (bug 1402461). Masayuki mentioned that we should make that information easily accessible  on all platforms, so we can handle that in a followup here.
Masayuki, InputContextAction currently has the Cause enum which indicates if the change came from keyboard, mouse, touch, etc. It also has IsUserAction() which just checks to see if the cause is one of those. How do you propose we avoid confusion with a new bool filled in from EventStateManager::IsHandlingInput()?
Flags: needinfo?(masayuki)
Priority: -- → P3
Okay, taking. This needs some changes in ESM and ISM.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)
Gijs:

Could you test on screen keyboard behavior of this build?
https://queue.taskcluster.net/v1/task/VFQYJLXZSwydxCCngJgV0A/runs/0/artifacts/public/build/target.zip

After landing a patch for bug 1402461, Firefox for Android started to change when virtual keyboard is shown. I'm trying to use similar condition even on Windows.  The different point is, even if JS changes focus or IME state, we should open on screen keyboard if we're dispatching an event of user input, e.g., click event. There are some testcases: attachment 8911330 [details] and attachment 8914517 [details].
Flags: needinfo?(gijskruitbosch+bugs)
Hi, snorp:

Could you check if this test build works as you expected?
ARM: https://queue.taskcluster.net/v1/task/ZSlt912FThClgkvaADBQ_Q/runs/0/artifacts/public/build/target.apk
x86: https://queue.taskcluster.net/v1/task/WkDT6mWbQeKfVyHDc_ih-Q/runs/0/artifacts/public/build/target.apk

InputContextAction::UserMightRequestOpenVKB() returns true if we're dispatching when non-keyboard event. I.e., this makes Firefox for Android won't show VKB when keyboard events cause changing focus or input context via JS. If you think that we should show VKB even when we're dispatching keyboard event, let me know.
Flags: needinfo?(snorp)
Your patch seems to behave the same as my original one, AFAICT. It also doesn't regress bug 1405403, which is good. It doesn't fix bug 1409113, though, so I think we may still have some edge cases to handle.
Flags: needinfo?(snorp)
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #5)
> Gijs:
> 
> Could you test on screen keyboard behavior of this build?
> https://queue.taskcluster.net/v1/task/VFQYJLXZSwydxCCngJgV0A/runs/0/
> artifacts/public/build/target.zip
> 
> After landing a patch for bug 1402461, Firefox for Android started to change
> when virtual keyboard is shown. I'm trying to use similar condition even on
> Windows.  The different point is, even if JS changes focus or IME state, we
> should open on screen keyboard if we're dispatching an event of user input,
> e.g., click event. There are some testcases: attachment 8911330 [details]
> and attachment 8914517 [details].

Yeah, the test build works better than 57, on win10 on my surface pro. Thank you! (and sorry for the PTO-induced delay)
Flags: needinfo?(gijskruitbosch+bugs)
Thank you very much for your test! I'll request review soon.
(Trying to understand what this bug is about)
Comment on attachment 8924584 [details]
Bug 1406446 - part 1: InputContextAction should treat focus change during handling a user input as caused by user input even if it's caused by JS

https://reviewboard.mozilla.org/r/195816/#review201018

ok, maybe I understand this :)

::: dom/events/EventStateManager.h:236
(Diff revision 1)
>  
>    nsresult SetCursor(int32_t aCursor, imgIContainer* aContainer,
>                       bool aHaveHotspot, float aHotspotX, float aHotspotY,
>                       nsIWidget* aWidget, bool aLockCursor);
>  
> -  static void StartHandlingUserInput()
> +  static void StartHandlingUserInput(EventMessage aMessage);

These methods need now some comment about what the param means here. Why does it matter which message is passed?

::: dom/events/EventStateManager.h:1082
(Diff revision 1)
>    // The number of user inputs handled since process start. This
>    // includes anything that is initiated by user, with the exception
>    // of page load events or mouse over events.
>    static uint64_t sUserInputCounter;
>  
> -  // The current depth of user inputs. This includes anything that is
> +  // The current depth of user and keyboard inputs. This includes anything

I don't understand this. What is the difference between user and keyboard inputs?
isn't keyboard input user input. Perhaps clarify the comment a bit.
Attachment #8924584 - Flags: review?(bugs) → review+
Comment on attachment 8924585 [details]
Bug 1406446 - part 2: Android widget should use API of InputContextAction rather than accessing EventStateManager

https://reviewboard.mozilla.org/r/195818/#review201082
Attachment #8924585 - Flags: review?(nchen) → review+
Comment on attachment 8924584 [details]
Bug 1406446 - part 1: InputContextAction should treat focus change during handling a user input as caused by user input even if it's caused by JS

https://reviewboard.mozilla.org/r/195816/#review201018

> These methods need now some comment about what the param means here. Why does it matter which message is passed?

It is necessary for checking if current input event is a keyboard event.
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/a6e70d92a22f
part 1: InputContextAction should treat focus change during handling a user input as caused by user input even if it's caused by JS r=smaug
https://hg.mozilla.org/integration/autoland/rev/a5dd465e0bc2
part 2: Android widget should use API of InputContextAction rather than accessing EventStateManager r=jchen
https://hg.mozilla.org/mozilla-central/rev/a6e70d92a22f
https://hg.mozilla.org/mozilla-central/rev/a5dd465e0bc2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.