Opened 3 years ago

Closed 8 months ago

#13841 closed defect (fixed)

OSX Cocoa: TAB navigation problem with wxComboCtrl

Reported by: fsenore Owned by: csomor
Priority: normal Milestone:
Component: wxOSX-Cocoa Version: stable-latest
Keywords: wxComboCtrl key navigation Cc: mailing@…
Blocked By: Blocking: #15008, #15008, #15008, #15008
Patch: no

Description

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.

Attachments (1)

combocmn_navig.patch download (1.3 KB) - added by johnr 20 months ago.

Download all attachments as: .zip

Change History (56)

comment:1 Changed 2 years ago by vadz

  • Status changed from new to confirmed

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

comment:2 Changed 20 months ago by johnr

This now works for me in my code with 2.9.5.

comment:3 Changed 20 months ago by vadz

  • Resolution set to fixed
  • Status changed from confirmed to closed

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

comment:4 Changed 20 months ago by fsenore

  • Cc mailing@… added
  • Resolution fixed deleted
  • Status changed from closed to reopened

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?

comment:5 Changed 20 months ago by 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.

comment:6 Changed 20 months ago by johnr

  • Keywords key navigation added

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

Changed 20 months ago by johnr

comment:7 Changed 20 months ago by 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.

comment:8 follow-up: Changed 20 months ago by csomor

  • Owner set to csomor
  • Status changed from reopened to accepted

thanks John for the patch, very much appreciated!

comment:9 Changed 20 months ago by SC

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [73517]) closes #13841

comment:10 in reply to: ↑ 8 Changed 20 months ago by 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) )

comment:11 Changed 20 months ago by 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

comment:12 follow-up: Changed 20 months ago by 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.

comment:13 in reply to: ↑ 12 Changed 20 months ago by 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.

comment:14 Changed 20 months ago by johnr

  • Blocking 15008 added

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

comment:14 Changed 20 months ago by johnr

  • Blocking 15008 removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

comment:15 Changed 16 months ago by csomor

  • Blocking 15008 added

