Closed Bug 670694 Opened 10 years ago Closed 10 years ago

virtual keyboard is not displayed when editing input boxes

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(fennec+)

RESOLVED WORKSFORME
Tracking Status
fennec + ---

People

(Reporter: gal, Assigned: snorp)

References

Details

(Whiteboard: [VKB])

Attachments

(1 file, 2 obsolete files)

I get this intermittent but very frequently. Google voice is very often affected. Keyboard shows when going to the url bar but not for content edits. Some message lost from content to chrome process? Device is samsung galaxy tab 10".
tracking-fennec: --- → ?
A good way of reproducing is google voice. Chat with 2 people. Clicking on a conversation doesn't bring up the keyboard the first time (cursor gets position, input field enlarges, no keyboard). A click on a 2nd convo gets the keyboard up. Clicking on the first again and the keyboard still works.
Assignee: nobody → alexp
tracking-fennec: ? → 8+
tracking-fennec: 8+ → +
This has gone from bad to worse. I am completely unable to edit most input fields including voice.google.com. Keystrokes dont show up until after I leave the field. We are completely unusable on most sites requiring input. This needs urgent attention.
OS: Linux → Android
Hardware: Other → ARM
Whiteboard: [VKB]
I think I know what is going on here.  While fixing bug 669995, I fixed a problem where touching an already focused element would not bring up the keyboard.  This smells like the same thing.  I'll try to reproduce this bug with my patches applied.
It looks like my patch may have helped, but it seems there are still some problems.  Likely focus related (again, sigh). -> Me
Assignee: alexp → snorp
If you push the patch to nightlies I can give it a try, but there are probably 2 problems here. 1) keyboard not popping up 2) characters not showing (they are actually entered and displayed after unselected the box)
Ok, this is another problem caused by the strict focus rules.  I can reproduce this on google voice by doing the following:

1) go to voice.google.com, login
2) click 'text' link in one of the transcribed messages
3) a textarea appears, focused, w/o vkb
4) clicking the textarea again does not make the vkb appear

There is a check in nsFocusManager::SetFocusInner that causes us to bail out when the already-focused element is the same as the one we are focusing.  It might be enough to also compare the flags that were used when that element was focused, but I'd like to get some opinions on this.
This avoids a problem in Fennec where a script (or something) focuses an
element and then the user clicks the same element.  We only show the
virtual keyboard for user-requested focus changes, so it's important to
take this information into account when trying to deup a focus change.
Attachment #556609 - Attachment is obsolete: true
This avoids a problem in Fennec where a script (or something) focuses an
element and then the user clicks the same element.  We only show the
virtual keyboard for user-requested focus changes, so it's important to
take this information into account when trying to deup a focus change.
Attachment #556613 - Flags: review?(enndeakin)
Comment on attachment 556613 [details] [diff] [review]
Bug 670694 - store and compare focus flags when focused element changes

Review of attachment 556613 [details] [diff] [review]:
-----------------------------------------------------------------

This causes the focus event to fire multiple times, once when element.focus() is called and then again when the element is clicked.

I'm guessing you instead just want to update the ime state when this situation occurs.
Attachment #556613 - Flags: review?(enndeakin) → review-
Attachment #556613 - Attachment is obsolete: true
Attachment #557242 - Flags: review?(enndeakin)
New patch just updates the IME state when necessary.
Comment on attachment 557242 [details] [diff] [review]
Bug 670694 - when refocusing same element, update IME status if necessary

>   // 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 (newWindow && mFocusedContentFlags == 0 && aFlags != mFocusedContentFlags) {

Just use beforehand:

if (!newWindow)
  return;

so that newWindow doesn't need to be checked multiple times.


>+      PRUint32 reason = GetFocusMoveReason(aFlags);
>+      nsIMEStateManager::OnChangeFocus(presContext, mFocusedContent, reason);
>+    }

Masayuki should comment on whether this is the right thing to do here.
Attachment #557242 - Flags: review?(enndeakin) → review+
Attachment #557242 - Flags: review?(masayuki)
Isn't this a dup of bug 507987? If so, you should mark it as dup of this.

Basically, I don't like to use nsIWidget::SetInputMode() for changing the software keyboard state. But it's okay for now.

However, why does this work? nsIMEStateManager::OnChangeFocus() should do nothing at that time. See:
http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsIMEStateManager.cpp#175
If focus was set already, the IME state of the widget should be same as new state.

And does this patch work after third click or later click? I mean if you close software keyboard manually, does every click event cause opening the software keyboard?

And also, why don't you handle the click event in nsEditor? This patch makes all refocusing events call nsIWidget::SetInputMode(). I guess it wastes performance and battery. I think nsEditorEventListener::MouseClick() is the best method for calling nsIMEStateManager::OnChangeFocus() or nsIMEStateManager::UpdateIMEState() isn't it?

