533245 - Can't give focus to focusable element in an embedded browser widget by mouse click

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Embedding: GRE Core, defect)

Description

[Xianzhu Wang]

User-Agent:       Mozilla/5.0 (Linux; U; en-US; rv:1.9.1.0) Gecko/20070309 Firefox/3.0.1
Build Identifier: 1.9.2b4

Similar to bug 490447, but with more severity. In bug 490447, focus can't be transferred between two embedded browser widgets in one window by mouse click. In 1.9.2b4, focus can't be given to single embedded browser widget by mouse click.

Reproducible: Always

Steps to Reproduce:
1. Compile 1.9.2b4 source code with gtk embedding and tests enabled.
2. Run the TestGtkEmbed example
3. Load http://www.google.com
4. Click the google search input box in the page
Actual Results:  
The search input box can't be focused by click

Expected Results:  
The search input box should be focused.

Comment 1

[Xianzhu Wang]

Just verified that:
1. Only text input boxes can't get focus by click;
2. Other elements like buttons, radio buttons, etc. can get focus by click;
3. Once any another element in the embedded widget got focus, then text input boxes can get focus by click;
4. Tab key works;

Comment 2

[Xianzhu Wang]

Just found a workaround about this problem:

In content/events/src/nsEventStateManager.cpp:

