Focus improvements, take 5 #791

rassilon · 2015-02-06T04:34:52Z

...oo so other people can more easily verify the changes.

The additions to MovingListener (and the fields it sets on ChromiumWebBrowser) are ONLY there for debugging related purposes. They will disappear when folks are happy with the results.

…g goo so other people can more easily verify the changes. The additions to MovingListener (and the fields it sets on ChromiumWebBrowser) are ONLY there for debugging related purposes. They will disappear when folks are happy with the results.

rassilon · 2015-02-06T04:38:42Z

@sylvain-hamel , @amaitland : Please try this PR out and let me know if you see anything that behaves incorrectly, and then send me the Debug output for the incorrect behavior.

TL;DR: ChromiumWebBrowser should only pay attention to OnGotFocus events. OnEnter/OnLeave only have meaning from a wider WinForm control validation perspective, and they are not (as you can see below) called during (de)activation. If you were writing a control that contained ChromiumWebBrowser and supported WinForm validation goo, you'd need to do some extra magic. But, that magic would be there and not in ChromiumWebBrowser.

The long version:
Bah, I think we're over thinking this. OnEnter/OnLeave aren't invoked during Form (de)activation.
Having an OnGetFocus() handler that unconditionally calls .SetFocus(true) seems to work for the cases I saw mentioned:

Tabbing in the tab demo form.
Minimizing/restoring
Alt/Tabing away and back again.

WinForms actually (due to our handy call to ControlContainer.ActivateControl from the CEF focus callback) manages to track our ActiveControl state for our control and generates any necessary SetFocus() calls for us (and thus triggering OnGetFocus).

I'll added some debugging logic to the pending MovingListener to track form activation/deactivation for possible logic inside ChromiumWebBrowser in order for me and others to understand the various event sequences involved.

Here are the sequences I tested:

Reminder: Not all of these operations execute on the same thread, so the ordering may seem slightly odd.

For tabbing to ChromiumWebBrowser just after app/browser creation:

WinForms calls OnEnter for ChromiumWebBrowser
WinForms calls OnGetFocus for ChromiumWebBrowser
ChromiumWebBrowser.OnGotFocus calls SetFocus(true)
FocusHandler.OnSetFocus calls with FocusSourceSystem since we allow it.
FocusHandler.OnGotFocus gets called by CEF.
FocusHandler.OnGotFocus calls ChromiumWebBrowser.Activate, which marks ChromiumWebBrowser as the ActiveControl.
ChromiumWebBrowser.OnGetFocus is NOT called here (I think thanks to WinForms thinking rightly that SetFocus loops are silly.
ChromiumWebBrowser.OnLeaveFocus gets called because CEF now has focus.

For alt-tabbing away: (the CEF window still has focus)

WM_ACTIVATE is sent to the parent Form with 'inactive'
WinForms calls OnDeactivated for the parent Form.
No OnLoseFocus calls are received by ChromiumWebBrowser because CEF had focus.
FocusHandler doesn't have a callback for that focus loss (which we don't appear to need anyway)
WM_ACTIVATE processing ends.

For alt-tabbing back hoping the CEF control still has focus:

WM_ACTIVATE is sent to the parent Form.
WinForms notices that ChromiumWebBrowser is the ActiveControl and ends up calling OnGotFocus on ChromiumWebBrowser.
OnGotFocus calls SetFocus(true) on CEF
FocusHander.OnSetFocus is called with FourceSourceSystem.
OnActivated is called on the parent form.
OnLostFocus is called on ChromiumWebBrowser due to CEF acquiring focus.
WM_ACTIVATE processing is completed by the parent Form.
FocusHandler OnGotFocus is called because CEF received focus which updates WinForms ActiveControl to ChromiumWebBrowser.
The CEF control DOES have focus. Yay!

For minimizing (while CEF still has focus):

OnDeactivate is called on the parent form.
OnLostFocus isn't called on ChromiumWebBrowser due to CEF having focus.

For restoring:

WM_ACTIVATE processing starts, calls OnActivate, and ends on the parent Form.
ChromiumWebBrowser OnGotFocus is called by WinForms issuing a SetFocus for ActiveControl.
OnGotFocus calls SetFocus(true) on CEF
FocusHander OnSetFocus is called with FourceSourceSystem.
'OnLostFocusis called onChromiumWebBrowser` due to CEF acquiring focus.
OnActivated is called on the parent form.
Again, OnGetFocus is not called on ChromumWebBrowser due to ActiveControl already having focus.
WM_ACTIVATE processing is done.
CEF still has focus, Yay!

