669995 - Virtual keyboard does not automatically appear on etherpad

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Jeff Vier [:jvier]]

Consistently broken on any etherpad in http://etherpad.mozilla.com:9000

Android device: Samsung Galaxy 10.2
Android version: 3.1

Comment 1

[Jeff Vier [:jvier]]

In Aurora (6.0a2), I got it to show the cursor and then when typing *not* show the characters, but I *could* see the typing on my desktop Firefox window with that etherpad open.

Note, also, that while the cursor shows, it's in the wrong place.  However, the typing happens where I tapped it.

Comment 2

[Tony Chung [:tchung]]

Confirmed on trunk builds, NExus S, Android 2.3.4.   

Mozilla/5.0 (Android; Linux armv7l; rv:8.0a1) Gecko/20110711 Firefox/8.0a1 Fennec/8.0a1

unable to even get the virtual keyboard to appear in the content region.

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

There is no regression range for this bug. The behavior is essentially the same across Firefox 5 through 8. 7 & 8 allow you to position the input cursor on long tap but this is a slight improvement in the functionality of the page.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

So I can tap inside the rich test editor on etherpad and get a cursor to appear. From there I can long press the menu key and start typing. However, the cursor positioning is awful at best.

So I see two distinct bugs here. The first is that the keyboard does not automatically pop up wen you click in the rich text editor. The second is that the cursor keeps jumping around as you type. Let's morph this bug into the first issue, and I'll file another bug for the cursor positioning.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

filed bug 670776 for the cursor/caret jumping around

Comment 6

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

To note: this bug existed for a while (see Bug 639704 - Can't type on Etherpad with Fennec )

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #6)
> To note: this bug existed for a while (see Bug 639704 - Can't type on
> Etherpad with Fennec )

no, that bug was that you *can't* type in etherpad's rich text editor with a cross process IME, which was fixed. You can most certainly type now.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Mine.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 669995 - always show virtual keyboard when input element is focused

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attached patch disabled the strict IME policy, which only showed the keyboard if a user action resulted in the element gaining focus.  This fixes the issue on etherpad and similar editors.

Comment 11

[Alex Pakhotin (:alexp)]

Comment on attachment 554259 [details] [diff] [review]
Bug 669995 - always show virtual keyboard when input element is focused

Fine with me, though we may just delete this preference, as we seem to default to false if it's not there.
I don't see why we might need it on mobile.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 669995 - always show virtual keyboard when input element is focused

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

New patch that just removes the pref, since it does in fact default to false.

Comment 14

[Ed Morley [:emorley]]

Sent to try:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=9a567d609584

Assuming all green, will push to inbound after.

I'm happy to fix it this time, but the attached patch header wasn't in an ideal format (seemed like the export to email format perhaps?), which caused the commit message to get messed up when qimporting. This page has the ideal hgrc settings, if that helps:
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Thanks :-)

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 554280 [details] [diff] [review]
Bug 669995 - always show virtual keyboard when input element is focused

Let's hold off before checking this patch in. The code being removed was added in bug 641836. Let's make sure we are not regressing it. Also, this seems similar to bug 604351, which we use a bit of a hack to get working.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

See also: bug 532738

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #10)
> Attached patch disabled the strict IME policy, which only showed the
> keyboard if a user action resulted in the element gaining focus.  This fixes
> the issue on etherpad and similar editors.

We actually want this behavior, don't we? We do not want scripts to auto-set focus, causing the keyboard to appear. In those cases we wanted the user to tap in the editor to get the keyboard.

Comment 18

[Matt Brubeck (:mbrubeck)]

Bug 680496 is a very similar problem caused by bug 532738.

In that case, tapping on the password field causes the a script to set focus to a different (hidden?) field, so content.ime.strict_policy causes us to suppress the keyboard.  It sounds like this case is similar: the focus is set by script, but in response to a user action.

If we want to keep the content.ime.strict_policy behavior, I think we should change it to let the keyboard appear if a script sets focus in response to user input like clicking (similar to the rules for pop-up blocking).  Until that's possible, I think we should disable content.ime.strict_policy.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Matt Brubeck (:mbrubeck) from comment #18)

> If we want to keep the content.ime.strict_policy behavior, I think we should
> change it to let the keyboard appear if a script sets focus in response to
> user input like clicking (similar to the rules for pop-up blocking).

I agree. Seems like a good plan. Do we need to talk to the event devs to see what we need to do this?

> Until that's possible, I think we should disable content.ime.strict_policy.

