OSX Cocoa: TAB navigation problem with wxComboCtrl

[fsenore]

The problem can be seen with the combo sample.
When the sample starts the log windows has the focus. If you press TAB the log windows loses the focus but no other control seems to receive it. If you press TAB 8 times the log window gets the focus again. That is exactly the number of controls in the window, so it looks like the focus moves from control to control, but the combos do not act like they received it: if you press some keys nothing gets written.
If you click a control you can type as usual, but if you press TAB the focus seems to be lost again.

[vadz]

I can confirm the problem (even with full keyboard navigation on, i.e. after pressing Ctrl-F7) but I have no idea about how to fix it. I'm not even sure if Cocoa can be convinced to give keyboard focus to custom wxWindows at all...

[johnr]

This now works for me in my code with 2.9.5.

[vadz]

Interesting... I wonder what fixed it. But thanks for testing!

[fsenore]

I have checked with the last svn trunk, in both the Carbon and the Cocoa port, but the problem has not been fixed. In the combo sample I still cannot move from one control to the other using TAB.

I am testing under OS X 10.5: might it be a problem with this version?

[johnr]

No it is still a problem with all versions but I had forgotten I have a work around in place. I have got a few scattered around and should probably not rely on memory.

[johnr]

Can you try the attached combocmn_navig.patch and report back?

[fsenore]

I tested the patch under both Carbon and Cocoa and it works!
Now it is possible to use a wxComboCtrl in data entry intensive programs.
Thank you very much for your good work.

[csomor]

thanks John for the patch, very much appreciated!

[SC]

(In [73517]) closes #13841

[johnr]

Replying to csomor:

thanks John for the patch, very much appreciated!

Actually I had another patch as well that I was just posting and had to refresh the page to do so. I wasn't sure which was best. I post it below anyway.

It needs no #ifdef WXMAC statement and doesn't rely on idle events. I have had no problems with either solution but I thought the idle solution might be safer.

Index: src/common/combocmn.cpp
===================================================================
--- src/common/combocmn.cpp	(revision 73495)
+++ src/common/combocmn.cpp	(working copy)
@@ -2050,21 +2050,20 @@
 
 
 void wxComboCtrlBase::OnFocusEvent( wxFocusEvent& event )
 {
-// On Mac, this leads to infinite recursion and eventually a crash
-#ifdef __WXMAC__
-    wxUnusedVar(event);
-#else
+    // On Mac, Setting focus to the textctrl here leads to infinite recursion
+    // and eventually a crash unless we prevent recursion using m_resetFocus.
+
     if ( event.GetEventType() == wxEVT_SET_FOCUS )
     {
-        wxWindow* tc = GetTextCtrl();
-        if ( tc && tc != DoFindFocus() )
+        if ( !m_resetFocus && GetTextCtrl() && !GetTextCtrl()->HasFocus() )
         {
-            tc->SetFocus();
+            m_resetFocus = true;
+            GetTextCtrl()->SetFocus();
+            m_resetFocus = false;
         }
     }
 
     Refresh();
-#endif
 }
 
 void wxComboCtrlBase::OnIdleEvent( wxIdleEvent& WXUNUSED(event) )

[csomor]

yes, the idle version might be safer, but it might be a problem if we have code that assumes that after being executed the focus is already at the right place, I'll think about it, perhaps - paranoid of side effects as I am - I'll use this, but have it in an ifdef all the same

[johnr]

Understood. I was concerned that user code might get 2 focus events and so did initially try wxEventBlocker but it wasn't robust in some situations.

[johnr]

Replying to johnr:

I was concerned that user code might get 2 focus events...

In fact that is what happens now in WXMAC and has always happened in WXMSW. The second event can be blocked in wxComboCtrlBase::OnIdleEvent using wxEventBlocker for WXMAC but then we would have to do the same for WXMSW in wxComboCtrlBase::OnFocusEvent. I suppose some might like to check whether the event object is the wxComboCtrl or the wxTextCtrl for both events. Might be best left alone.

[johnr]

(In #15008) I have added combocmn_fix.patch that fixes problems with contained control's inactive scrollbars, contained control selection highlights, inaccurate hit-tests and the failure to close noted in ticket #15036.

These problems can be seen in the combo sample's ComboCtrlListViewWithAnimation control when the patch combo_lview.patch is applied to the combo sample. This disables the call to Raise and ShowWithEffect. There is also an ongotfocus counter that is reset by opening the popup that sends to the debug output if it works (absent in msw).

The fix is trivial involving Raise and Show but ideally we would change wxPopupWindow Show() implementation but I am reluctant to dive in there for fear of upsetting its other usages. Changing defines TRANSIENT_POPUPWIN_IS_PERFECT and POPUPWIN_IS_PERFECT for WXMAC as per WXMSW result in a fix but use a frame or dialog along with a different event handler. That option didn't look that desirable.

Both Raise and Show are necessary otherwise we still get one or more of the problems listed above.
Show() alone
1. No selection highlight until scrollbar clicked
2. hit-test is inaccurate until scrollbar clicked
3. popup doesn't close with clicks on other apps unlesss scrollbar clicked first

Raise() alone
1. scrollbar works
2. hit-test is inaccurate and not working outside the parent win boundary
3. popup closes with clicks on other apps

I have re-opened #13841 because the setfocus via onidle interferes with combopopup button click opening when this patch is applied. See my comment there.

I have tested this fairly extensively and will continue to do so but it would be useful if someone else could try it.

[johnr]

I was influenced by the existing OnIdle code when I did the original patch.
When the fix from #15008 is applied, setting focus via the OnIdle event interferes with button click popup opening where there are mutiple comboctrls in a window. The first combo in the parent window will open its popup well but not the others, probably because of the delay in iterating all the window's children and getting idle events to them.

The alternative patch I posted above works flawlessly in combination with the #15008 patch.