Enhancement - FocusHandler Take4

[amaitland]

Another attempt at getting focus to work correctly - Simplest approach possible this time

Only feature that's not currently working when compared to #702 is when you minimize then maximize the window when the browser had focus, it's lost. It should be fairly noninvasive.

• Made DefaultFocusHandler methods virtual
• Untabify DefaultFocusHandler
• Implement OnEnter/OnLeave
• Move InvokeOnUiThreadIfRequired into WinForms project

[amaitland]

I think it's worth going forward with this for starters. User can implement OnLostFocus and OnGotFocus should they wish to send focus to the browser upon maxamize.

[amaitland]

@jankurianski Will these changes impact your current FocusHandler usage scenario?

[jankurianski]

//Notify -> // Notify

[jankurianski]

@amaitland I think this is a good change. It's got the good default WinForms focus behaviour that @sylvain-hamel was looking for in #702. But this also retains IFocusHandler for special scenarios.

My scenario is wanting to prevent the Chromium control from taking focus upon navigation or click. So I checked out this PR, and was able to set my own FocusHandler that always returns true in OnSetFocus. It worked as expected.

I also tried the test cases in #702 with the DefaultFocusHandler and they worked as well.

Not sure where the conflict is in this PR, as I was able to build OK. I think you should go ahead with the merge so we can get this good focus behaviour in 39 👍

[amaitland]

@jankurianski Greatly appreciated! I'll merge in master manually and fix the minor formatting issues 👍

[sylvain-hamel]

Hi @amaitland, thanks for your work on that. I'll give it a go and I let you know if I find anything.

Only feature that's not currently working when compared to #702 is when you minimize then maximize the window when the browser had focus, it's lost

Will you log a separate issue for this specific bug? Do you know the cause of the bug?

[sylvain-hamel]

Hi @amaitland,

The bug does not only affect minimize/maximize. It affects any form of deactivation/activation of the application, which is very important:

• start sample app
• click in browser
• alt-tab to other application
• alt-tab back to sample app
• bug: focus is lost

The reason is that you took out the part where we used ChromiumWebBrowserMessageInterceptor to handle messages of the last child window.

The code in DefaultFocusHandler is correct:

        public virtual void OnGotFocus()
        {
            browser.InvokeOnUiThreadIfRequired(browser.Activate);
        }

But as discovered earlier while investigating all this, IFocusHandler::OnGotFocus is not called when you deactivate/activate application. This is why we have to watch messages of the last child window.

Can you bring this code back?

Recall that after you had discovered the name of that window, I suggested that the code could be like this:

    // The window that gets the focus is a child window of the browser and is called
    // Chrome_RenderWidgetHostHWND, see See https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/hmBh5YHjOFY
    var cefWindowHandle = SomeHelper.GetSubWindowsByName(
        webBrowser.Handle, "Chrome_RenderWidgetHostHWND");      

instead of

    User32.GetSubWindows(webBrowser.Handle, childWindows);      
    var cefWindowHandle = childWindows.Last();

See discussion in #710 for more detail.

[amaitland]

@sylvain-hamel As it stands I'm not prepared to spend any more time on this. Your welcome to provide another PR with a GetChildWindowByName method.

[amaitland]

It's also worth noting that I created NativeMethodWrapper.h so rather than dll imports native calls can be exposed pretty easily 👍

[rassilon]

So, I've done some looking into this.

It seems our troubles stem from:

• Win32 on activation of a top level window (i.e. your Form subclass) does this by default:

When a top level window loses focus, then regains it, the Win32 DefWindowProc() assigns focus to the first child with the WS_TABSTOP style, in my case, that's a toolbar. It does not return to the actual control that had focus.

That is from: http://cboard.cprogramming.com/windows-programming/141510-wm_activate-woes.html

• WinForms in order to deal with this, handles the activation message WM_ACTIVATE and utilizes ContainerControl to find the innermost control that needs focus again and supplies the control with a WM_SETFOCUS message due to calling the Win32 API SetFocus().
• There is no easy way to make the HWND from ClientAdapter->GetBrowserHwnd() part of this ContainerControl system. (Presuming, that's even the HWND we need to care about, and based on other notes I've seen, it doesn't appear to be.)

Since we seem to need the Chrome_RenderWidgetHostHWND window class HWND underneath GetBrowserHwnd().

Does that about sum it up? If so, then the core problem seems to be Chromium ought to expose the current focus HWND out to CEF, for CEF to expose to us. That way regardless of whether it's the legacy RenderWidgetHostHWND or some replacement we wouldn't care about the Window Class name.

That way, we might be able to sanely construct a subclassed WndProc for the owning Form of the ChromiumWebBrowser control that could make these kinds of decisions.

(If we couldn't, it wouldn't be CEF's fault, it might be due to difficulty in working well within the constraints of how WinForms code is structured in this area.)

Regarding, cefclient: cefclient doesn't have this problem because it doesn't implement
a focus handler AND none of the other controls it creates are marked as being tab stops. This is evidenced by putting the cursor at the end of the address bar and hitting the TAB key.

@sylvain-hamel, @amaitland , does this make sense based on your own investigation?

If it does, we should ask for my suggestion from upstream.

Thoughts?
Bill

[sylvain-hamel]

@rassilon I think you have indeed identified the root cause.

As for what the right fix is, I'm not certain.

I read @amaitland 's comment in #672 ...

I've been very reluctant to implement the changes proposed by #702 as it relies on an undocumented CEF HWMD. It is entirely possibly to replicate the code in user land (same as this issue can be fixed by the user), though I do appreciate that it's ideal for the framework to handle common scenarios where possible. (Just a bit of background if you do look over #702 in detail).

... and I tend to agree. If we send the request for change upsteam and if they fix it rapidly, then I'd be ok to keep this bug in CefSharp until it's fixed. But if it's going to take a while, then I'd rather have a temporary solution that is based on a undocumented thing then a bug. How long do you think it will take to get the fix? And do you feel like they are going to fix v39?

Thoughts?
Sylvain

[rassilon]

After some surface examination of the underlying Chromium code, it appears that asking for a way to forward activation messages into Chromium through Cef might be the approach since hwnds are so Windows specific.

I'll write up an email for Cef and Chromium development and see what they think.

If upstream is willing to provide a way to do this in the future, I would tend to be less concerned about the window class name fragility.

There might be a way without using the window class name with some fancy wm_syscommand sc_minimize/wm_activate juggling. I'm just not convinced that's a stable long term solution either even if it appeared to work in tabulation demo.

Bill

[sylvain-hamel]

@rassilon Thanks!

[RadicalLinux]

Okay actually there is one issue with the commit.
The SetFocus In CefSharp.Winforms - ChromiumWebBrowser.cs needs to check for null, if it doesn't, it causes exceptions to occur on some machines. Here is what it should be:

        public void SetFocus(bool isFocused)
        {
            if (IsBrowserInitialized && managedCefBrowserAdapter != null)
            {
                managedCefBrowserAdapter.SetFocus(isFocused);
            }
        }

For some reason on some machines, this gets called during Cef.Shutdown and causes a NullReferenceException, but adding the null check fixes the issue.

[amaitland]

@RadicalLinux Submit a PR 👍