Brad Lassey [:blassey] (use needinfo?) Assignee
	Comment 1 • 17 years ago

softkb needs to be moved out of minimo and into extensions

	Brad Lassey [:blassey] (use needinfo?) Assignee
	Comment 2 • 17 years ago

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

	Brad Lassey [:blassey] (use needinfo?) Assignee
	Comment 3 • 17 years ago

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

	Doug Turner (:dougt)
	Comment 4 • 17 years ago

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.

	Benjamin Smedberg
	Comment 5 • 17 years ago

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+

	Benjamin Smedberg
	Comment 6 • 17 years ago

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.

	Doug Turner (:dougt)
	Comment 7 • 17 years ago

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?

	Doug Turner (:dougt)
	Comment 9 • 17 years ago

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?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 10 • 17 years ago

+++ 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?

	Brad Lassey [:blassey] (use needinfo?) Assignee
	Comment 11 • 17 years ago

(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?
> 

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 12 • 17 years ago

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.

	Brad Lassey [:blassey] (use needinfo?) Assignee
	Comment 13 • 17 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 14 • 17 years ago

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

	Vitaly Repin
	Comment 15 • 17 years ago

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.

	Antonio Gomes (tonikitoo)
	Comment 16 • 17 years ago

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 ...

	Antonio Gomes (tonikitoo)
	Comment 17 • 17 years ago

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>

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 18 • 17 years ago

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?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 22 • 16 years ago

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.

	Brad Lassey [:blassey] (use needinfo?) Assignee
	Comment 24 • 16 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 25 • 16 years ago

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

	Brad Lassey [:blassey] (use needinfo?) Assignee
	Comment 26 • 16 years ago

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.

	Brad Lassey [:blassey] (use needinfo?) Assignee
	Comment 29 • 16 years ago

author	Brad Lassey <blassey@mozilla.com>
	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

	Antonio Gomes (tonikitoo)
	Comment 30 • 16 years ago

reopening ....

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

	Antonio Gomes (tonikitoo)
	Comment 31 • 16 years ago

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

dep set.

	Antonio Gomes (tonikitoo)
	Comment 32 • 16 years ago

works fine w/ 

xr 85b9917cf2e3
m-b 121ad0a9d105

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

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 33 • 15 years ago

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.

	Antonio Gomes (tonikitoo)
	Comment 34 • 15 years ago

(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

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 35 • 15 years ago

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.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 36 • 15 years ago

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

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 37 • 15 years ago

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.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 38 • 15 years ago

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()).

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 39 • 15 years ago

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

	Brad Lassey [:blassey] (use needinfo?) Assignee
	Comment 40 • 15 years ago

(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.