I disagree. Seems like a regression to me. The bad UX of popping up the VKB on pageload, and other times the user did not initiate, are not a good trade for fixing this bug.

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Matt Brubeck (:mbrubeck) from comment #18) 
> If we want to keep the content.ime.strict_policy behavior, I think we should
> change it to let the keyboard appear if a script sets focus in response to
> user input like clicking (similar to the rules for pop-up blocking).  Until
> that's possible, I think we should disable content.ime.strict_policy.

I think doing it in the style of pop-ups would still leave a window for broken behavior.  For instance, the script could set the focus in a timeout instead of doing it directly in the click handler, defeating our check (as I understand it).

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Matt Brubeck (:mbrubeck) from comment #18)
> Bug 680496 is a very similar problem caused by bug 532738.
> 
> In that case, tapping on the password field causes the a script to set focus
> to a different (hidden?) field, so content.ime.strict_policy causes us to
> suppress the keyboard.  It sounds like this case is similar: the focus is
> set by script, but in response to a user action.
I agree, although snorp makes a good point in comment 20

> 
> If we want to keep the content.ime.strict_policy behavior, I think we should
> change it to let the keyboard appear if a script sets focus in response to
> user input like clicking (similar to the rules for pop-up blocking).  Until
> that's possible, I think we should disable content.ime.strict_policy.

