1233006 - On-screen keyboard is not displayed for google docs when tapping to focus text in the doc

Status: NEW

(Core :: Widget: Win32, defect, P3)

Description

[Vasilica Tamas, QA [:vtamas]]

Note: this is a follow-up for Bug 1226145

Reproducible on: Firefox 46.0a1 and Firefox 44.0a2 under Windows 


STR
1.Launch Firefox with a clean profile.
2.Navigate to https://goo.gl/sPqJWb

ER
The OSK is successfully displayed.

AR
The OSK does not appear.


Additional notes:
- Reproducible on: Firefox 46.0a1 (2015-12-15) and Firefox 44.0a2 (2015-12-14) under Windows 10 64-bit.
- This testing was performed on a Dell Xps 12.
- This issue reproduces in both on/off Tablet Mode ways.

Comment 1

[Makoto Kato [:m_kato]]

Google docs use focus method() for iframe when user focus document body.  But OSK will be opened by mouse or touch only.

Comment 2

[:Gijs (he/him)]

(In reply to Makoto Kato [:m_kato] from comment #1)
> Google docs use focus method() for iframe when user focus document body. 
> But OSK will be opened by mouse or touch only.

Hmm, at the moment, there are still cases where we open it for automatic focus, depending on the machine. We're looking to get rid of those (bug 1239744), but it'd still somewhat surprise me if this was the reason... I guess I should investigate further.

Is normal IME support generally working with gdocs, or do they provide their own thing or something?

Comment 3

[Makoto Kato [:m_kato]]

(In reply to :Gijs Kruitbosch from comment #2)
> (In reply to Makoto Kato [:m_kato] from comment #1)
> > Google docs use focus method() for iframe when user focus document body. 
> > But OSK will be opened by mouse or touch only.
> 
> Hmm, at the moment, there are still cases where we open it for automatic
> focus, depending on the machine. We're looking to get rid of those (bug
> 1239744), but it'd still somewhat surprise me if this was the reason... I
> guess I should investigate further.
> 
> Is normal IME support generally working with gdocs, or do they provide their
> own thing or something?

Now, Google docs works with IME well.

Comment 4

[:Gijs (he/him)]

I actually can't reproduce this on latest nightly on my surface pro 3. The keyboard is shown as expected. Are you still seeing this?

Comment 5

[Makoto Kato [:m_kato]]

(In reply to :Gijs Kruitbosch from comment #4)
> I actually can't reproduce this on latest nightly on my surface pro 3. The
> keyboard is shown as expected. Are you still seeing this?

I test this today.  It seems to work fine.

Comment 6

[Vasilica Tamas, QA [:vtamas]]

(In reply to :Gijs Kruitbosch from comment #4)
> I actually can't reproduce this on latest nightly on my surface pro 3. The
> keyboard is shown as expected. Are you still seeing this?

We are still able to reproduce this issue on Firefox 47.0a1 (2016-02-01) under Windows 10 64-bit using Dell Xps 12 and Surface Pro 2.

The osk does not appear if the google doc is opened, for example, by clicking on the link from STR.

The OSK only remains displayed while switching the focus from a tab where the osk is enabled, to the google docs tab. This is the only case when the osk is displayed for google docs.

Comment 7

[Andrew Overholt [:overholt]]

I can reproduce in a clean 48 profile on a Surface Book with the latest Windows 10 updates installed. If I click on the link in comment 0, I never get the OSK.

Comment 8

[:Gijs (he/him)]

(In reply to Andrew Overholt [:overholt] from comment #7)
> I can reproduce in a clean 48 profile on a Surface Book with the latest
> Windows 10 updates installed. If I click on the link in comment 0, I never
> get the OSK.

And you're in tablet mode and/or the OSK otherwise works, right?

The thing is, two of us couldn't repro on nightly 6 months ago. It would be quite useful to figure out what is different between those who can repro and those who can't... :-\

Comment 9

[:Gijs (he/him)]

Also, IIRC the last time I tried to repro this, tapping into the document would still succeed at bringing up the keyboard. Is that not / no longer the case? Because that obviously affects the severity of the issue here...

Comment 10

[Andrew Overholt [:overholt]]

The Windows on-screen keyboard does not come up if I tap in the document (in 48 with a clean profile on Windows 10's latest updates on a Surface Book). Sorry to be the bearer of bad news. Maybe I'm doing something wrong?

Comment 11

[:Gijs (he/him)]

(In reply to Andrew Overholt [:overholt] from comment #10)
> The Windows on-screen keyboard does not come up if I tap in the document (in
> 48 with a clean profile on Windows 10's latest updates on a Surface Book).
> Sorry to be the bearer of bad news. Maybe I'm doing something wrong?

Well, it's hard to know. What about the questions in comment #8 ? Is "Tablet mode" turned on in Windows (NB: different from whatever you're doing with the hardware detach/flip/whatever), and/or have you configured windows to show the touch keyboard even in desktop mode? Does the keyboard come up when you tap in the Firefox address bar? Maybe you're just running into bug 1288219 ?

Comment 12

[Andrew Overholt [:overholt]]

I ensured I was in tablet mode and tried again with 48. The OSK doesn't come up if I navigate to the page or if I tap in the document. It *does* come up if I tap into the address bar.

Comment 13

[:Gijs (he/him)]

Seems my surface pro 3 has bricked itself, so I asked Jared to investigate here.

Comment 14

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I investigated this issue on 
- 48.0.1 build1 (20160816033124)
- 49.0b4 build1 (20160814184416)
- latest Nightly 51.0a1 (2016-08-16)
- latest Aurora 50.0a2 (2016-08-17)
using Dell Xps 12 and Windows 10 Home Insider Preview x64. The bug is reproducible with google documents, spreadsheets, presentations (not with forms), with on/off Tablet Mode and it isn't e10s related. 
In Google Chrome OSK is successfully displayed in the situation under discussion.

Comment 15

[Andrew Overholt [:overholt]]

Let me see if I can still reproduce.

Comment 16

[Andrew Overholt [:overholt]]

Clarifying this STR for future me:

1. tablet mode, clean Firefox profile, on-screen keyboard not up
2. navigate to https://goo.gl/sPqJWb
3. does the on-screen keyboard come up? (answer below)
4. tap into the document
5. does the on-screen keyboard come up? (answer below)
6. tap the address bar
7. does the on-screen keyboard come up? (answer below)

Results on a Surface Book with the latest Windows 10:

50 beta: 3. no, 5. no, 7. yes
52 nightly: 3. no, 5. no, 7. yes (same)

FWIW, a simple data:text/html,<textarea></textarea> *does* trigger the OSK so whatever custom logic we have (or don't) for Google Docs' custom editor thing isn't triggered the same way.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

I see the same results as comment #16. My breakpoint at IMEHandler::MaybeShowOnScreenKeyboard doesn't get hit when tapping on the document but does for the location bar.

Comment 18

[:Gijs (he/him)]

(In reply to [Limited availability until Oct 31] Jared Wein [:jaws] (please needinfo? me) from comment #17)
> I see the same results as comment #16. My breakpoint at
> IMEHandler::MaybeShowOnScreenKeyboard doesn't get hit when tapping on the
> document but does for the location bar.

In this case I would expect this problem to also hit IME users, unless Google is using custom code to deal with that case somehow. Masayuki, do you know more?

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I don't see any trouble of IME state in the test document but I reproduce the symptom too. But my notebook doesn't run release build of Win10, I tested on Win10 build 14931 (slow ring).

Comment 20

[:Gijs (he/him)]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #19)
> I don't see any trouble of IME state in the test document but I reproduce
> the symptom too. But my notebook doesn't run release build of Win10, I
> tested on Win10 build 14931 (slow ring).

Right, I don't think this matters.

From (remote) debugging this from my surface pro to my desktop, it seems that we hit the IMEStateManager's SetInputContextForChildProcess with the right aAction (ie a touch/click action) but there doesn't seem to be a call path from there to MaybeShowOnScreenKeyboard and friends, in part because UserMightRequestOpenVKB() only returns true when focus doesn't change (cf. https://dxr.mozilla.org/mozilla-central/rev/ade8d4a63e57560410de106450f37b50ed71cca5/widget/IMEData.h#370 ), and that isn't the case for the messages that come with ACTION_TOUCH (for reference, the callsite on Windows is here: https://dxr.mozilla.org/mozilla-central/source/widget/windows/WinIMEHandler.cpp#426 ).

Instead, there *is* a callpath from https://dxr.mozilla.org/mozilla-central/rev/ade8d4a63e57560410de106450f37b50ed71cca5/dom/ipc/TabParent.cpp#1719 (TabParent::RecvNotifyIMEFocus) through IMEStateManager::NotifyIME and IMEHandler::NotifyIME. However, by that time the last focus change cause has been set to unknown by another message (presumably this is the focus() calls  comment #1 talks about) and so we still don't open the on screen keyboard.

I'm not sure how to proceed from here, because I think we should avoid calling into MaybeShowOnScreenKeyboard multiple times for a single focus change, and I don't know the best way to include the cases here and then take away something somewhere else so we don't end up with multiple calls.

Masayuki, you know IME the best, do you have some ideas?

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If the call of focus() occurs during dispatching mousedown event or something synchronously, perhaps, we should set the reason to mouse, touch or key at setting focus.

It computes here:
https://dxr.mozilla.org/mozilla-central/rev/ade8d4a63e57560410de106450f37b50ed71cca5/dom/base/nsFocusManager.cpp#382

It can manage with AutoHandlingUserInputStatePusher if it's handling an input event.
https://dxr.mozilla.org/mozilla-central/rev/ade8d4a63e57560410de106450f37b50ed71cca5/dom/events/EventStateManager.h#1019

If you add a static method which returns current instance of AutoHandlingUserInputStatePusher, you can override the reason in nsFocusManager.  Although, this is pretty risk to uplift.

Otherwise, if it's called asynchronously, I have no idea.

Comment 22

[:Gijs (he/him)]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #21)
> If the call of focus() occurs during dispatching mousedown event or
> something synchronously, perhaps, we should set the reason to mouse, touch
> or key at setting focus.
> 
> It computes here:
> https://dxr.mozilla.org/mozilla-central/rev/
> ade8d4a63e57560410de106450f37b50ed71cca5/dom/base/nsFocusManager.cpp#382
> 
> It can manage with AutoHandlingUserInputStatePusher if it's handling an
> input event.
> https://dxr.mozilla.org/mozilla-central/rev/
> ade8d4a63e57560410de106450f37b50ed71cca5/dom/events/EventStateManager.h#1019
> 
> If you add a static method which returns current instance of
> AutoHandlingUserInputStatePusher, you can override the reason in
> nsFocusManager.  Although, this is pretty risk to uplift.
> 
> Otherwise, if it's called asynchronously, I have no idea.

I think maybe I was unclear. Instead of using NotifyIME, can we call MaybeShowOnScreenKeyboard based on receiving SetInputContext, where we *do* still know what action triggered the focus change? That seems simpler than trying to update 'reason' for synchronously called focus() from mousedown or similar events, and would also work if focus() is asynchronously called.

More generally, if you have cycles to investigate this further that would be really helpful - I am pretty busy at the moment.

We've already shipped this for a while, so I don't think it needs to be uplifted necessarily - we should just fix it.

Comment 23

[:Gijs (he/him)]

One other potential thing we could try: making https://dxr.mozilla.org/mozilla-central/source/widget/windows/WinIMEHandler.cpp#417 only reassign the last action if the mFocusChange of the action does not indicate that focus has moved.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to :Gijs Kruitbosch from comment #22)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #21)
> > If the call of focus() occurs during dispatching mousedown event or
> > something synchronously, perhaps, we should set the reason to mouse, touch
> > or key at setting focus.
> > 
> > It computes here:
> > https://dxr.mozilla.org/mozilla-central/rev/
> > ade8d4a63e57560410de106450f37b50ed71cca5/dom/base/nsFocusManager.cpp#382
> > 
> > It can manage with AutoHandlingUserInputStatePusher if it's handling an
> > input event.
> > https://dxr.mozilla.org/mozilla-central/rev/
> > ade8d4a63e57560410de106450f37b50ed71cca5/dom/events/EventStateManager.h#1019
> > 
> > If you add a static method which returns current instance of
> > AutoHandlingUserInputStatePusher, you can override the reason in
> > nsFocusManager.  Although, this is pretty risk to uplift.
> > 
> > Otherwise, if it's called asynchronously, I have no idea.
> 
> I think maybe I was unclear. Instead of using NotifyIME, can we call
> MaybeShowOnScreenKeyboard based on receiving SetInputContext, where we *do*
> still know what action triggered the focus change?

My idea is, the reason of SetInputContext() call should be updated if focus() is called during handling an input event. I assumed that sLastContextActionCause isn't a user action. Am I wrong?

> That seems simpler than
> trying to update 'reason' for synchronously called focus() from mousedown or
> similar events, and would also work if focus() is asynchronously called.

I think that it's impossible to open VKB at an async call of focus() because if we'd allow that, any focus change can cause opening VKB. However, it was hated when we started to develop Fennec.

> More generally, if you have cycles to investigate this further that would be
> really helpful - I am pretty busy at the moment.

I'm busy too. However, this is probably not so difficult. So, when I have time to do this in this Q4, I'll try to fix this.

> We've already shipped this for a while, so I don't think it needs to be
> uplifted necessarily - we should just fix it.

Okay, that sounds good.

Comment 25

[:Gijs (he/him)]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #24)
> (In reply to :Gijs Kruitbosch from comment #22)
> > (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #21)
> > > If the call of focus() occurs during dispatching mousedown event or
> > > something synchronously, perhaps, we should set the reason to mouse, touch
> > > or key at setting focus.
> > > 
> > > It computes here:
> > > https://dxr.mozilla.org/mozilla-central/rev/
> > > ade8d4a63e57560410de106450f37b50ed71cca5/dom/base/nsFocusManager.cpp#382
> > > 
> > > It can manage with AutoHandlingUserInputStatePusher if it's handling an
> > > input event.
> > > https://dxr.mozilla.org/mozilla-central/rev/
> > > ade8d4a63e57560410de106450f37b50ed71cca5/dom/events/EventStateManager.h#1019
> > > 
> > > If you add a static method which returns current instance of
> > > AutoHandlingUserInputStatePusher, you can override the reason in
> > > nsFocusManager.  Although, this is pretty risk to uplift.
> > > 
> > > Otherwise, if it's called asynchronously, I have no idea.
> > 
> > I think maybe I was unclear. Instead of using NotifyIME, can we call
> > MaybeShowOnScreenKeyboard based on receiving SetInputContext, where we *do*
> > still know what action triggered the focus change?
> 
> My idea is, the reason of SetInputContext() call should be updated if
> focus() is called during handling an input event. I assumed that
> sLastContextActionCause isn't a user action. Am I wrong?

I haven't verified this, but I suspect you're correct. My thinking is that if the user taps into an input field the first time and we receive the SetInputContext with a user action (touch/click), we call MaybeShowOnScreenKeyboard() directly, instead of setting something and then waiting for NotifyIME to hit (which might only happen after the .focus() call has also hit and changed sLastContextActionCause.

Comment 26

[Jim Mathies [:jimm]]

What's the proper behavior here? What I see:

* windows 10, surface 4, no keyboard
* latest nightly
* soft keyboard generally works in text edits in content

when I open this google doc, the cursor is sitting at the start of the paragraph and the soft keyboard does not display initially. In my opinion this is proper behavior, we shouldn't spam the user with the SKB on page load. But, tapping somewhere in the paragraph, I don't get the SKB either. In fact, I can't invoke the SKB anywhere in google docs. Do we have other bugs open this? 

I'm tempted to mark this wontfix based on the original report, or morph it into an "skb doesn't display when I tap a google doc" bug.

Comment 27

[Jim Mathies [:jimm]]

I'm seeing what Andrew is seeing in comment 16.

Comment 28

[Jim Mathies [:jimm]]

[Tracking Requested - why for this release]:

Comment 29

[:Gijs (he/him)]

(In reply to Jim Mathies [:jimm] from comment #26)
> What's the proper behavior here? What I see:
> 
> * windows 10, surface 4, no keyboard
> * latest nightly
> * soft keyboard generally works in text edits in content
> 
> when I open this google doc, the cursor is sitting at the start of the
> paragraph and the soft keyboard does not display initially. In my opinion
> this is proper behavior, we shouldn't spam the user with the SKB on page
> load.

Yes.

> But, tapping somewhere in the paragraph, I don't get the SKB either.
> In fact, I can't invoke the SKB anywhere in google docs. Do we have other
> bugs open this? 

No, that's this bug.

> I'm tempted to mark this wontfix based on the original report, or morph it
> into an "skb doesn't display when I tap a google doc" bug.

I've made the summary clearer. Can someone from platform work on this? I don't understand the interactions between our widget/IME/focus/event code well enough to be very productive here.

Comment 30

[Jim Mathies [:jimm]]

(In reply to :Gijs from comment #29)
> > But, tapping somewhere in the paragraph, I don't get the SKB either.
> > In fact, I can't invoke the SKB anywhere in google docs. Do we have other
> > bugs open this? 
> 
> No, that's this bug.
> 
> > I'm tempted to mark this wontfix based on the original report, or morph it
> > into an "skb doesn't display when I tap a google doc" bug.
> 
> I've made the summary clearer. Can someone from platform work on this? I
> don't understand the interactions between our widget/IME/focus/event code
> well enough to be very productive here.

Thanks. Yes we can pick this up, just need to find someone with some cycles.

Comment 31

[Julien Cristau [:jcristau] (back April 22)]

Mass wontfix for bugs affecting firefox 52.

Comment 32

[David Parks [:handyman]]

Attachment: Unblock on-screen keyboard when user input reinforces focus

(In reply to Andrew Overholt [:overholt] from comment #16)
> Clarifying this STR for future me:
> 
> 1. tablet mode, clean Firefox profile, on-screen keyboard not up
> 2. navigate to https://goo.gl/sPqJWb
> 3. does the on-screen keyboard come up? (answer below)
> 4. tap into the document
> 5. does the on-screen keyboard come up? (answer below)
> 6. tap the address bar
> 7. does the on-screen keyboard come up? (answer below)
> 
> Results on a Surface Book with the latest Windows 10:
> 
> 50 beta: 3. no, 5. no, 7. yes
> 52 nightly: 3. no, 5. no, 7. yes (same)

So the expected result here is no-yes-yes.  This issue is due to special code blocking the keyboard from being exposed... without [1], the behavior would be yes-yes-yes.  This is basically what :masayuki and :gijs had suspected.  I moved this conditional to later and remember if it blocked the keyboard.  I later detect that a user event reinforces the focus in the focus manager and expose the OSK there.  This gets us to no-yes-yes.

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=057bf2e9bb62385ffa3cd25c13d43d749fa050cd

[1] https://dxr.mozilla.org/mozilla-central/rev/45692c884fdd5136a64fb2f8a61a0c8183b69331/widget/windows/WinIMEHandler.cpp#689

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, Android widget does this:

> void
> GeckoEditableSupport::SetInputContext(const InputContext& aContext,
>                                       const InputContextAction& aAction)
> {
>     MOZ_ASSERT(mEditable);
> 
>     ALOGIME("IME: SetInputContext: s=0x%X, 0x%X, action=0x%X, 0x%X",
>             aContext.mIMEState.mEnabled, aContext.mIMEState.mOpen,
>             aAction.mCause, aAction.mFocusChange);
> 
>     // Ensure that opening the virtual keyboard is allowed for this specific
>     // InputContext depending on the content.ime.strict.policy pref
>     if (aContext.mIMEState.mEnabled != IMEState::DISABLED &&
>         aContext.mIMEState.mEnabled != IMEState::PLUGIN &&
>         Preferences::GetBool("content.ime.strict_policy", false) &&
>         !aAction.ContentGotFocusByTrustedCause() &&
>         !aAction.UserMightRequestOpenVKB()) {
>         return;
>     }

So, isn't it possible to use same code for aAction?

I.e.,

cache InputContextAction to sLastContextAction instead of InputContextAction::Cause to sLastContextActionCause, then, check

if (!sLastContextAction.ContentGotFocusByTrustedCause() &&
    !sLastContextAction.UserMightRequestOpenVKB()) {
  return false;
}

in IMEHandler::NeedOnScreenKeyboard()?

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8856604 [details] [diff] [review]
Unblock on-screen keyboard when user input reinforces focus

r- until coming new patch or reply for the previous comment.

Comment 35

[David Parks [:handyman]]

Attachment: Unblock on-screen keyboard when user input reinforces focus

I think this has what you were looking for.

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ab7677d951f757297996cae41078dde798b1cf4

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8859806 [details] [diff] [review]
Unblock on-screen keyboard when user input reinforces focus

>diff --git a/dom/base/nsFocusManager.cpp b/dom/base/nsFocusManager.cpp
>--- a/dom/base/nsFocusManager.cpp
>+++ b/dom/base/nsFocusManager.cpp
>@@ -1198,18 +1198,29 @@ nsFocusManager::SetFocusInner(nsIContent
> 
>   // unless it was set above, retrieve the window for the element to focus
>   if (!newWindow)
>     newWindow = GetCurrentWindow(contentToFocus);
> 
>   // if the element is already focused, just return. Note that this happens
>   // after the frame check above so that we compare the element that will be
>   // focused rather than the frame it is in.
>-  if (!newWindow || (newWindow == mFocusedWindow && contentToFocus == mFocusedContent))
>+  if (!newWindow ||
>+      (newWindow == mFocusedWindow && contentToFocus == mFocusedContent)) {
>+    if ((aFlags & (FLAG_BYMOUSE | FLAG_BYTOUCH)) &&
>+        newWindow && newWindow->GetDocShell() &&
>+        newWindow->GetDocShell()->GetPresShell() &&
>+        newWindow->GetDocShell()->GetPresShell()->GetPresContext() &&
>+        newWindow->GetDocShell()->GetPresShell()->GetPresContext()->GetRootWidget()) {
>+      // If we had blocked the on-screen keyboard while we waited for the user to
>+      // actively select our window then... that's what just happened.
>+      newWindow->GetDocShell()->GetPresShell()->GetPresContext()->GetRootWidget()->UnblockOSK();
>+    }
>     return;
>+  }

Ah, sorry, my comment was unclear.

So, I meant, I assumed that Firefox for Android works as expected. If so, you don't need to touch nsFocusManager and nsIWidget. (I hope that this should be fixed only in IMEHandler if it's possible.)

>+  // If the last focus cause was not user-initiated (ie a result of code
>+  // setting focus to an element) then don't auto-show a keyboard. This
>+  // avoids cases where the keyboard would pop up "just" because e.g. a
>+  // web page chooses to focus a search field on the page, even when that
>+  // really isn't what the user is trying to do at that moment.
>+  // We would like to show the keyboard if the user selects the element
>+  // later, even though the element will not get another focus event.
>+  // Therefore, we remember the element that would have gotten focus
>+  // and expose the keyboard elsewhere if that window is later selected.
>+  if (!sLastContextAction.ContentGotFocusByTrustedCause() &&
>+      !sLastContextAction.UserMightRequestOpenVKB()) {
>+    sBlockedOSK = true;
>+    return;
>+  }

So, isn't it enough to fix this only with this change?

Comment 37

[David Parks [:handyman]]

(In reply to Masayuki Nakano [:masayuki] from comment #36)
> So, I meant, I assumed that Firefox for Android works as expected. If so,
> you don't need to touch nsFocusManager and nsIWidget. (I hope that this
> should be fixed only in IMEHandler if it's possible.)

I see what you are going for and I had a similar thought when I wrote the original.  The focus manager already has enough complexity and there might be a mechanism in place for this condition -- but I dont see it (yet).  I'm looking.

> >+  if (!sLastContextAction.ContentGotFocusByTrustedCause() &&
> >+      !sLastContextAction.UserMightRequestOpenVKB()) {
> >+    sBlockedOSK = true;
> >+    return;
> >+  }
> 
> So, isn't it enough to fix this only with this change?

Well, thats very similar to the code we have now.  We currently have 

>  if (!InputContextAction::IsUserAction(sLastContextActionCause)) {
>    return false;
>  }

which tells NeedsOnScreenKeyboard that we don't need it based on sLastContextActionCause.  Thats pretty close to what the new conditional does with sLastContextAction... additionally, it sets sBlockedOSK but that does nothing without the UnblockOSK call added to the focus manager (I created sBlockedOSK for this patch).

Now, Android actually does _not_ handle this case properly.  You can see this by setting it to "Request Desktop Site" and clicking on the link in the bug.  It will actually open the keyboard immediately (which is wrong) instead of never opening it (this bug).  Judging from the Android code you sited, I would have expected it to either work properly or not open at all.  I'm currently checking why that happens.  If there is already a mechanism for the delayed keyboard opening behavior we want then I should be able to find it and use it.  If not, I'll probably suggest we go with the patch as is and just file a bug that the Android widget doesn't behave correctly in this case either.

FYI, I think the expectation that is failing is that it will call MaybeShowOnScreenKeyboard again when the user presses the text field (after the first attempt was blocked)... but that does not happen.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you for your explanation. I'm surprised at not working on Android too.

And I checked log of IMEStateManager. Then, when it takes focus at restoring the tab, focus is once lost as similar to deactivating the window. Then, getting focus again (this causes mFocusChange == FOCUS_NOT_CHANGED and mCause == CAUSE_UNKNOWN). When I tap in the document, focus once lost with open/close menu (but I'm not sure how web page causes this) and then, the content gets focus again (this causes mFocusChange == FOCUS_NOT_CHANGED and mCause == CAUSE_UNKNOWN).

If the focus moving are fired synchronously, we need to improve the setter of mCause. Then, UserMightRequestOpenVKB() will work as expected (comment 24). I think that EventStateManager::StartHandlingUserInput() should take current event message and store it until StopHandlingUserInput() is called. Then, nsFocusManager::GetFocusMoveActionCause() can return actual cause even if aFlags doesn't have user input information.

Otherwise, if the web apps set focus with setTimer() from an event listener, we need to more complicated hack. So, if GoogleDocs isn't using this way, I think that we should not support this case.

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8859806 [details] [diff] [review]
Unblock on-screen keyboard when user input reinforces focus

>@@ -1198,18 +1198,29 @@ nsFocusManager::SetFocusInner(nsIContent
> 
>   // unless it was set above, retrieve the window for the element to focus
>   if (!newWindow)
>     newWindow = GetCurrentWindow(contentToFocus);
> 
>   // if the element is already focused, just return. Note that this happens
>   // after the frame check above so that we compare the element that will be
>   // focused rather than the frame it is in.
>-  if (!newWindow || (newWindow == mFocusedWindow && contentToFocus == mFocusedContent))
>+  if (!newWindow ||
>+      (newWindow == mFocusedWindow && contentToFocus == mFocusedContent)) {
>+    if ((aFlags & (FLAG_BYMOUSE | FLAG_BYTOUCH)) &&
>+        newWindow && newWindow->GetDocShell() &&
>+        newWindow->GetDocShell()->GetPresShell() &&
>+        newWindow->GetDocShell()->GetPresShell()->GetPresContext() &&
>+        newWindow->GetDocShell()->GetPresShell()->GetPresContext()->GetRootWidget()) {
>+      // If we had blocked the on-screen keyboard while we waited for the user to
>+      // actively select our window then... that's what just happened.
>+      newWindow->GetDocShell()->GetPresShell()->GetPresContext()->GetRootWidget()->UnblockOSK();
>+    }
>     return;
>+  }

Ah, wait.

When we hit |!newWindow || (newWindow == mFocusedWindow && contentToFocus == mFocusedContent| but aFlags has FLAG_BYMOSUE or FLAG_BYTOUCH, shouldn't we just call IMEStateManager::OnChangeFocus()? Then, it should cause calling additional SetInputContext() with proper cause. Then, ContentGotFocusByTrustedCause() or UserMightRequestOpenVKB() should return true.

Comment 40

[David Parks [:handyman]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #39)

> When we hit |!newWindow || (newWindow == mFocusedWindow && contentToFocus ==
> mFocusedContent| but aFlags has FLAG_BYMOSUE or FLAG_BYTOUCH, shouldn't we
> just call IMEStateManager::OnChangeFocus()? Then, it should cause calling
> additional SetInputContext() with proper cause. Then,
> ContentGotFocusByTrustedCause() or UserMightRequestOpenVKB() should return
> true.

I get what you are going for here.  I'm having a hard time comprehending all of the effects of calling SetInputContext so I can't say what it might have broken but I am able to get this to work as you suggest... if I remove this conditional [1] whose comment says:

> // When focus is NOT changed actually, we shouldn't set IME state since
> // that means that the window is being activated and the child process
> // may have composition.  Then, we shouldn't commit the composition with
> // making IME state disabled.

I can imagine a problem if a content process has focus when a deactivated window gets reactivated... which I believe is what this is saying.  Is there a decent way to change the conditional so it works for both cases?

---

On the other hand, I'm not certain that either of us have the right suggestion for how to handle this.  I looked into how we handle the w3schools autofocus attribute example [2] and it properly does what we want here --- but it does it differently.  The difference in behavior seems to stem from the w3schools demo calling EditorEventListener::HandleEvent (which eventually calls IMEStateManager::OnClickInEditor which takes care of the keyboard).  The Google Docs case never gets here.  Here's a fragment of the call stack in that case:

>       xul.dll!mozilla::IMEStateManager::SetIMEState
>	xul.dll!mozilla::IMEStateManager::OnClickInEditor
>	xul.dll!mozilla::EditorEventListener::MouseClick
>	xul.dll!mozilla::EditorEventListener::HandleEvent
>	xul.dll!mozilla::EventListenerManager::HandleEventSubType
>	xul.dll!mozilla::EventListenerManager::HandleEventInternal
>	xul.dll!mozilla::EventListenerManager::HandleEvent
>	xul.dll!mozilla::EventTargetChainItem::HandleEvent
>	xul.dll!mozilla::EventTargetChainItem::HandleEventTargetChain
>	xul.dll!mozilla::EventDispatcher::Dispatch
>	xul.dll!mozilla::PresShell::DispatchEventToDOM
>	xul.dll!mozilla::PresShell::HandleEventInternal
>	xul.dll!mozilla::PresShell::HandleEventWithTarget
>	xul.dll!mozilla::EventStateManager::InitAndDispatchClickEvent
>	xul.dll!mozilla::EventStateManager::CheckForAndDispatchClick
>	xul.dll!mozilla::EventStateManager::PostHandleEvent
>	xul.dll!mozilla::PresShell::HandleEventInternal
>	xul.dll!mozilla::PresShell::HandlePositionedEvent
>	xul.dll!mozilla::PresShell::HandleEvent
>	xul.dll!mozilla::PresShell::HandleEvent
>	xul.dll!nsViewManager::DispatchEvent
>	xul.dll!nsView::HandleEvent
>	xul.dll!mozilla::widget::PuppetWidget::DispatchEvent
>	xul.dll!mozilla::layers::APZCCallbackHelper::DispatchWidgetEvent
>	xul.dll!mozilla::layers::APZCCallbackHelper::DispatchSynthesizedMouseEvent
>	xul.dll!mozilla::layers::APZCCallbackHelper::FireSingleTapEvent
>	xul.dll!mozilla::layers::APZEventState::ProcessSingleTap
>	xul.dll!mozilla::dom::TabChild::RecvHandleTap
>       [...]

That's as far as I've gotten with that but do you think the right answer is calling OnChangeFocus from nsFocusManager::SetFocusInner, or should I focus on why the EditorEventListener isn't called in the Google Docs case?

---

[1] https://dxr.mozilla.org/mozilla-central/rev/81977c96c6ff49e4b70f88a55f38d47f5e54a08b/dom/events/IMEStateManager.cpp#511-522
[2] https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_input_autofocus

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to David Parks (dparks) [:handyman] from comment #40)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from
> comment #39)
> 
> > When we hit |!newWindow || (newWindow == mFocusedWindow && contentToFocus ==
> > mFocusedContent| but aFlags has FLAG_BYMOSUE or FLAG_BYTOUCH, shouldn't we
> > just call IMEStateManager::OnChangeFocus()? Then, it should cause calling
> > additional SetInputContext() with proper cause. Then,
> > ContentGotFocusByTrustedCause() or UserMightRequestOpenVKB() should return
> > true.
> 
> I get what you are going for here.  I'm having a hard time comprehending all
> of the effects of calling SetInputContext so I can't say what it might have
> broken but I am able to get this to work as you suggest... if I remove this
> conditional [1] whose comment says:
> 
> > // When focus is NOT changed actually, we shouldn't set IME state since
> > // that means that the window is being activated and the child process
> > // may have composition.  Then, we shouldn't commit the composition with
> > // making IME state disabled.
> 
> I can imagine a problem if a content process has focus when a deactivated
> window gets reactivated... which I believe is what this is saying.  Is there
> a decent way to change the conditional so it works for both cases?

Ah, on Windows, IME composition shouldn't be committed forcibly when window is inactivated. I.e., composition is kept in inactivated window and can be continued to be modified after the window gets activated.  When a window becomes active, parent process gets focus first, then, child process gets focus.  When parent process gets focus, it disables IME temporarily. However, it causes committing composition forcibly.  Therefore, the block cancels setting input context for protecting the existing composition.

I think that only when sPresContext is nullptr or there is no active composition, it should not set input context in this case. Active composition can be retrieved with IMEStateManager::GetTextCompositionFor().

> On the other hand, I'm not certain that either of us have the right
> suggestion for how to handle this.  I looked into how we handle the
> w3schools autofocus attribute example [2] and it properly does what we want
> here --- but it does it differently.  The difference in behavior seems to
> stem from the w3schools demo calling EditorEventListener::HandleEvent (which
> eventually calls IMEStateManager::OnClickInEditor which takes care of the
> keyboard).  The Google Docs case never gets here.  Here's a fragment of the
> call stack in that case:
> 
> >       xul.dll!mozilla::IMEStateManager::SetIMEState
> >	xul.dll!mozilla::IMEStateManager::OnClickInEditor
> >	xul.dll!mozilla::EditorEventListener::MouseClick
> >	xul.dll!mozilla::EditorEventListener::HandleEvent
> >	xul.dll!mozilla::EventListenerManager::HandleEventSubType
> >	xul.dll!mozilla::EventListenerManager::HandleEventInternal
> >	xul.dll!mozilla::EventListenerManager::HandleEvent
> >	xul.dll!mozilla::EventTargetChainItem::HandleEvent
> >	xul.dll!mozilla::EventTargetChainItem::HandleEventTargetChain
> >	xul.dll!mozilla::EventDispatcher::Dispatch
> >	xul.dll!mozilla::PresShell::DispatchEventToDOM
> >	xul.dll!mozilla::PresShell::HandleEventInternal
> >	xul.dll!mozilla::PresShell::HandleEventWithTarget
> >	xul.dll!mozilla::EventStateManager::InitAndDispatchClickEvent
> >	xul.dll!mozilla::EventStateManager::CheckForAndDispatchClick
> >	xul.dll!mozilla::EventStateManager::PostHandleEvent
> >	xul.dll!mozilla::PresShell::HandleEventInternal
> >	xul.dll!mozilla::PresShell::HandlePositionedEvent
> >	xul.dll!mozilla::PresShell::HandleEvent
> >	xul.dll!mozilla::PresShell::HandleEvent
> >	xul.dll!nsViewManager::DispatchEvent
> >	xul.dll!nsView::HandleEvent
> >	xul.dll!mozilla::widget::PuppetWidget::DispatchEvent
> >	xul.dll!mozilla::layers::APZCCallbackHelper::DispatchWidgetEvent
> >	xul.dll!mozilla::layers::APZCCallbackHelper::DispatchSynthesizedMouseEvent
> >	xul.dll!mozilla::layers::APZCCallbackHelper::FireSingleTapEvent
> >	xul.dll!mozilla::layers::APZEventState::ProcessSingleTap
> >	xul.dll!mozilla::dom::TabChild::RecvHandleTap
> >       [...]
> 
> That's as far as I've gotten with that but do you think the right answer is
> calling OnChangeFocus from nsFocusManager::SetFocusInner, or should I focus
> on why the EditorEventListener isn't called in the Google Docs case?

Yeah, if we know the reason why IMEStateManager::OnClickInEditor() won't be called on Google Docs, it's the best.

Comment 42

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information