2882      if (newFocus && currFrame) {
...

Added the following lines:

    EnsureDocument(mPresContext);
    if (mDocument) {
      fm->SetFocusedWindow(mDocument->GetWindow());
    }

Before this change, when clicking a focusable element in a embedded browser before the browser window is focused, only SetFocus() is called for the element, while the embedded browser window is still not focused. Adding SetFocusedWindow() is to ensure the embedded browser window is also focused.

I'm wondering if this is the correct solution.

Comment 3

[Xianzhu Wang]

(In reply to comment #1)
> Just verified that:
> 1. Only text input boxes can't get focus by click;
> 2. Other elements like buttons, radio buttons, etc. can get focus by click;

Correction to 1 and 2: All focusable elements can't be focused by click directly. However, an element can be focused by clicking its associated label.

> 3. Once any another element in the embedded widget got focus, then text input
> boxes can get focus by click;
> 4. Tab key works;

Comment 4

[david kennedy]

Confirmation of this bug report.
Summary:
Text entry boxes in Firefox v:3.5.8 either fail to gain focus on a mouse click or loose focus when typing.
Firefox v:3.5.8 64 bit.
System: 
OS:
openSuSE linux 11.2
Linux 2.6.31.12-0.2-desktop x86_64
KDE 4.3.5
Hardware:
Dual Core AMD Opteron 265, 1000 MHz 4 Cores.
Memory 3.8 GB. Swap 4.0 GB.
GPU: nvidia Quatro FX 1300.
Notes:
On a system restart the problem is removed. After some use of firefox, the problem gradually becomes more severe. Eventually only pasting from an editor will permit text entry into (for example) a Google search text box.
Problem may be exacerbated by having multiple Firefox windows open with one or more receiving push data.

Comment 5

[Carey Stevens]

This problem can also be reproduced in win32 using the win32_test application
from the Embedding NEWAPI.

Build win32_test.exe against a 1.9.2 xulrunner and text can't be entered into
the google search box.

Adding the patch in Comment 2 alone is not enough to enable the text input,
because the call to SetFocusedWindow() susequently calls

nsFocusManager::RaiseWindow(nsPIDOMWindow* aWindow)
{
  // don't raise windows that are already raised or are in the process of
  // being lowered
  if (!aWindow || aWindow == mActiveWindow || aWindow == mWindowBeingLowered)
    return;

which returns without doing anything because the aWindow == mActiveWindow test
is true

Removing the mActiveWindow test results in the window focus being set and text
input working. Though the caret still does not show.

I guess the correct fix would involve not actually calling RaiseWindow but
setting the focus some other way. It looks like if Focus() in nsFocusManager
was called the caret would be updated too.

Comment 6

[Karl Tomlinson (:karlt)]

I'm not clear how exactly the client is meant to tell the focus manager that the toplevel window has focus but the browser does not have keyboard focus.

Comment 7

[Carey Stevens]

(In reply to comment #6)
> I'm not clear how exactly the client is meant to tell the focus manager that
> the toplevel window has focus but the browser does not have keyboard focus.

An embedding client makes a call like
    nsCOMPtr<nsIWebBrowserFocus> browserFocus;
    browserFocus = do_QueryInterface(mWebBrowser);
    browserFocus->Activate();

which I believe should both activate and give focus to the window, however it is only being activated and not focused. 
eg FocusManager's mActiveWindow is set but mFocusedWindow is not.

Comment 9

[Tim Kersten]

Attachment: workaround patch for 1.9.2.13

Attached workaround patch based mostly on the above comments.

Comment 10

[Tim Kersten]

Attachment: Workaround patch v2 for 1.9.2.13

Who actually uses embedded XULRunner?

I'm asking since in an unpatched state, the current embedded xulrunner is critically broken.

The previously posted patch workaround only works for widgets, but it turns out that focus is still not given to other elements (on click), so javascript listening for key events fails. I've short circuited another bit of code to make this work, but haven't tested it extensively. Widgets, and other elements now seem to get keyboard focus. 

Tabbing into the embedded xulrunner works, unpatched.

Comment 11

[Chad Austin]

Tim: We at IMVU embed XULRunner 1.9.1, but for these focus reasons, we've been unable to upgrade to 1.9.2.  I'm very hopeful that your patch will unblock us.

Comment 12

[Tim Kersten]

Attachment: GTK MozEmbed focus fix.

My previous work around addresses some of the symptoms caused by the bug, but causes others. I took another look and believe I found the cause, at least for the GTK Embedded version: Click's aren't propagated to the embedded browser, and the relevant code to set the focus is not called when it needs to be... i.e. what a focus_in event would normally do.

Anyway, http://groups.google.com/group/mozilla.dev.embedding/msg/98cb5d92e48f5d01 says:
"recommend that you *not* use GtkEmbedMoz"
and
"The GtkEmbedMoz code will probably be removed in a future release of 
Mozilla because it is buggy and unmaintained, and not the direction we 
want for embedding in general."

Comment 13

[Whatever J.]

I was experiencing the same problem with xulrunner 2.0 [1] and win32_test [2], but it was working fine with winEmbed [3]. So I took win32_test apart and found it works with this implementation:

NS_IMETHODIMP WebBrowserChrome::GetVisibility(PRBool * aVisibility)
{
    NS_ENSURE_ARG_POINTER(aVisibility);
    *aVisibility = PR_TRUE;
    return NS_OK;
}

It was returning NS_ERROR_NOT_IMPLEMENTED, which seems to cause problems with focus. Since this is in common it would explain why gtk has the same issue.


[1] http://releases.mozilla.org/pub/mozilla.org/xulrunner/releases/2.0rc1/
[2] http://hg.mozilla.org/incubator/embedding/file/29ac0fe51754/
[3] http://hg.mozilla.org/mozilla-central/file/4512b571e48e/embedding/tests/winEmbed/

Comment 14

[Daniel Drake]

Thanks for the useful info on this bug report. We are facing the same issue at http://dev.laptop.org/ticket/10623 using embedding through hulahop (http://wiki.laptop.org/go/HulaHop)

Fixing hulahop in the same way that GTKMozembed was fixed by Tim Kersten solves the problem, but it does rely on the change to mozilla-1.9.2/widget/src/gtk2/nsWindow.cpp:button_press_event_cb to allow other handlers of button_press_event outside of mozilla itself.

But, I am unsure as to whether this is the right fix, I have several questions:

1. Why did this work before (in v1.9.1?) and not now?
2. Why does this work in Firefox but not in my own embedding via hulahop?
3. Why does button_press_event in gtk2/nsWindow.cpp return TRUE, blocking all other handlers of this event?
4. Why doesn't gtk2/nsWindow.cpp handle the focus-in focus-out dance all on its own? It does have handlers for focus-in, focus-out and button-press which are called first, yet us embedders are required to add our own handlers on top of these which do routine things like "widget was focused, tell mozilla that we have focus"
5. As I'm new to Mozilla stuff, I don't understand how the suggestion from Whatever J. applies. I can't see where to slot in such GetVisibility implementation into hulahop or gtkmozembed. Or does WebBrowserChrome represent the current "direction for embedding" that users like hulahop/gtkmozembed should rewrite themselves around?

Comment 15

[Karl Tomlinson (:karlt)]

(In reply to comment #14)
> 1. Why did this work before (in v1.9.1?) and not now?

Don't know but nsIFocusManager might have something to do with it.

> 2. Why does this work in Firefox but not in my own embedding via hulahop?

Firefox has only one GtkWidget (except when there are plugins).

> 3. Why does button_press_event in gtk2/nsWindow.cpp return TRUE, blocking
> all other handlers of this event?

It needs to return TRUE when the event is handled, so that we don't get two actions (eg. two context menus).  It could return FALSE when the event is not handled, but I don't know whether or not that is a simple change.

> 4.

Don't know enough to comment.

> Or does WebBrowserChrome
> represent the current "direction for embedding" that users like
> hulahop/gtkmozembed should rewrite themselves around?

Bug 648156 seems to imply there is a new direction.  I haven't read the thread so don't know what that direction is.

Comment 16

[Daniel Drake]

Thanks, that all helps. I've read bug #648156 and that thread, and I'm none the wiser as to how embedding should happen.

Also, hulahop does take the same approach as winEmbed in that it implements WebBrowserChrome. Hulahop's GetVisibility() method is working correctly and returning TRUE, so I think Whatever J's issue is different.

Comment 17

[Daniel Drake]

Attachment: New patch: connect gtk2 signals later

Here is my proposed fix for this issue.

Right now, Mozilla's gtk2 widget connects a load of signals at GTK's "BEFORE" level (i.e. the handlers get executed first) and hence doesn't allow the user of the widget to attach any signal handlers before mozilla does it's work.

In the case of this bug, we've identified that Mozilla doesn't like to process its button_click event without the web browser being told it has focus, but with the current implementation, the button_press_event handler returns TRUE (disabling any handlers the user might have added).

By connecting these signals in the AFTER stage, external widget users have the option of connecting signal handlers both before (via g_signal_connect) and after (via g_signal_connect_after; this is guaranteed to be executed after Mozilla's own, because signals are delivered to handlers in the order that they were connected).

This will help embedders with this issue and others, and should not have any effect for firefox which does not bind any other handlers to these signals.

Comment 18

[Daniel Drake]

and just to complete the story... with that fix in place, embedders such as gtkmozembed and hulahop are required to connect a button_press_event handler at the "BEFORE" level (g_signal_connect()) which tells the web browser component that it has focus, and returns FALSE (so that Mozilla's own handler then gets executed).

Comment 19

[Karl Tomlinson (:karlt)]

One way for embedders to get GtkWidget signals before Gecko, without any Gecko changes, is to connect to the GtkWidget::event signal and check the type of the "event" parameter.

Comment 20

[Daniel Drake]

Thanks - that works. So, this embedding oddity can be worked around entirely outside Mozilla (meaning that my above patch is not really necessary), and I've pushed a patch to hulahop which does just that.

Comment 21

[Jerry Evans]

(In reply to Whatever J. from comment #13)

Big thanks. This has been a problem in my MFC wrapper for ages, which was *wrongly* doing this:

    if(! m_pBrowserFrameGlue)
         return NS_ERROR_FAILURE;
    m_pBrowserFrameGlue->GetBrowserFrameVisibility(aVisibility);
    return NS_OK;

> 
> NS_IMETHODIMP WebBrowserChrome::GetVisibility(PRBool * aVisibility)
> {
>     NS_ENSURE_ARG_POINTER(aVisibility);
>     *aVisibility = PR_TRUE;
>     return NS_OK;
> }
>

Comment 22

[Benjamin Smedberg]

Mass change of bugs in the "Embedding: GRE Core" component in preparation for archiving it. I believe that these are no longer relevant; but if they are, they should be reopened and moved into a component relevant to the code in question.