Virtual keyboard does not automatically appear on etherpad

RESOLVED WORKSFORME

Status

P3
normal
RESOLVED WORKSFORME
8 years ago
7 years ago

People

(Reporter: jeff, Assigned: snorp)

Tracking

(Blocks: 1 bug)

Firefox 6
All
Android
Dependency tree / graph
Bug Flags:
in-litmus +

Details

(Whiteboard: [VKB][QA+])

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

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

Android device: Samsung Galaxy 10.2
Android version: 3.1
(Reporter)

Comment 1

8 years ago
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.
Priority: -- → P3
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.
Keywords: regressionwindow-wanted
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.
Keywords: regressionwindow-wanted
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.
Summary: Editing etherpad broken → Virtual keyboard does not automatically appear on etherpad
filed bug 670776 for the cursor/caret jumping around
To note: this bug existed for a while (see Bug 639704 - Can't type on Etherpad with Fennec )
(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.
Mine.
Assignee: nobody → snorp
Created attachment 554259 [details] [diff] [review]
Bug 669995 - always show virtual keyboard when input element is focused
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.
Attachment #554259 - Flags: review?(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.
Attachment #554259 - Flags: review?(alexp) → review+
Created attachment 554280 [details] [diff] [review]
Bug 669995 - always show virtual keyboard when input element is focused
Attachment #554259 - Attachment is obsolete: true
New patch that just removes the pref, since it does in fact default to false.
Keywords: checkin-needed
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 :-)
Status: NEW → ASSIGNED
Keywords: checkin-needed
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.
Attachment #554280 - Flags: review+ → review-
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
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.
Blocks: 532738
(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.
(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).
(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
(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
Whiteboard: [VKB]
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
(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.
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?
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
Duplicate of this bug: 680496
I'll try to fix it the popup way now and see where that gets us.
Duplicate of this bug: 681979
Created attachment 556088 [details] [diff] [review]
Bug 669995 - Add nsIFocusManager::FocusWindow (part 1/3)

Allows setting focused window with appropriate flags (FOCUS_BYMOUSE, etc)
Created attachment 556089 [details] [diff] [review]
Bug 669995 - Use nsIFocusManager::FocusWindow to focus frames (part 2/3)
Created attachment 556090 [details] [diff] [review]
Bug 669995 - When ignoring a focus change, don't save the IME state (part 3/3)
Attachment #554280 - Attachment is obsolete: true
This new set of patches just fixes the underlying bug.
Attachment #556088 - Flags: review?(alexp)
Attachment #556089 - Flags: review?(alexp)
Attachment #556090 - Flags: review?(alexp)
Attachment #556088 - Flags: review?(alexp) → review?(enndeakin)
Attachment #556089 - Flags: review?(alexp) → review?(enndeakin)
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.
Attachment #556090 - Flags: review?(alexp) → review+

Comment 35

7 years ago
Can you summarize what the issue is and how this patch fixes it?
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).
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Attachment #556090 - Flags: review+ → review?(masayuki)
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...
Attachment #556090 - Flags: review?(masayuki) → review-
(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

7 years ago
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.
Attachment #556088 - Flags: review?(enndeakin) → review+

Updated

7 years ago
Attachment #556089 - Flags: review?(enndeakin) → review+
Created attachment 557223 [details] [diff] [review]
Bug 669995 - Add nsIFocusManager::FocusWindow (part 1/3)

Allows setting focused window with appropriate flags (FOCUS_BYMOUSE, etc)
Created attachment 557226 [details] [diff] [review]
Bug 669995 - Add nsIFocusManager::FocusWindow (part 1/3)

Allows setting focused window with appropriate flags (FOCUS_BYMOUSE, etc)
Attachment #557223 - Attachment is obsolete: true
Attachment #556088 - Attachment is obsolete: true
The IME change in part 1 was bleed through from another bug.  Fixed that and made the comment change requeste in comment 40.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
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.
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.
Duplicate of this bug: 686359
Duplicate of this bug: 686764
Duplicate of this bug: 687101
I think we should consider backing out 532738 (on beta, aurora, and trunk) until this regression can be fixed.
tracking-fennec: --- → ?
(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

7 years ago
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 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.
Attachment #554259 - Attachment is obsolete: false
Attachment #554259 - Flags: approval-mozilla-beta?
Attachment #554259 - Flags: approval-mozilla-aurora?
(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.
(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 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.  :(
Attachment #554259 - Flags: approval-mozilla-beta?
Attachment #554259 - Flags: approval-mozilla-aurora?

Comment 57

7 years ago
(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.
Blocks: 686789
tracking-fennec: ? → +
Blocks: 688717
Blocks: 688783
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.
Attachment #554259 - Attachment is obsolete: true
Flags: in-litmus?(fennec)
Whiteboard: [VKB] → [VKB][QA+]
Blocks: 691027
No longer blocks: 691027

Comment 59

7 years ago
Testcase added: https://litmus.mozilla.org/show_test.cgi?id=33519
Flags: in-litmus?(fennec) → in-litmus+
Works with native UI.
Agreed with snorp - we can close this now.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.