That sure was a LOT of work... ugh,
Bill

amaitland · 2015-02-06T05:02:24Z

@rassilon Amazing effort! Not long until I finish up for the day, I'll try and take a look over the weekend 👍

amaitland · 2015-02-06T11:56:49Z

@rassilon I see MovingListener has activation handling code, yet it's not instantiated. Am I missing something?

rassilon · 2015-02-06T13:56:28Z

Yes, this last commit where I add that bit on this branch. All of the testing occurred before I created this PR branch.

Bill

rassilon · 2015-02-06T14:01:59Z

Oh, additionally:

When you tab out of the browser control (and into the Dummy Text box):

FocusHandler.OnTakeFocus is invoked from CEF
OnLeave is called by WinForms because we've moved to the next control.
OnLostFocus is NOT called (and we don't care) because CEF is what HAD focus.
If we needed to, it seems feasible that we might be able to cause our OnLostFocus event to fire from FocusHandler.OnTakeFocus. I'm just not sure if its worth it, and what the logic overhead of ensuring we don't try to accidentally call CEF SetFocus(true) again.

Bill

rassilon · 2015-02-06T14:42:04Z

The AppVeyor failure was due to:

NuGet.exe : Unable to find version '3.2171.1979' of package 'cef.redist.x64'

Fyi,
Bill

rassilon · 2015-02-06T14:56:53Z

Oh, if you launch these changes with BrowserForm instead of the Tabulation Demo don't be shocked when you can't tab into the browser.

I'm not entirely precisely sure why that happens at present. It's probably due to WinForm specifics in UserControl that aren't there in Control. (It's not JUST about TabStop = true sadly, something else is going on.)

However, I think that makes sense in any event. ChromiumWebBrowser shouldn't have a policy about how its hosted from that perspective.

Bill

jornh · 2015-02-06T16:00:44Z

Poked the RE-BUILD PR on AppVeyor a few times to no avail (probably timeout towards the myget.org CI feed).

After pushing the cef.* 3.2171.1979 packages to www.nuget.org and another poke all ✅ again. @rassilon thanks a lot for the heads-up 👍

amaitland · 2015-02-10T07:30:28Z

Unfortunately I think we still have a problem. VS 2012 isn't playing nice at the moment so I don't have the Kernel Debug output, I'll look at that when I get a sec.

Steps to reproduce

Start another Application (e.g. notepad)
Start WinForms Example
Give browser Focus
Click inside notepad
Click inside textbox1
Expected: textbox1 should have a yellow background
Result: textbox1 Enter doesn't have a yellow background (I'm guessing the Enter event isn't fired)

rassilon · 2015-02-10T19:55:04Z

OnEnter and OnLeave aren't called during (de)activation. Only OnGotFocus/OnLostFocus are called during (de)activation.

As a reminder, OnEnter/OnLeave are primarily about WinForm's validating controls stuff, and txtUrl isn't a validating control thing, its more of just a testcase atm. :)

However, even taking that into consideration, I am seeing CEF telling IFocusHandler that it received focus during WM_ACTIVATE while also noticing that txtURL has its OnGotFocus method called. This leads to a timing issue where when the VS IDE doesn't get in the way, txtURL is the focus winner, but when the IDE does get in the way, CEF is the winner.

I haven't quite figured out why things are working this way just yet, so I'm not sure what the proper fix might be (if any). When the IDE does get in the way, the IDE is doing something really slowly (usually symbol loading).

Fyi,
Bill

amaitland · 2015-02-11T01:07:15Z

As a reminder, OnEnter/OnLeave are primarily about WinForm's validating controls stuff, and txtUrl isn't a validating control thing, its more of just a testcase atm. :)

Do you have some additional background information on that? I'm no WinForms expert, so I'm really only going on the brief amount I've read on MSDN. I would have though that Validating and Validated would have been the ones related to validation. Enter/Leave by all accounts relate to a control gaining, losing focus.

Appreciate all the time you've spent on this 👍

rassilon · 2015-02-11T20:35:51Z

Here's the bit I saw explaining OnEnter vs OnGotFocus from SO:

