Closed
Bug 507987
Opened 16 years ago
Closed 14 years ago
[Maemo] [N8x0] virtual keyboard not shown by selecting an input field after forcing it to hide
Categories
(Firefox for Android Graveyard :: General, defect, P5)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 11
People
(Reporter: jukey, Assigned: masayuki)
References
Details
(Whiteboard: [vkb])
Attachments
(1 file, 7 obsolete files)
26.98 KB,
patch
|
dougt
:
review-
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1 (.NET CLR 3.5.30729)
Build Identifier: fennec nightly build 20090802 on Maemo 4.3.1 (5-2008.43-7)
As described at the end of this comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=501119#c9
Reproducible: Always
Steps to Reproduce:
1. start fennec
2. go to google.com
3. select the input field (there is only one :))
4. "press" the virtual key in the bottom right corner to close the virtual keyboard
5. tip into the input field again
Actual Results:
nothing happens!
Expected Results:
the vbk should come up again
From Comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=501119#c9
[...]
our vkb works only based on IME changes, and in that case
when forcing the vkb to hide (by pressing the button) here is no focus change
internally in mozilla, so the input field yet holds the focus (see it is yet
yellow and cursor is still blinking). When you click again on the field it does
not show up since the field is not re-acquiring the focus.
note you can reproduce the problem described in steps 6 and 7 in google.com
too.
--> to solve that we show also consire showing up VKB based on click events,
not only focus
Updated•16 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•16 years ago
|
Flags: wanted-fennec1.0?
Comment 1•15 years ago
|
||
This bug is quite simple. it removes an early out in the focus manager which prevented the refocusing / re-requesting of the keyboard.
Attachment #427095 -
Flags: review?(mark.finkle)
Comment 2•15 years ago
|
||
(In reply to comment #1)
> Created an attachment (id=427095) [details]
> This patch enables refocusing of already focused elements
>
> This bug is quite simple. it removes an early out in the focus manager which
> prevented the refocusing / re-requesting of the keyboard.
jeremias, might it be good to add a maemo/fennec guard on it ? and then not alter other products behaviour ...
Comment 3•15 years ago
|
||
Yes, thats fine with me. Will go alter it.
Comment 4•15 years ago
|
||
The alternative patch tries to trigger only the IME state change on refocusing instead of running through all the focus handling code. (It might still be a good idea to add the Maemo guarding, although I don't understand yet, why it works on other platforms.)
Comment 5•15 years ago
|
||
I left in a getenv() check that was in there for debugging reasons.
Attachment #428669 -
Attachment is obsolete: true
Attachment #428737 -
Flags: review?(dougt)
Updated•15 years ago
|
Attachment #427095 -
Attachment is obsolete: true
Attachment #427095 -
Flags: review?(mark.finkle)
Updated•15 years ago
|
Attachment #428737 -
Flags: review?(dougt) → review?(masayuki)
Comment 6•15 years ago
|
||
masayuki, can you take a look at this patch?
Assignee | ||
Comment 7•15 years ago
|
||
Comment on attachment 428737 [details] [diff] [review]
Alternative patch with superfluous getenv() condition removed
I don't think this approach is right.
And also I cannot understand why this fixes the bug. Even if you call nsIMEStateManager::OnChangeFocus() with same arguments at previous calling, it will do nothing. See http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsIMEStateManager.cpp#131
I want to know what happens when you close the virtual keyboad and try to open it again. I'd like you to log the GTK2 widget behavior. I'm working on GTK2 widget's IME code separation now. The new code can long its behavior. See bug 520732. Apply the patch and set NSPR_LOG_MODULES to nsGtkIMModuleWidgets:1 and NSPR_LOG_FILE to path of the log file.
Assignee | ||
Comment 8•15 years ago
|
||
And please use "-p" when you create the diff file.
Comment 9•15 years ago
|
||
This patch fixes the bug also for you? But you do not understand why? When i think about it, there better should be a maemo define around it.
The special thing about the Maemo virtual keyboard is that you can minimize it but the focus have to stay in order to type with the hardware keyboard.
The only situation when a IME gets triggered from xulrunner is when the focus changes to a editable object. But as I said, the focus stays when you minimize the vkb!
What the patch does is, it asks to open a ime if you clicked into a already focused object. The question asked there is, is the keyboard opened, which is false when its hidden and so the IME gets opened again.
Assignee | ||
Comment 10•15 years ago
|
||
(In reply to comment #9)
> This patch fixes the bug also for you? But you do not understand why? When i
> think about it, there better should be a maemo define around it.
You shouldn't use #ifdefs in XP level code except that there are no ways.
> What the patch does is, it asks to open a ime if you clicked into a already
> focused object. The question asked there is, is the keyboard opened, which is
> false when its hidden and so the IME gets opened again.
My question is, why you to call nsIMEStateManager::OnChangeFocus() again can fix this bug? It shouldn't call the nsIWidget::SetIMEEnabled() again because there are no state changes.
I guess that you should separate the #ifdef MOZ_PLATFORM_MAEMO block of SetIMEEnabled() from it. And when the user click, you should call the new method from nsWindow if you need it.
Comment 11•15 years ago
|
||
> My question is, why you to call nsIMEStateManager::OnChangeFocus() again can
> fix this bug? It shouldn't call the nsIWidget::SetIMEEnabled() again because
> there are no state changes.
Mhm there should be a state change... http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsIMEStateManager.cpp#141 will return false when the keyboard was hidden before.
And thats why the keyboard gets opened again. Do you see any regression or issues caused by the change?
Assignee | ||
Comment 12•15 years ago
|
||
(In reply to comment #11)
> Mhm there should be a state change...
> http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsIMEStateManager.cpp#141
> will return false when the keyboard was hidden before.
If the state was changed to disabled, the virtual keyboard should be closed by the timing.
> And thats why the keyboard gets opened again. Do you see any regression or
> issues caused by the change?
There are some bugs but I fixed them in the patch of bug 520732. Would you check that?
I said again, I don't think you should change XP code, you should change only nsWindow of GTK2.
Comment 13•15 years ago
|
||
Well, since I do not work with GTK2 Platform at all (but for the Qt Platform) I cant/wont be able to test the stuff with it. Do you see any regression with the patch on your side? If not, than why not pushing it? Its really important for the qt version to have it in!
Assignee | ||
Comment 14•15 years ago
|
||
Comment on attachment 428737 [details] [diff] [review]
Alternative patch with superfluous getenv() condition removed
-'ing per my previous comments.
Attachment #428737 -
Flags: review?(masayuki) → review-
Assignee | ||
Comment 15•15 years ago
|
||
this is draft, but I guess this works.
Comment 17•15 years ago
|
||
This approach is a lot more complicated and may not so easy to realize in the qt port since focushandling is really different on qt level and the focused object on qt level must not be the same as on nsWindow level.
I can't understand why you prefer this above a 5 line solution which will work on qt and gtk and in more cases.
The only thing our patch does, it requests a ime if a already focused object was focused again but the ime was not visible. You are actually doing the same, just more complicated and restricted to the user left click. But you do not cover refocusing caused through scripts or other events (i.e. through touch gestures). Our patch cause no regression in the qt firefox build, so i think there is no need for any #ifdef needed.
Assignee | ||
Comment 18•15 years ago
|
||
Because the patch breaks DOM event model at least. The patch doesn't honor the event's status which causes the focus moving.
And I bet that the patch causes another bug. Seems that the software keyboard could be reopened by some other actions. E.g., focus() method could cause.
And I still think that the patch works accidentally. It could be broken by another future change.
I don't think that porting to qt is difficult. qt's nsWindow should override the nsIWidget::OnIMEMouseClick() and reset the software keyboard in it.
Assignee | ||
Comment 19•15 years ago
|
||
If this patch works, I'll request reviews.
Attachment #433614 -
Attachment is obsolete: true
Assignee | ||
Comment 20•15 years ago
|
||
Attachment #433697 -
Attachment is obsolete: true
Comment 21•15 years ago
|
||
(In reply to comment #18)
> And I bet that the patch causes another bug. Seems that the software keyboard
> could be reopened by some other actions. E.g., focus() method could cause.
Exactly it was actually meant to be so that other actions can cause it, like the call of focus method. That this does not work with this approach is what makes me think that its wrong.
> And I still think that the patch works accidentally. It could be broken by
> another future change.
Alright, I see this does not bring us somewhere as long as I think that this entire IMEMouseClick() approach is wrong because of its restriction to MouseClick and not reacting on any other refocusing.
We may need another Bugreport about that issue.
> I don't think that porting to qt is difficult. qt's nsWindow should override
> the nsIWidget::OnIMEMouseClick() and reset the software keyboard in it.
I will give it a try next week, that is for sure. I'm still curious why you need to check if the nsWindow you click into is the already focused one, in my opinion mouseclick shouldn't even get called when its not the already focused object.
Assignee | ||
Comment 22•15 years ago
|
||
Dougt, can you test my latest patch?
(In reply to comment #21)
> (In reply to comment #18)
> > And I bet that the patch causes another bug. Seems that the software keyboard
> > could be reopened by some other actions. E.g., focus() method could cause.
>
> Exactly it was actually meant to be so that other actions can cause it, like
> the call of focus method. That this does not work with this approach is what
> makes me think that its wrong.
I don't think that it's good behavior. I guess that it causes some a11y issue. E.g., some sites may set focus by timer. Then, the software keyboard is popupped unintentionally. It's another discussion/bug "what" behavior should popup the software keyboard automatically.
Comment 23•15 years ago
|
||
The patch didn't apply for me in some gtk stuff, didn't matter much. We managed to get it working in qt and will go with it.
Push for some Review. Qt part will follow soon.
Assignee | ||
Comment 24•15 years ago
|
||
updating for latest trunk, I posted this patch to tryserver.
Attachment #433698 -
Attachment is obsolete: true
Assignee | ||
Comment 25•15 years ago
|
||
Comment on attachment 436448 [details] [diff] [review]
Patch v2.1.2
The test build is here.
https://build.mozilla.org/tryserver-builds/masayuki@d-toybox.com-try-ed755040d54c/
First, I need dougt's review for gtk2 part.
Dougt, I cannot test this patch. So, can you test and review this?
Attachment #436448 -
Flags: review?(dougt)
Comment 26•15 years ago
|
||
The builds worked for me, but I am undersure why we use click events to bring up the software keyboard. It seems a bit unclean to do it this way? Masayuki, can you explain why this approach is best?
Assignee | ||
Comment 27•15 years ago
|
||
(In reply to comment #26)
> but I am undersure why we use click events to bring up the software keyboard.
I don't know whether the behavior is good or not. I only wrote a patch for this bug.
> It seems a bit unclean to do it this way? Masayuki,
> can you explain why this approach is best?
The request of this bug is "when the user taps the focused editor, we should reopen the software keyboard", so, we should do it only when click event is fired. However, there are complex issues.
1. The Steffen's patch should create unintentional behaviors. And the patch shouldn't work for this bug. I think there is another bug, and it causes the patch to work. So, the approach will be broken by easily and unintentionally. We should handle each event which is needed.
2. Click event is generated by nsEventStateManager by mouse button up event, so, widget don't know when click event is fired.
3. If the web page wants to handle the click event, it should be preferred rather than reopening the software keyboard. Besides, web pages should be able to prevent the default behavior.
The patch notifies the click event from editor to widget only when it is focused. And widget just handles the event. So, each platform can handle their preferred action.
Comment 28•15 years ago
|
||
We found a regression with this solution.
Taking fennec and clicking into the awesomebar does open the vkb but does not open to the awesomebar page anymore. The awesomebar page is only shown after typing something.
Reverting the changes in ./editor/... fix this issue.
Assignee | ||
Comment 29•15 years ago
|
||
(In reply to comment #28)
> Taking fennec and clicking into the awesomebar does open the vkb but does not
> open to the awesomebar page anymore. The awesomebar page is only shown after
> typing something.
I guess that Fennec's awesomebar listens click event in bubble phase but the editor event listener listens the click event and consumes it previously.
Comment 30•15 years ago
|
||
> I guess that Fennec's awesomebar listens click event in bubble phase but the
> editor event listener listens the click event and consumes it previously.
So well, we shouldn't consume it then, just reacting but nothing more?
Assignee | ||
Comment 31•15 years ago
|
||
(In reply to comment #30)
> > I guess that Fennec's awesomebar listens click event in bubble phase but the
> > editor event listener listens the click event and consumes it previously.
>
> So well, we shouldn't consume it then, just reacting but nothing more?
I'm not sure that it's good behavior that one click event opens both vkb and awesomebar's page. If we think so, awesome bar should listen the click event in capture phase. But I don't think the double reaction is good thing...
Assignee | ||
Comment 32•15 years ago
|
||
Ah, did you click when the awesome bar isn't focused? If so, the double reaction isn't strange, maybe. I think that the double reaction isn't good if the editor was already focused.
Comment 33•15 years ago
|
||
(In reply to comment #32)
> Ah, did you click when the awesome bar isn't focused? If so, the double
> reaction isn't strange, maybe. I think that the double reaction isn't good if
> the editor was already focused.
Yes it wasn't focused. Just start fennec, go to some page and click in the awesomebar in order to see the awesomebar page.
Expected Result:
Awesomebar page and VKB get open.
Actually Result:
VKB gets open on top of the already loaded website.
Assignee | ||
Comment 34•15 years ago
|
||
Unfortunately, I don't have an environment which can run fennec.
Assignee | ||
Comment 35•15 years ago
|
||
> Yes it wasn't focused.
So, I think that Fennec should handle focus event for opening the awesome bar page rather than click event.
Comment 36•15 years ago
|
||
(In reply to comment #35)
> > Yes it wasn't focused.
>
> So, I think that Fennec should handle focus event for opening the awesome bar
> page rather than click event.
mhm I think its happening here:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser-ui.js#711
On Mouseup the command gets "called" and cause that here
http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser-ui.js#554
the history gets open. Dunno if you can change that so easily. But I also thought that opening the history view is caused by the focus change and not by the mouse-up. But since I'm no expert in that area we should ask someone who is. Maybe Mark Finkle?
Comment 37•15 years ago
|
||
I started to wonder. Cant we just remove the focus from the textfield in case of a manual hide of the user? That would solve that issue too.
What do you think?
Assignee | ||
Comment 38•15 years ago
|
||
I don't think so, we shouldn't move focus automatically. It could confuse the users.
Comment 39•15 years ago
|
||
Comment on attachment 436448 [details] [diff] [review]
Patch v2.1.2
there seems to be a bunch of discussion of other approaches. I am r- for now.
Attachment #436448 -
Flags: review?(dougt) → review-
Comment 40•15 years ago
|
||
(In reply to comment #36)
> mhm I think its happening here:
>
> http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser-ui.js#711
>
> On Mouseup the command gets "called" and cause that here
>
> http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser-ui.js#554
>
> the history gets open. Dunno if you can change that so easily. But I also
> thought that opening the history view is caused by the focus change and not by
> the mouse-up. But since I'm no expert in that area we should ask someone who
> is. Maybe Mark Finkle?
We made the change to use "mouseup" instead of "click" very recently. When using "click" we found that 2 clicks were needed to open the awesomebar results _if_ the user clicked on the placeholder text, "Enter Search or Address".
For some reason the placeholder was eating the first "click", so we moved to use "mouseup", which didn't have any placeholder text problems.
Comment 41•15 years ago
|
||
Another thing to the topic directly. At least in the Qt Version I found that there is another general Issue which brings me back to the question if the focus shouldn't get moved away from the textfield when the user hides the keyboard manually.
The fact bringing me to this is that we receive an external focus out event from the system, but somehow do not react on it. Of course there is an Dispatch Deactivate Event (NS_DEACTIVATE) from nsWindow going into xulrunner, but its not respected nor is there any reaction to it.
So I looked a bit deeper and found it strange that in the qt nsWindow Dispatch Events are not called for a TopLevelWindow, but for the the current NsWindow while in all the other ports (gtk, windows,...) its always the TopLevelWindow which gets called.
I'm just about the change that bit and see if there is a change.
As a conclusion, it looks like that there is already coming a "right" behavior from the system, but at least in qt we do not react on it in the correct way. What I don't know is, if this is also the case in gtk.
Updated•15 years ago
|
Component: Linux/Maemo → General
OS: Linux → Linux (embedded)
Hardware: Other → ARM
![]() |
||
Updated•14 years ago
|
Whiteboard: [vkb]
![]() |
||
Updated•14 years ago
|
Priority: -- → P5
Assignee | ||
Comment 42•14 years ago
|
||
fixed by bug 685395.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 11
You need to log in
before you can comment on or make changes to this bug.
Description
•