I agree. Though it should be noted that I didn't like ontent.ime.strict_policy being turned on in the first place

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Brad Lassey [:blassey](On vacation 8/22-9/20) from comment #21)
> (In reply to Matt Brubeck (:mbrubeck) from comment #18)
> > 
> > If we want to keep the content.ime.strict_policy behavior, I think we should
> > change it to let the keyboard appear if a script sets focus in response to
> > user input like clicking (similar to the rules for pop-up blocking).
err... meant to agree with this, not the explaination
>> Until
> > that's possible, I think we should disable content.ime.strict_policy.
> 
> I agree. Though it should be noted that I didn't like
> ontent.ime.strict_policy being turned on in the first place

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #20)
> (In reply to Matt Brubeck (:mbrubeck) from comment #18) 
> > If we want to keep the content.ime.strict_policy behavior, I think we should
> > change it to let the keyboard appear if a script sets focus in response to
> > user input like clicking (similar to the rules for pop-up blocking).  Until
> > that's possible, I think we should disable content.ime.strict_policy.
> 
> I think doing it in the style of pop-ups would still leave a window for
> broken behavior.  For instance, the script could set the focus in a timeout
> instead of doing it directly in the click handler, defeating our check (as I
> understand it).

Setting the focus in a timeout seems a broken behavior to me, and I don't think we want to let the keyboard in this case, otherwise the keyboard will possibly popup in the way while you're looking at something else because you have moved since the last touch.
I think the popup style thing is good enough.

Comment 24

[Matt Brubeck (:mbrubeck)]

(In reply to Vivien Nicolas (:vingtetun) from comment #23)
> Setting the focus in a timeout seems a broken behavior to me, and I don't
> think we want to let the keyboard in this case, otherwise the keyboard will
> possibly popup in the way while you're looking at something else because you
> have moved since the last touch.

There might be good reasons to set focus inside of a setTimeout(,0) or something, but unless we are seeing that in real-world sites I think we could let that case slide for now and focus on fixing the cases we know about.

Comment 25

[Martijn Wargers (dead)]

Perhaps a good middle ground would be that the first time on a particular page, the vkb can only pop up by user instantiated focus, after that, scripts can move focus all they want and the vkb will popup when focus gets in editable elements/text fields?

Comment 26

[Martijn Wargers (dead)]

Or perhaps, when the virtual keyboard is already open, don't close it, when focus is shifted to a different input.
It seems that the stock Android browser is doing something like that: http://people.mozilla.com/~mwargers/tests/focusmove.html

Comment 28

[James Willcox (:snorp) ([e-mail]) (he/him)]

I'll try to fix it the popup way now and see where that gets us.

Comment 30

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 669995 - Add nsIFocusManager::FocusWindow (part 1/3)

Allows setting focused window with appropriate flags (FOCUS_BYMOUSE, etc)

Comment 31

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 669995 - Use nsIFocusManager::FocusWindow to focus frames (part 2/3)

Comment 32

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 669995 - When ignoring a focus change, don't save the IME state (part 3/3)

Comment 33

[James Willcox (:snorp) ([e-mail]) (he/him)]

This new set of patches just fixes the underlying bug.

Comment 34

[Alex Pakhotin (:alexp)]

Comment on attachment 556090 [details] [diff] [review]
Bug 669995 - When ignoring a focus change, don't save the IME state (part 3/3)

Are these patches completely independent? I guess at least part 2 depends on the part 1. Probably it'd make sense to combine them into one patch.

Comment 35

[Neil Deakin]

Can you summarize what the issue is and how this patch fixes it?

Comment 36

[James Willcox (:snorp) ([e-mail]) (he/him)]

In Fennec, we enable the content.ime.strict_policy pref, which requires all focus changes to originate from a user action (see part 3 for context).  The way this is accomplished is by setting a flag when focusing an element.  Currently, when focusing an iframe, it is done by calling nsFocusManager::SetFocusedWindow, which does not take any of these flags.  By adding nsIFocusManager::FocusWindow (which takes focus flags) and using it appropriately, we solve the problem.  This is what the first two patches do.

The third patch solves an unrelated problem that contributes to this bug.  When ignoring the focus (due to the rules above) we were saving the new IME state even though that state was not being applied.  This causes a problem when the element actually *is* focused by a user gesture, because we think that the IME state is not changing (when it actually is).

Comment 37

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #36)
> The third patch solves an unrelated problem that contributes to this bug. 
> When ignoring the focus (due to the rules above) we were saving the new IME
> state even though that state was not being applied.  This causes a problem
> when the element actually *is* focused by a user gesture, because we think
> that the IME state is not changing (when it actually is).

For the third patch you really need the opinion of Masayuki, he has insist a lot for the IME state to be update even if the focus is not in the first implementation of the content.ime.strict_policy preference.

Comment 38

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

Comment on attachment 556090 [details] [diff] [review]
Bug 669995 - When ignoring a focus change, don't save the IME state (part 3/3)

This might break the behavior of caller of nsWindow::GetInputMode(). You must save it every time...

Comment 39

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Masayuki Nakano (Mozilla Japan)(offline: 8/31 and 9/1) from comment #38)
> Comment on attachment 556090 [details] [diff] [review]
> Bug 669995 - When ignoring a focus change, don't save the IME state (part
> 3/3)
> 
> This might break the behavior of caller of nsWindow::GetInputMode(). You
> must save it every time...

IMHO, the input mode should match what we are actually doing.  If we set it to enabled and aren't showing the keyboard, then that's wrong.

Setting the state and then concealing it like we are doing now leads to bugs exactly like this one.  I agree, though, that silently failing SetInputMode is not ideal.  Maybe we could return some error condition?  Or better, add a separate 'bool ShouldSetInputMode(int mode)' method?  I like that better.

Comment 40

[Neil Deakin]

Comment on attachment 556088 [details] [diff] [review]
Bug 669995 - Add nsIFocusManager::FocusWindow (part 1/3)

>+   * Focuses the given window
>+   *
>+   * @throws NS_ERROR_INVALID_ARG if aWindow is null
>+   */
>+  void focusWindow(in nsIDOMWindow aWindow, in unsigned long aFlags);

Add a note to the comment description that indicates that this is the same as setting focusedWindow but allows setting flags.

I didn't review the ime change.

Comment 41

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 669995 - Add nsIFocusManager::FocusWindow (part 1/3)

Allows setting focused window with appropriate flags (FOCUS_BYMOUSE, etc)

Comment 42

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 669995 - Add nsIFocusManager::FocusWindow (part 1/3)

Allows setting focused window with appropriate flags (FOCUS_BYMOUSE, etc)

Comment 43

[James Willcox (:snorp) ([e-mail]) (he/him)]

The IME change in part 1 was bleed through from another bug.  Fixed that and made the comment change requeste in comment 40.

Comment 44

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

(In reply to James Willcox (:snorp) ([e-mail]) from comment #39)
> (In reply to Masayuki Nakano (Mozilla Japan)(offline: 8/31 and 9/1) from
> comment #38)
> > Comment on attachment 556090 [details] [diff] [review]
> > Bug 669995 - When ignoring a focus change, don't save the IME state (part
> > 3/3)
> > 
> > This might break the behavior of caller of nsWindow::GetInputMode(). You
> > must save it every time...
> 
> IMHO, the input mode should match what we are actually doing.  If we set it
> to enabled and aren't showing the keyboard, then that's wrong.
> 
> Setting the state and then concealing it like we are doing now leads to bugs
> exactly like this one.  I agree, though, that silently failing SetInputMode
> is not ideal.  Maybe we could return some error condition?  Or better, add a
> separate 'bool ShouldSetInputMode(int mode)' method?  I like that better.

Probably, you misunderstand IME enabled state. IME enabled state doesn't mean whether software keyboard is open or closed. Software keyboard is just a virtual input device (the software keyboard is implemented by IME on Android though). It makes key events and IME processes the events and returns the result to software keyboard. The "disabled" state means that IME isn't available on the focused element. IME enabled state must not be changed without focus move. If you need to store the software keyboard state, you must make new bool variable and save the state to it.

Comment 45

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

SetInputMode() sets IME enabled state which cannot be changed by users.

On the other hand, IME open/closed state and software keyboard open/closed state can be changed by user.

I think that nsIWidget should have two methods for these different states, or combine all of them in the param of SetInputMode() and nsIMEStateManager() should call it always.

Comment 46

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

snorp:

I sent emails to you. I think that I should redesign and implement the new IME APIs and you use the new APIs via nsIMEStateManager. Then, our different thought about software keyboard will be fixed simply.

Comment 50

[Matt Brubeck (:mbrubeck)]

I think we should consider backing out 532738 (on beta, aurora, and trunk) until this regression can be fixed.

Comment 51

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Matt Brubeck (:mbrubeck) from comment #50)
> I think we should consider backing out 532738 (on beta, aurora, and trunk)
> until this regression can be fixed.

We need to make a decision fast. Beta is being locked down.

Comment 52

[Armen [:armenzg]]

If we said that this doesn't work on Google Docs would it help to increase its importance?

This is absolutely a regression since Firefox 6 works and Firefox 7 does not.

Comment 53

[Matt Brubeck (:mbrubeck)]

Comment on attachment 554259 [details] [diff] [review]
Bug 669995 - always show virtual keyboard when input element is focused

I think we should take this patch for now, and land it on Aurora and Beta if possible.  This just sets content.ime.strict_policy to false for mobile, effectively reverting the change from bug 532738.

If this fixes the problems we're seeing here, I think we should land this for now, and take it on Aurora and Beta if possible.

Comment 54

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Matt Brubeck (:mbrubeck) from comment #53)
> Comment on attachment 554259 [details] [diff] [review]
> Bug 669995 - always show virtual keyboard when input element is focused
> 
> I think we should take this patch for now, and land it on Aurora and Beta if
> possible.  This just sets content.ime.strict_policy to false for mobile,
> effectively reverting the change from bug 532738.
> 
> If this fixes the problems we're seeing here, I think we should land this
> for now, and take it on Aurora and Beta if possible.

Matt - let's land this ASAP and see what get's fixed and what falls out. Then we can better ask for aurora and beta.

Comment 55

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #54)

> Matt - let's land this ASAP and see what get's fixed and what falls out.
> Then we can better ask for aurora and beta.

I just saw we are waiting for some QA to verify the change works using about:config. Fine by me.

Comment 56

[Matt Brubeck (:mbrubeck)]

Comment on attachment 554259 [details] [diff] [review]
Bug 669995 - always show virtual keyboard when input element is focused

Hmm, because of changes in bug 641836, this will also allow the keyboard to display on page load in cases where it was previously prevented (even in Firefox 5 and earlier).  We might need to back out more of bug 641836 to prevent regressions.  :(

Comment 57

[Armen [:armenzg]]

(In reply to Armen Zambrano G. [:armenzg] - Release Engineer from comment #52)
> If we said that this doesn't work on Google Docs would it help to increase
> its importance?
> 
> This is absolutely a regression since Firefox 6 works and Firefox 7 does not.

I spent more time between Firefox 7 and Firefox 6. What I see on Google Docs' spreadsheets can be workaround by 1) pressing the field once more (it sometimes work) and 2) pressing on the other text fields besides the first one (which would pull the keyboard up).

The problem I see is in both FF6 and FF7 but it can be workaround.

On another note, on etherpad I can't click on URLs. Not sure if that is know or if anyone else can reproduce it.

Comment 58

[Matt Brubeck (:mbrubeck)]

Comment on attachment 554259 [details] [diff] [review]
Bug 669995 - always show virtual keyboard when input element is focused

I opened bug 688783 to do a proper backout of bug 641836 until this bug can be fixed.

Comment 59

[Andreea Pod]

Testcase added: https://litmus.mozilla.org/show_test.cgi?id=33519

Comment 60

[Alex Pakhotin (:alexp)]

Works with native UI.

Comment 61

[Alex Pakhotin (:alexp)]

Agreed with snorp - we can close this now.