http://stackoverflow.com/questions/2702055/what-is-the-difference-between-the-control-enter-and-control-gotfocus-events

Bill

amaitland · 2015-02-12T00:10:47Z

Thanks 👍

Merge remote-tracking branch 'origin/master' into focus_take5 Conflicts: CefSharp.WinForms/ChromiumWebBrowser.cs

rassilon · 2015-03-03T07:10:27Z

Anybody want to try this out outside of the debugger and see if they can break it? I'm hoping the issue I saw just won't repro outside of the debugger.

Bill

amaitland · 2015-03-05T12:40:20Z

I had a quick try and still seeing the same text box activation problem (not turning the background yellow). I'll provide a more detailed response when I get a chance.

rassilon · 2015-03-05T16:41:33Z

Hrm. Are we trying to ensure the ChromiumWebBrowser only gets focus when it should and that focus is sent to the correct control during activation if the last control that had focus wasn't the ChromiumWebBrowser control? Or are we trying to perfect turning the address bar yellow as well in order to verify controls that have custom focus behavior can have sane focus interactions with ChromiumWebBrowser if they do the correct things?

I hadn't (up to now) been caring too much if the address bar didn't turn yellow when clicking into the address bar (during activation) since the correct event wasn't being listened to.

Thoughts?

Bill

amaitland · 2015-03-05T20:45:42Z

@sylvain-hamel Any comment?

sylvain-hamel · 2015-03-06T04:18:37Z

@rassilon, @amaitland we are trying to get the textbox to turn yellow because Cef/CefSharp prevents it from doing so. That's because of the way it handles activation/deactivation. You can refer to my original PR to see this test case working.

rassilon · 2015-03-06T05:31:50Z

@sylvain-hamel , do you want the address bar to turn white during deactivation? (i.e. alt-tab, or click on other app, etc...)

I think I found the other problem that was still hiding.

Bill

…ebBrowser was the .ActiveControl before the app was deactivated. This will allow WinForm's .ActiveControl logic to occur properly. Update address bar logic to deal with BackColor being in yellow whenever the control has focus.

rassilon · 2015-03-06T07:34:34Z

See the long essay I wrote in DefaultFocusHandler for the detail that was missing.

Please give this a spin everyone. ( @amaitland , @sylvain-hamel )

Thanks,
Bill

amaitland · 2015-03-06T08:05:40Z

@rassilon Great! Will check it out when I'm back in front of a PC with VS installed 👍

rassilon · 2015-03-06T08:06:41Z

Cool. If it makes folks happy I'll do a cleanup pass.

Bill

rassilon · 2015-03-06T15:47:32Z

Here's the TL;DR of the problem:

Top level windows that exist in different threads than the main WinForm UI thread need to ensure they play nicely with WinForms .ActiveControl management during application activation if the window hierarchy for your WinForms Form spans threads as it does for WinForms containing a CEF UI.

Since the ChromiumWebBrowser control is on the WinForms UI thread, but its child window CEF is on the CEF UI thread.

Bill

amaitland · 2015-03-09T05:37:48Z

@rassilon Looking very very promising 😄 Fantastic work 👍

@sylvain-hamel Mind taking the latest changes for a spin?

amaitland · 2015-03-12T10:28:21Z

@rassilon I'm happy for you to merge this one when your happy that it's ready! After you merge just need to add an entry to the ChangeLog 👍

rassilon · 2015-03-12T10:46:42Z

I'll see about cleaning up the logging on Saturday and then merge it in. I'm busy having fun with Windows image creation and dism.exe this week at work.

Thanks for trying this PR out.

Bill

amaitland · 2015-03-12T11:36:27Z

I'm busy having fun with Windows image creation and dism.exe this week at work.

No problem 👍

sylvain-hamel · 2015-03-13T14:57:19Z

Hi @rassilon, @amaitland I'm sorry but it might take a while before I can give this a try.

amaitland · 2015-03-16T03:53:30Z

CefSharp.WinForms/ChromiumWebBrowser.cs

+        /// Set to true while handing an activating WM_ACTIVATE message.
+        /// MUST ONLY be cleared by DefaultFocusHandler.
+        /// </summary>
+        internal bool IsActivating = false;


Should we expose this for users to set should they choose to implement IFocusHandler totally from scratch? Potential side effects?