(In #15008) really strange - MSW does bring the window to the front and calls SetForegroundWindow and Kevin O. made extra efforts to avoid the popup-window getting focus automatically

I can understand that one wants a non obtrusive popup-bubble not stealing the keyboard focus, but I cannot see why we have the two different behaviors ... we had a ticket for that #4453, so I'd opt for two different calls, perhaps reusing the ShowWithoutActivating name, this would open the way for really bringing the two wxPopupWindow::Show() implementations in line.

comment:16 Changed 16 months ago by vadz

  • Blocking

(In #15008) Sorry, I don't really understand what's going on here -- what are the different behaviours?

All I can say is that in theory ideal would be to use ShowWithoutActivating() from wxPopupTransientWindow::Popup() to avoid stealing the activation from the TLW parent but still let the controls inside the popup to handle mouse and keyboard events. But the latter is, of course, more important than the former.

comment:17 Changed 16 months ago by csomor

  • Blocking

(In #15008) forgot to link r74090

comment:18 Changed 16 months ago by SC

  • Blocking

(In #15008) (In [74091]) make sure mouse moved events are always delivered, see #15008

comment:19 Changed 16 months ago by SC

  • Blocking

(In #15008) (In [74092]) deal with Cocoa as we do with Carbon, see #15008

comment:20 Changed 16 months ago by SC

  • Blocking

(In #15008) (In [74093]) deal with Cocoa as we do with Carbon, see #15008

comment:21 Changed 16 months ago by SC

  • Blocking

(In #15008) (In [74098]) using ordinary Show for popup windows as on MSW which activates it as well, I'll rewire ShowWithoutActivating for the other use case, fixes #15008

comment:22 Changed 13 months ago by paulclinger

  • Blocking

(In #15008) Stefan, I have to reopen this as this last commit broke auto-complete/userlist functionality in wxSTC. I tested on different versions of OSX (10.7, 10.8) and the effect is exactly the same: the app crashes as soon as it tried to open an auto-complete popup, with this stack trace:

0 0x003a1efd ListBoxImpl::Select(int) + 15 (PlatWX.cpp:1100)
1 0x007ae99b AutoComplete::Show(bool) + 59 (AutoComplete.cxx:120)
2 0x007e2cc7 ScintillaBase::AutoCompleteStart(int, char const*) + 1675 (ScintillaBase.cxx:276)
3 0x007e2e91 ScintillaBase::WndProc(unsigned int, unsigned long, long) + 389 (ScintillaBase.cxx:658)
4 0x0039c91d wxStyledTextCtrl::UserListShow(int, wxString const&) + 75 (buffer.h:117)
5 0x0014eb6b wxLua_wxStyledTextCtrl_UserListShow(lua_State*) + 171

Unfortunately, I just noticed that, but the result is 100% reproducible.

I bisected it to the last commit (http://trac.wxwidgets.org/changeset/74098); I also tested reverting that commit and applying it to the current trunk and it fixes the issue.

comment:23 Changed 13 months ago by paulclinger

  • Blocking

(In #15008) The issue is also filed as a new ticket: http://trac.wxwidgets.org/ticket/15508

comment:24 Changed 12 months ago by vadz

  • Blocking

(In #15008) Stefan, should I revert r74098 or do you see a better fix? Something must be done before 3.0, crashing is worse than the original problem.

comment:25 Changed 12 months ago by vadz

  • Blocking

(In #15008) Does anybody have a simple way of reproducing it? I.e. a minimal patch to the stctest sample ideally?

From just reading the code, I really don't understand why should r74098 result in a crash...

comment:26 Changed 12 months ago by paulclinger

  • Blocking

(In #15008) Vadim, I'm not sure why that code is relevant either. The only thing comes to mind is that there may be some sizing event called. Unfortunately I have been unable to simplify the code that crashes to make it easier to test, but given that it's reported to happen in vastly different applications, I think the issue is real.

If this is of any help, I do see a window painted with a gray background and it seems to be of the right size, but there is no content and it crashes with the stack trace I showed earlier. Note that I'm using UserListShow and the other reports seem to be using CallTipShow, but I believe they both use Show method from popupwindow.

What's confusing is that the patch file is "src/osx/carbon/popupwin.cpp", but I'm using cocoa. Why would a file for "carbon" make any difference for my build?

comment:27 Changed 12 months ago by vadz

  • Blocking

(In #15008) Replying to paulclinger:

What's confusing is that the patch file is "src/osx/carbon/popupwin.cpp", but I'm using cocoa. Why would a file for "carbon" make any difference for my build?

Cocoa port reuses some files under "carbon" for historical reasons, so this is not a mystery on its own. The rest still is however...

comment:28 Changed 12 months ago by paulclinger

  • Blocking

(In #15008) Vadim: just a brief update on this. I tried to figure out what in wxPopupWindow::Show(bool show) method "fixes" the issue. If I have an empty method (just "return true" as the body), there is no crash (but obviously there is no popup either). If I add "ShowWithoutActivating()" call (as in the changeset: http://trac.wxwidgets.org/changeset/74098), then there is a popup and no crash.

comment:29 Changed 12 months ago by vadz

  • Blocking

(In #15008) Unfortunately I still don't understand what's going on here and without being able to reproduce and, hence, debug it I just can't do much (other than hoping that Stefan looks at it...)

comment:30 Changed 12 months ago by csomor

  • Blocking

(In #15008) I hope I'll find the time to look at it

comment:31 Changed 11 months ago by johnr

  • Blocking

(In #15008) This is also a problem for me with a custom combo autocomplete routine. Mouse over and mouse scroll events all work properly but when the popup shows then keyboard events that would normally be handled by the textbox component do not work. I have not had time to look into it yet or see if I can work around it.

comment:32 Changed 11 months ago by johnr

  • Blocking

(In #15008) I also see a crash if I click on another program's window while the popup is shown (and yes the popup shows on top of the other program's window). It crashes in wxWindowBase::GetParent() in window.h where this = NULL.

comment:33 Changed 11 months ago by johnr

  • Blocking

(In #15008) Replying to johnr:

It crashes in wxWindowBase::GetParent() in window.h where this = NULL.

That was from wxComboFrameEventHandler::OnIdle(...) that I had copied for a derived combo popup class. The original did not check for a NULL winFocused:

wxWindow* winFocused = ::wxWindow::FindFocus();
if( ... && winFocused->GetParent() != ...

I see this has been fixed in combocmn.cpp revision 73809 with 
if ( !winFocused || (

comment:34 Changed 11 months ago by johnr

  • Blocking

(In #15008) Replying to johnr:

Mouse over and mouse scroll events all work properly but when the popup shows then keyboard events that would normally be handled by the textbox component do not work.

The popup takes focus and hence the key strokes. I can work around this but the textbox used as part of my composite control loses focus and so generates onlostfocus events. I can work around that as well but I lose the highlighting of the textbox's text selection as the user types. I am probably missing something here in my own code but it works fine in MSW.

comment:35 Changed 11 months ago by johnr

  • Blocking

(In #15008) Replying to johnr:

The popup takes focus and hence the key strokes...

I went back to using pre revision 74098 ShowPopup via ShowWithoutActivating()

comment:36 Changed 11 months ago by arondobos

  • Blocking

(In #15008) Here's a short test program that crashes consistently on OSX-cocoa when I type '(' into the editor. I have a frame with a wxSTC trying to show a calltip. Works fine on Win32/x64, and linux x64. I've compiled wxWidgets 3.0.0-rc2 with these flags: ./configure --prefix=/Users/adobos/local/wx3-svn --enable-stl=yes --enable-debug=no --enable-shared=no --with-cocoa --with-libjpeg=builtin --with-libpng=builtin --with-regex=builtin --with-libtiff=builtin --with-zlib=builtin --with-expat=builtin

I don't know enough about Cocoa/OSX to help resolve it, but I really hope it can be fixed for 3.0.0 final!

comment:37 Changed 11 months ago by arondobos

  • Blocking

(In #15008) Sorry I left this out: the OSX stack trace is below.


Exception Type: EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000010

VM Regions Near 0x10:
-->

TEXT 0000000103e81000-000000010433d000 [ 4848K] r-x/rwx SM=COW /Users/USER/*/stctest.app/Contents/MacOS/stctest

Application Specific Information:
objc_msgSend() selector name: becomeKeyWindow

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0 libobjc.A.dylib 0x00007fff8a1dc250 objc_msgSend + 16
1 com.apple.AppKit 0x00007fff8c7ccf04 -[NSWindow _changeKeyAndMainLimitedOK:] + 719
2 com.apple.AppKit 0x00007fff8c7cc76b -[NSWindow _makeKeyRegardlessOfVisibility] + 104
3 com.apple.AppKit 0x00007fff8c7cc6c5 -[NSWindow makeKeyAndOrderFront:] + 25
4 National Renewable Energy Laboratory 0x0000000103f03d99 wxNonOwnedWindowCocoaImpl::Show(bool) + 195
5 National Renewable Energy Laboratory 0x0000000103e929b3 wxNonOwnedWindow::Show(bool) + 51
6 National Renewable Energy Laboratory 0x0000000104177956 ScintillaBase::CallTipShow(Point, char const*) + 578
7 National Renewable Energy Laboratory 0x0000000104178b01 ScintillaBase::WndProc(unsigned int, unsigned long, long) + 1067
8 National Renewable Energy Laboratory 0x000000010409fe22 wxStyledTextCtrl::CallTipShow(int, wxString const&) + 62
9 National Renewable Energy Laboratory 0x0000000103e85661 MyEditor::OnCharAdded(wxStyledTextEvent&) + 181 (stctest.cpp:21)

comment:38 Changed 11 months ago by arondobos

  • Blocking

comment:39 Changed 10 months ago by neurodroid

  • Blocking

(In #15008) Is there any way to temporarily work around this issue? It makes my app unusable at this point.

comment:40 Changed 10 months ago by neurodroid

  • Blocking

comment:41 Changed 10 months ago by jguzman

  • Blocking

comment:42 Changed 10 months ago by johnr

  • Blocking

(In #15008) Replying to neurodroid:

Is there any way to temporarily work around this issue? It makes my app unusable at this point.

Revert http://trac.wxwidgets.org/changeset/74098

comment:43 Changed 10 months ago by johnr

  • Blocking

(In #15008) Stefan, perhaps you have better ideas for this ticket but for me using Show rather than ShowWithoutActivating() makes the popupwin take focus which I think causes the problems reported above and for me also when using wxRichtextToolTips from mouseover routines.

For comboctrl popups, the combination of ShowWithoutActivating() as it was pre revision 74098 followed by Raise() as I have in the combocmn_fix.patch above works well at least for me.

comment:44 Changed 10 months ago by csomor

  • Blocking

(In #15008) Thanks John, I have this ticket placed prominently in my inbox, I know I have to fix this soon, but I really need some hours to dive into all the issues which lead to the change in the first place, I fear breaking other things otherwise ...

comment:45 Changed 10 months ago by jguzman

  • Blocking

(In #15008) I also have this ticket under "high priority" for my application. I would be so happy to have this ticket solved! Thanks!

comment:47 Changed 10 months ago by SC

(In [75401]) reverting r74098, applying John's version, see #15008, #15765, #13841 will be backported if no regressions are discovered

comment:48 Changed 8 months ago by johnr

  • Blocking

(In #15008) This now works with the exception of comboctrls hosted in modal dialogs. I think it is a problem with the modal loop which causes problems in a few other areas. I note the modal loop still consumes 100% CPU usage when running.

comment:49 Changed 8 months ago by vadz

  • Blocking

(In #15008) On topic of 100% CPU consumption, see http://review.bakefile.org/r/558 -- perhaps you have any comments/ideas about this?

comment:50 Changed 8 months ago by johnr

  • Blocking

(In #15008) Replying to vadz:

On topic of 100% CPU consumption, see http://review.bakefile.org/r/558 -- perhaps you have any comments/ideas about this?

I have been away for a couple of weeks and see that has been applied in r75779. Unfortunately it made no difference to the comboctrl's popup events when hosted in a modal dialog. Seems similar to the non-responsive html help frame displayed from a modal dialog.

With regard to your question I did some tests with the password dialog on the timeout figure in
return DispatchTimeout( m_isInsideYield ? 0 : 1000 );

Figures are approximate, machine dependent and use 1 core of 4.
timeout cpu%
1000 1.8 to 2
100 2.1 to 2.8
10 4 to 4.2
1 20.5
0 100

The modal loop differs from MSW in that mouse hover events still occur in a parent frame hosting a modal dialog. When I swipe the mouse across a ribbon where I show wxRichToolTips then there is some latency there now when the dialog begins and ends its ShowModal state and several tooltips may show simultaneously instead of not showing at all with a brief mouse over. I suppose the main event loop system continues but is slowed by the 1000 figure. I have adjusted my copy to use 10 to avoid this and accept 4% of cpu time until I see another fix.

comment:51 Changed 8 months ago by johnr

  • Blocking

AFAICS this ticket is fixed.

comment:52 Changed 8 months ago by vadz

  • Resolution set to fixed
  • Status changed from reopened to closed

Thanks for your testing!

comment:53 Changed 7 months ago by johnr

  • Blocking

(In #15008) I had a quick look at this today. When the comboctrl/popup is hosted in a model dialog then the NSApp doesn't receive any mouse events from the OS when the mouse is inside the popup boundaries.

comment:54 Changed 7 months ago by vadz

  • Blocking

(In #15008) I am not sure what is/are the remaining problem(s) here and how can they be reproduced exactly, could someone please clarify? Or maybe close this ticket and open a new one as a lot of things have changed apparently since it was opened.

In any case, this doesn't look like something we can fix in time for 3.0.1 unfortunately.

comment:55 Changed 7 months ago by johnr

  • Blocking

(In #15008) Replying to vadz:

I am not sure what is/are the remaining problem(s) here and how can they be reproduced exactly, could someone please clarify? Or maybe close this ticket and open a new one as a lot of things have changed apparently since it was opened.

The original problem in wxComboCtrl's popup window hosted in a non-modal parent is fixed but remains for modal parents. The latter situation has a different cause and I will open a new ticket for it next time I work with OSX.

AFAIK adverse effects with wxSTC caused by one of the above patches are resolved. I have closed the ticket.

Note: See TracTickets for help on using tickets.