Closed
Bug 669995
Opened 13 years ago
Closed 13 years ago
Virtual keyboard does not automatically appear on etherpad
Categories
(Firefox for Android Graveyard :: General, defect, P3)
Tracking
(fennec+)
RESOLVED
WORKSFORME
Tracking | Status | |
---|---|---|
fennec | + | --- |
People
(Reporter: jeff, Assigned: snorp)
References
Details
(Whiteboard: [VKB][QA+])
Attachments
(3 files, 4 obsolete files)
1.30 KB,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
1.41 KB,
patch
|
masayuki
:
review-
|
Details | Diff | Splinter Review |
5.84 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
Consistently broken on any etherpad in http://etherpad.mozilla.com:9000
Android device: Samsung Galaxy 10.2
Android version: 3.1
Reporter | ||
Comment 1•13 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.
Updated•13 years ago
|
Priority: -- → P3
Comment 2•13 years ago
|
||
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.
Updated•13 years ago
|
Keywords: regressionwindow-wanted
Comment 3•13 years ago
|
||
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
Comment 4•13 years ago
|
||
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
Comment 5•13 years ago
|
||
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 )
Comment 7•13 years ago
|
||
(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.
Assignee | ||
Comment 9•13 years ago
|
||
Assignee | ||
Comment 10•13 years ago
|
||
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.
Assignee | ||
Updated•13 years ago
|
Attachment #554259 -
Flags: review?(alexp)
Comment 11•13 years ago
|
||
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+
Assignee | ||
Comment 12•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #554259 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #554280 -
Flags: review+
Assignee | ||
Comment 13•13 years ago
|
||
New patch that just removes the pref, since it does in fact default to false.
Keywords: checkin-needed
Comment 14•13 years ago
|
||
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 15•13 years ago
|
||
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-
Comment 16•13 years ago
|
||
See also: bug 532738
Comment 17•13 years ago
|
||
(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.
Comment 18•13 years ago
|
||
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
Comment 19•13 years ago
|
||
(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.
Assignee | ||
Comment 20•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
(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
Updated•13 years ago
|
Whiteboard: [VKB]
Comment 23•13 years ago
|
||
(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.
Comment 24•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
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
Assignee | ||
Comment 28•13 years ago
|
||
I'll try to fix it the popup way now and see where that gets us.
Assignee | ||
Comment 30•13 years ago
|
||
Allows setting focused window with appropriate flags (FOCUS_BYMOUSE, etc)
Assignee | ||
Comment 31•13 years ago
|
||
Assignee | ||
Comment 32•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #554280 -
Attachment is obsolete: true
Assignee | ||
Comment 33•13 years ago
|
||
This new set of patches just fixes the underlying bug.
Assignee | ||
Updated•13 years ago
|
Attachment #556088 -
Flags: review?(alexp)
Assignee | ||
Updated•13 years ago
|
Attachment #556089 -
Flags: review?(alexp)
Assignee | ||
Updated•13 years ago
|
Attachment #556090 -
Flags: review?(alexp)
Updated•13 years ago
|
Attachment #556088 -
Flags: review?(alexp) → review?(enndeakin)
Updated•13 years ago
|
Attachment #556089 -
Flags: review?(alexp) → review?(enndeakin)
Comment 34•13 years ago
|
||
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•13 years ago
|
||
Can you summarize what the issue is and how this patch fixes it?
Assignee | ||
Comment 36•13 years ago
|
||
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•13 years ago
|
||
(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.
Assignee | ||
Updated•13 years ago
|
Attachment #556090 -
Flags: review+ → review?(masayuki)
Comment 38•13 years ago
|
||
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-
Assignee | ||
Comment 39•13 years ago
|
||
(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•13 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•13 years ago
|
Attachment #556089 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 41•13 years ago
|
||
Allows setting focused window with appropriate flags (FOCUS_BYMOUSE, etc)
Assignee | ||
Comment 42•13 years ago
|
||
Allows setting focused window with appropriate flags (FOCUS_BYMOUSE, etc)
Assignee | ||
Updated•13 years ago
|
Attachment #557223 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #556088 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #557226 -
Flags: review+
Assignee | ||
Comment 43•13 years ago
|
||
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•13 years ago
|
||
(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.
Comment 45•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
I think we should consider backing out 532738 (on beta, aurora, and trunk) until this regression can be fixed.
tracking-fennec: --- → ?
Comment 51•13 years ago
|
||
(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•13 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 53•13 years ago
|
||
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?
Comment 54•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
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•13 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.
Updated•13 years ago
|
tracking-fennec: ? → +
Comment 58•13 years ago
|
||
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
Updated•13 years ago
|
Flags: in-litmus?(fennec)
Whiteboard: [VKB] → [VKB][QA+]
Comment 59•13 years ago
|
||
Testcase added: https://litmus.mozilla.org/show_test.cgi?id=33519
Flags: in-litmus?(fennec) → in-litmus+
Comment 60•13 years ago
|
||
Works with native UI.
Comment 61•13 years ago
|
||
Agreed with snorp - we can close this now.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•