I suppose you're right and it should be available to be set by their own IFocusHandler. I'll fix it up.

Focus improvements, take 5...

amaitland · 2015-03-17T22:49:57Z

Just an FYI I've renamed this PR and added it to the ChangeLog 😄

Think we can push ahead with 39 now 👍

Readd ServiceModel reference in this branch.

e200b6e

amaitland added this to the 39.0.0 milestone Feb 6, 2015

Actually invoke MovingListener on this branch.

2cea509

amaitland added the winforms label Feb 24, 2015

rassilon added 2 commits March 3, 2015 02:00

Update to master/HEAD

1ef60f3

Merge remote-tracking branch 'origin/master' into focus_take5 Conflicts: CefSharp.WinForms/ChromiumWebBrowser.cs

Move WM_ACTIVATE logging from MovingListener to Interceptor.

340d671

Remove debugging logic, and add some additional commentary.

b27b7d8

amaitland reviewed Mar 16, 2015
View reviewed changes

Further cleanups based on comments.

8bd76e1

rassilon added a commit that referenced this pull request Mar 17, 2015

Merge pull request #791 from rassilon/focus_take5

4cb60e6

Focus improvements, take 5...

rassilon merged commit 4cb60e6 into cefsharp:master Mar 17, 2015

rassilon deleted the focus_take5 branch March 17, 2015 13:57

amaitland changed the title ~~Focus improvements, take 5, a hopeful fix with lots of extra debugging g...~~ Focus improvements, take 5 Mar 17, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Focus improvements, take 5 #791

Focus improvements, take 5 #791

rassilon commented Feb 6, 2015

rassilon commented Feb 6, 2015

amaitland commented Feb 6, 2015

amaitland commented Feb 6, 2015

rassilon commented Feb 6, 2015

rassilon commented Feb 6, 2015

rassilon commented Feb 6, 2015

rassilon commented Feb 6, 2015

jornh commented Feb 6, 2015

amaitland commented Feb 10, 2015

rassilon commented Feb 10, 2015

amaitland commented Feb 11, 2015

rassilon commented Feb 11, 2015

amaitland commented Feb 12, 2015

rassilon commented Mar 3, 2015

amaitland commented Mar 5, 2015

rassilon commented Mar 5, 2015

amaitland commented Mar 5, 2015

sylvain-hamel commented Mar 6, 2015

rassilon commented Mar 6, 2015

rassilon commented Mar 6, 2015

amaitland commented Mar 6, 2015

rassilon commented Mar 6, 2015

rassilon commented Mar 6, 2015

amaitland commented Mar 9, 2015

amaitland commented Mar 12, 2015

rassilon commented Mar 12, 2015

amaitland commented Mar 12, 2015

sylvain-hamel commented Mar 13, 2015

amaitland Mar 16, 2015

rassilon Mar 17, 2015

amaitland commented Mar 17, 2015

Focus improvements, take 5 #791

Focus improvements, take 5 #791

Conversation

rassilon commented Feb 6, 2015

rassilon commented Feb 6, 2015

amaitland commented Feb 6, 2015

amaitland commented Feb 6, 2015

rassilon commented Feb 6, 2015

rassilon commented Feb 6, 2015

rassilon commented Feb 6, 2015

rassilon commented Feb 6, 2015

jornh commented Feb 6, 2015

amaitland commented Feb 10, 2015

rassilon commented Feb 10, 2015

amaitland commented Feb 11, 2015

rassilon commented Feb 11, 2015

amaitland commented Feb 12, 2015

rassilon commented Mar 3, 2015

amaitland commented Mar 5, 2015

rassilon commented Mar 5, 2015

amaitland commented Mar 5, 2015

sylvain-hamel commented Mar 6, 2015

rassilon commented Mar 6, 2015

rassilon commented Mar 6, 2015

amaitland commented Mar 6, 2015

rassilon commented Mar 6, 2015

rassilon commented Mar 6, 2015

amaitland commented Mar 9, 2015

amaitland commented Mar 12, 2015

rassilon commented Mar 12, 2015

amaitland commented Mar 12, 2015

sylvain-hamel commented Mar 13, 2015

amaitland Mar 16, 2015

Choose a reason for hiding this comment

rassilon Mar 17, 2015

Choose a reason for hiding this comment

amaitland commented Mar 17, 2015