Closed
Bug 1406446
Opened 7 years ago
Closed 7 years ago
Expose EventStateManager::IsHandlingUserInput() (or equivalent) in InputContextAction
Categories
(Core :: Widget, enhancement, P3)
Core
Widget
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.
|
Reporter | |
Comment 1•7 years ago
|
||
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)
|
||
Updated•7 years ago
|
Priority: -- → P3
|
Assignee | |
Comment 2•7 years ago
|
||
Okay, taking. This needs some changes in ESM and ISM.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)
|
|
Assignee | |
Comment 3•7 years ago
|
||
|
|
Assignee | |
Comment 4•7 years ago
|
||
|
|
Assignee | |
Comment 5•7 years ago
|
||
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)
|
|
Assignee | |
Comment 6•7 years ago
|
||
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)
|
|
Reporter | |
Comment 7•7 years ago
|
||
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)
|
||
Comment 8•7 years ago
|
||
(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)
|
|
Assignee | |
Comment 9•7 years ago
|
||
Thank you very much for your test! I'll request review soon.
|
|
Assignee | |
Comment 10•7 years ago
|
||
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 13•7 years ago
|
||
(Trying to understand what this bug is about)
|
|
||
Comment 14•7 years ago
|
||
| mozreview-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
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 15•7 years ago
|
||
| mozreview-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+
|
|
Assignee | |
Comment 16•7 years ago
|
||
| mozreview-review-reply | ||
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.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 19•7 years ago
|
||
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
|
|
||
Comment 20•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/a6e70d92a22f
https://hg.mozilla.org/mozilla-central/rev/a5dd465e0bc2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•