406837 - clicking on text input box or address bar doesn't raise virtual keyboard on n800

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

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

Comment 1

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

Attachment: fixes soft keyboard extension to work in 1.9

softkb needs to be moved out of minimo and into extensions

Comment 2

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

Attachment: subscribes to software-keyboard and raises the keyboard on the event

This requires the HILDON to be defined in the build system. I'm having trouble making that part work.

Comment 3

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

Attachment: adds HILDON flag to the build system

for this to work, add HILDON=1 to your mozconfig.

as discussed on #mobile, we may want something along the lines of --enable-platform-support=hildon or something else more generic

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 291490 [details] [diff] [review]
fixes soft keyboard extension to work in 1.9

i am not sure why you needed to do this:

-L../../xpcom/string/src/

in the Makefile.in

the ws is a bit off.

Comment 5

[Benjamin Smedberg]

Comment on attachment 291491 [details] [diff] [review]
subscribes to software-keyboard and raises the keyboard on the event

>Index: Makefile.in

>+ifdef HILDON
>+DEFINES		+= -DHILDON
>+INCLUDES	+= -I/usr/include/gtk-2.0/gtk	
>+endif

The INCLUDES change is incorrect. MOZ_GTK2_CFLAGS sets -I/usr/include/gtk-2.0 you should be including any subfiles using

#include <gtk/someheader.h>

Otherwise, this looks fine to me but I'm not a widget peer... please ask roc for r+

Comment 6

[Benjamin Smedberg]

Comment on attachment 291511 [details] [diff] [review]
adds HILDON flag to the build system

By long convention, makefile variables in mozilla start with the NS_ prefix so that random environment variables don't affect the build by accident.

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 291511 [details] [diff] [review]
adds HILDON flag to the build system

couldn't this be something like

NS_PLATFORM_GUI=HILDON  or something like that?

Comment 8

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

Attachment: updated to address comments

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 315975 [details] [diff] [review]
updated to address comments

spacing is off in:
config/autoconf.mk.in

I am sure you don't want the printf()

use strncmp instead of strcmp()

Do we really have to have this code in widget instead of just living in a component similar to what we did with wince?

also, is NS_HILDON what other applications are using?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+++ minimo/components/softkb/Makefile.in	21 Dec 2007 15:54:52 -0000
@@ -35,7 +35,7 @@
 # ***** END LICENSE BLOCK *****
 
 
-DEPTH		= ../../..
+DEPTH		= ../..

I do not understand this.

+		        -lxpcomglue_s   \
+		        -lstring_s      \
+		        -L../../xpcom/string/src/ \

Or this, why can't you use REQUIRES?

@@ -1,4 +1,4 @@
-dnl -*- Mode: Autoconf; tab-width: 4; indent-tabs-mode: nil; -*-
+

And why this?

+    printf("observed: %s\n",aTopic);

Lose this