See my patch in bug 507987.
https://bugzilla.mozilla.org/attachment.cgi?id=436448&action=diff#a/editor/libeditor/base/nsEditorEventListener.cpp_sec2

Note that all focus check methods which are implementing in the patch have been implemented already.
(In reply to Masayuki Nakano (Mozilla Japan) from comment #13)
> Isn't this a dup of bug 507987? If so, you should mark it as dup of this.

Looks to be the same, though this is on Android.

> 
> Basically, I don't like to use nsIWidget::SetInputMode() for changing the
> software keyboard state. But it's okay for now.

Isn't that precisely what it's for?  At least, I don't see that it really does anything else...

> 
> However, why does this work? nsIMEStateManager::OnChangeFocus() should do
> nothing at that time. See:
> http://mxr.mozilla.org/mozilla-central/source/content/events/src/
> nsIMEStateManager.cpp#175
> If focus was set already, the IME state of the widget should be same as new
> state.

This fix actually depends on my 3rd patch from bug 669995, which does not save the IME state when we bail out in SetInputMode.  I've marked this one as depending on that now.  All of these IME bugs I'm working on are so tangled with each other...

> 
> And does this patch work after third click or later click? I mean if you
> close software keyboard manually, does every click event cause opening the
> software keyboard?

It does reopen the keyboard if you manually close it and click the input element again.

> 
> And also, why don't you handle the click event in nsEditor? This patch makes
> all refocusing events call nsIWidget::SetInputMode(). I guess it wastes
> performance and battery. I think nsEditorEventListener::MouseClick() is the
> best method for calling nsIMEStateManager::OnChangeFocus() or
> nsIMEStateManager::UpdateIMEState() isn't it?

Not all refocusing events will call SetInputMode.  Only ones that either change the focused element or actually change the input state.

I don't understand why you insist on trying to do all of this in the event handling layer.  We have all of this machinery available in the focus/IME managers, and it's a fairly trivial change.  Also, there are other ways to focus things besides clicks (keyboard, maybe some a11y tool, who knows what else?).
Depends on: 669995
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #14)
> > Basically, I don't like to use nsIWidget::SetInputMode() for changing the
> > software keyboard state. But it's okay for now.
> 
> Isn't that precisely what it's for?  At least, I don't see that it really
> does anything else...

I think that we should have two (at least) separated APIs in nsIWidget for managing inputting mode and inputting state.  The former is SetInputMode(). The state isn't changed without focus move and user cannot change it. The latter should open/close IME or software keyboard or change inputting language. These states can be changed by user.

> > However, why does this work? nsIMEStateManager::OnChangeFocus() should do
> > nothing at that time. See:
> > http://mxr.mozilla.org/mozilla-central/source/content/events/src/
> > nsIMEStateManager.cpp#175
> > If focus was set already, the IME state of the widget should be same as new
> > state.
> 
> This fix actually depends on my 3rd patch from bug 669995, which does not
> save the IME state when we bail out in SetInputMode.  I've marked this one
> as depending on that now.  All of these IME bugs I'm working on are so
> tangled with each other...

As I said, we must save the input mode when SetInputMode() is called. It shouldn't refuse the IME enabled state. If you need to save the software keyboard state, you should save it in new bool variable.

> > And does this patch work after third click or later click? I mean if you
> > close software keyboard manually, does every click event cause opening the
> > software keyboard?
> 
> It does reopen the keyboard if you manually close it and click the input
> element again.

Why? The widget's IME state should be "enabled" or "password" even if software keyboard is closed manually. Therefore, nsIMEStateManager shouldn't call nsIWidget::SetInputMode() again with same enabled state because it thinks it doesn't need to call.

> > And also, why don't you handle the click event in nsEditor? This patch makes
> > all refocusing events call nsIWidget::SetInputMode(). I guess it wastes
> > performance and battery. I think nsEditorEventListener::MouseClick() is the
> > best method for calling nsIMEStateManager::OnChangeFocus() or
> > nsIMEStateManager::UpdateIMEState() isn't it?
> 
> Not all refocusing events will call SetInputMode.  Only ones that either
> change the focused element or actually change the input state.
> 
> I don't understand why you insist on trying to do all of this in the event
> handling layer.  We have all of this machinery available in the focus/IME
> managers, and it's a fairly trivial change.  Also, there are other ways to
> focus things besides clicks (keyboard, maybe some a11y tool, who knows what
> else?).

I worry about the "what else" cases. That means you will change the behavior in unknown cases too. I think that implementing for each necessary situations is safer and makes simpler code.
Blocks: 686844
It seems to be working with native UI.
Should we just close this bug?
Agreed with snorp - we can close this now.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.