+        if(NS_LITERAL_STRING("close").Equals(aData)){

Use EqualsLiteral

+    int count = 0;

What's this for?

+    if(!gContext){
+        GdkWindow* winGdk = GDK_WINDOW(topLevelParent);
+        if(winGdk){
+            gContext = gtk_im_multicontext_new ();
+            gtk_im_context_set_client_window (gContext,winGdk);
+            nsCOMPtr<nsIObserverService> observerService = do_GetService("@mozilla.org/observer-service;1");
+            if (observerService)
+                observerService->AddObserver(this, "software-keyboard",PR_FALSE );

Is adding this to every single Gtk widget actually necessary?

> use strncmp instead of strcmp()

Why?

Comment 11

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

(In reply to comment #10)
> +++ minimo/components/softkb/Makefile.in        21 Dec 2007 15:54:52 -0000
> @@ -35,7 +35,7 @@
>  # ***** END LICENSE BLOCK *****
> 
> 
> -DEPTH          = ../../..
> +DEPTH          = ../..
> 
> I do not understand this.
> 


This depends on bug #405716 to move minimo/components/* to extensions/*.  I'd assume that change would be made when they are moved


> +                       -lxpcomglue_s   \
> +                       -lstring_s      \
> +                       -L../../xpcom/string/src/ \
> 
> Or this, why can't you use REQUIRES?
>
The minimo component was written using internal string classes; once moved to extensions it gets linked against external string classes.  Rewritting it to use external interfaces got messy, is there a way to specify internal linkage with REQUIRES?
 
> @@ -1,4 +1,4 @@
> -dnl -*- Mode: Autoconf; tab-width: 4; indent-tabs-mode: nil; -*-
> +
> 
> And why this?

I'm assuming the mode line was added after I moved the code from minimo/components to extensions and I bashed it when I moved it back for the diff.  I'll put it back in.

> 
> +    printf("observed: %s\n",aTopic);
> 
> Lose this
> 

sure thing
> +        if(NS_LITERAL_STRING("close").Equals(aData)){
> 
> Use EqualsLiteral
> 

sure thing

> +    int count = 0;
> 
> What's this for?
>
debugging cruft, I'll remove it
 
> +    if(!gContext){
> +        GdkWindow* winGdk = GDK_WINDOW(topLevelParent);
> +        if(winGdk){
> +            gContext = gtk_im_multicontext_new ();
> +            gtk_im_context_set_client_window (gContext,winGdk);
> +            nsCOMPtr<nsIObserverService> observerService =
> do_GetService("@mozilla.org/observer-service;1");
> +            if (observerService)
> +                observerService->AddObserver(this,
> "software-keyboard",PR_FALSE );
> 
> Is adding this to every single Gtk widget actually necessary?
> 

My intention was to add it to all of the windows, is that not what I'm doing?

> > use strncmp instead of strcmp()
> 
> Why?
> 

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You're adding it to all child widgets --- but not toplevel windows --- as far as I can tell.

Either you're internal or you're external. If you're internal you should be able to use REQUIRES. If you're external you shouldn't be taking shortcuts and trying to use internal interfaces. Up to you.

Comment 13

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

my previous explanation for the EXTRA_DSO_LIBS turns out to be bogus. We are linking as internal.  However, several symbols are missing without that line.  Would this make you feel more comfortable?


  EXTRA_DSO_LDOPTS += $(MOZ_COMPONENT_LIBS) \
+        $(DIST)/lib/$(LIB_PREFIX)xpcomglue_s.$(LIB_SUFFIX) \
+        $(DEPTH)/xpcom/string/src/$(LIB_PREFIX)string_s.$(LIB_SUFFIX) \
         $(NULL)

the first line is used in several make files, you can see here:
http://mxr.mozilla.org/seamonkey/search?string=xpcomglue&find=Makefile.in&findi=&filter=&hitlimit=&tree=seamonkey

the second line is inspired by :
http://mxr.mozilla.org/seamonkey/source/embedding/browser/activex/src/plugin/Makefile.in#212

which is admittedly not a very good source. Also, as it turns out string_s isn't copied to the dist dir at all.  If anyone can tell me the right thing to put in REQUIRES to avoid either of those lines, I'd be happy to use it.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not an expert, bsmedberg should be able to tell us how to use REQUIRES or explain why REQUIRES isn't wanted here.

Comment 15

[Vitaly Repin]

Attachment: microb component. Software keyboard. Can be useful to fix this bug

Several months before I have created this component for microb browser (maemo).

The code structure is pretty straight forward.  If you, guys, have any questions, feel free to ask.
 
I hope it could be useful to fix this bug.

Comment 16

[Antonio Gomes (tonikitoo)]

patches attached to this bug as are will fatally break bug 426344 (gtk focus problems with N800 soft keyboard on OS2008), although they are in the right path. I explain: a "gtkimcontext" object already exists for each (gtk)window (see widget/src/gtk2/nsWindow.cpp at http://lxr.mozilla.org/seamonkey/source/widget/src/gtk2/nsWindow.cpp#5504). Creating a new one from vkb extension can can be problematic then. It is actually possible grab this object with some tweaks to widget side and then from vkb source side:

ENGINE:
(...)
        GtkIMContext *im = IMEGetContext();
+
+        if (mContainer)
+            g_object_set_data(G_OBJECT(mContainer), "imcontext", im);

VKB EXTENSION:
(..)
#ifndef WINCE
GtkIMContext*
nsSoftKeyBoard::GetIMContext () {

  // getting the right gdk_window object needed by gtk_im_* stuff.
  nsCOMPtr<nsIScriptGlobalObject> parentTop = do_QueryInterface(mTopWindow);
  if (!parentTop) return nsnull;

  nsIDocShell *docShell = parentTop->GetDocShell();
  if (!docShell) return nsnull;

  nsCOMPtr<nsIBaseWindow> baseWin(do_QueryInterface(docShell));
  if (!baseWin) return nsnull;

  // get the root origin for this content window
  nsCOMPtr<nsIWidget> mainWidget;
  baseWin->GetMainWidget(getter_AddRefs(mainWidget));

  mGdkWindow = NS_STATIC_CAST(GdkWindow *, mainWidget->GetNativeData(NS_NATIVE_WINDOW));

  if (!mGdkWindow) return nsnull;

    GList *children, *child;
    children = gdk_window_peek_children(mGdkWindow);

    for (child = children; child != NULL; child = child->next)
    {
        gpointer user_data = NULL;
        gdk_window_get_user_data(GDK_WINDOW(child->data), &user_data);
        if (user_data)
        {
            gpointer data = NULL;
            data = g_object_get_data(G_OBJECT(user_data), "imcontext");
            if (data) {
              return (GtkIMContext *) data;
            }
        }
    }
    return nsnull;
}
#endif
(..)

doug, blizzard do you think it is reasonable to get checked in ? 

taking the bug ...

Comment 17

[Antonio Gomes (tonikitoo)]

Attachment: make hildon|gtk_im_context_show|hide to get called from extensions/softkb code instead of from widget. AFAIU, calling extensions' code from mozilla guts is not a very good practice.

maybe previous patches were tweaking the widget/ too much. I updated it according to doug's comment #9

issues: 

1) if patch is OK I can recreate it changing HILDON by NS_HILDON or even trying to add support to something like "--enable-platform-support=hildon" .. what do you think ?

2) it seems like current panning implementation does not let 'content' to trigger DOM events properly (e.g. 'blur') or even to content to get focused. Btw, that kinda breaks softkb behavior with mine and blassey's patch. mfinkle ?

to apply:
$ cd mozilla
$ patch -p1 < ../<path-to-patch>

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You should request review from someone specific.

Would it make more sense to define a method in nsGTKToolkit, which is basically global, to get the IM context you want?

Comment 19

[Antonio Gomes (tonikitoo)]

Attachment: mozconfig used to build mobile and xulrunner

Comment 20

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

Attachment: slightly updated and moved to a fennec component

Comment 21

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

Attachment: adds IMContext to gtk nsWindow

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 330002 [details] [diff] [review]
slightly updated and moved to a fennec component

I'm not really the right person to review this, not sure who is.

Comment 24

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

Attachment: IME approach

This patch has two issues with fennec.  In the address bar, each key press steals focus, which closes the awesome bar until the next key press.  The end effect is that every other key press is lost.  

Also, clicking on a text box rendered to the canvas has no effect.  We might have to do something in the fennec chrome to get around that.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

how does the focus-stealing happen? the soft keyboard steals it?

Comment 26

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

the autocomplete pop up is stealing it, but not completely.  The caret is still in the url entry bar, but the input doesn't go to the bar.

Comment 28

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

Attachment: shows/hides based on state and not im focus

Comment 29

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

author	Brad Lassey <[e-mail]>
	Wed Sep 10 12:12:58 2008 -0400 (at Wed Sep 10 12:12:58 2008 -0400)
changeset 19111	78f592a0a109
parent 19110	0230a5162f93
child 19112	de4f208c9413

Comment 30

[Antonio Gomes (tonikitoo)]

reopening ....

reproducible w/ fennec 0.9pre from http://people.mozilla.com/~pavlov/fennec/hourly

Comment 31

[Antonio Gomes (tonikitoo)]

neil's patch in bug 455891 reverts patch bug 454235 changes, and seems to fix this vkb problem.

dep set.

Comment 32

[Antonio Gomes (tonikitoo)]

works fine w/ 

xr 85b9917cf2e3
m-b 121ad0a9d105

vkb popsup/hides according to focus in/out event on both content and chrome

Comment 33

[Joel Maher ( :jmaher ) (UTC -8)]

I am having trouble with this specific issue on build 20090617 (from the nightly drop) when using my nokia n810.

I have tried in regular app mode as well as full screen.  The soft keyboard never pops up when clicking on the URL bar or other text input fields.

Comment 34

[Antonio Gomes (tonikitoo)]

(In reply to comment #33)
> I am having trouble with this specific issue on build 20090617 (from the
> nightly drop) when using my nokia n810.
> 
> I have tried in regular app mode as well as full screen.  The soft keyboard
> never pops up when clicking on the URL bar or other text input fields.

joel I am assuming you have set you IM to popup vkb in n810.

-> control panel -> text input settings -> on screen -> enable stylus input method

Comment 35

[Joel Maher ( :jmaher ) (UTC -8)]

yes, this is checked, thanks for double checking.  This has worked before in the past, it is just odd that it stopped working all the sudden.

Comment 36

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

Joel - If you load google.com, does the textbox on the page get focus? (cursor and yellow background)

Comment 37

[Joel Maher ( :jmaher ) (UTC -8)]

mfinkle, yes it does.  for some reason that brings up the soft keyboard, but youtube does not.  Both sites have focus (including yellow background) on the search box.

I have not seen the soft keyboard for the url bar on any site.

Comment 38

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Is this a trunk build?  If so, the recent focus rewrite could be to blame -- I've seen some problems on Windows CE, being unable to type in input boxes in dialogs (e.g. prompt()).

Comment 39

[Joel Maher ( :jmaher ) (UTC -8)]

this is built from trunk (xulrunner-1.9.2a1pre builds)

Comment 40

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

(In reply to comment #37)
> mfinkle, yes it does.  for some reason that brings up the soft keyboard, but
> youtube does not.  Both sites have focus (including yellow background) on the
> search box.
perhaps you're seeing what's being described in bug 511975. If that is the case, let's re-close this bug and concentrate on this particular issue in that bug.