From 23ddf26571eafc96c3c46d706005fbb6ae3600c7 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 11 Mar 2019 13:43:01 +0100 Subject: [PATCH] Fix bug with wxRadioButton state changing unexpectedly in wxMSW In wxMSW, a focused wxRadioButton is always checked, which meant that checking a wxRadioButton while focus was not in the window containing it and later giving the focus to that window could uncheck it by giving focus to another wxRadioButton that had had it previously. Fix this by adding WXSetPendingFocus() to wxMSW wxWindow and calling it from wxRadioButton::SetValue() to ensure that when the focus is regained, it goes to the newly checked radio button and not some other one. This replaces the previously used, for the same purpose, wxMSW-specific wxTopLevelWindow::SetLastFocus(), so while this solution is not exactly pretty, it's not worse than we had before, while being more generic. Also add a unit test checking that things work correctly in the scenario described above. Closes https://github.com/wxWidgets/wxWidgets/pull/1257 Closes #18341. --- include/wx/containr.h | 6 ++++ include/wx/msw/toplevel.h | 9 +++-- include/wx/msw/window.h | 8 +++++ src/msw/radiobut.cpp | 32 ++++++++--------- src/msw/window.cpp | 11 ++++++ tests/controls/radiobuttontest.cpp | 57 ++++++++++++++++++++++++++++++ 6 files changed, 103 insertions(+), 20 deletions(-) diff --git a/include/wx/containr.h b/include/wx/containr.h index 4c0d9d5de8..c9eb5857c5 100644 --- a/include/wx/containr.h +++ b/include/wx/containr.h @@ -266,6 +266,12 @@ public: { return m_container.HasTransparentBackground(); } + + WXDLLIMPEXP_INLINE_CORE + virtual void WXDoUpdatePendingFocus(wxWindow* win) wxOVERRIDE + { + return m_container.SetLastFocus(win); + } #endif // __WXMSW__ protected: diff --git a/include/wx/msw/toplevel.h b/include/wx/msw/toplevel.h index 8e79013a5e..0256104ebc 100644 --- a/include/wx/msw/toplevel.h +++ b/include/wx/msw/toplevel.h @@ -100,9 +100,12 @@ public: // event handlers void OnActivate(wxActivateEvent& event); - // called by wxWindow whenever it gets focus - void SetLastFocus(wxWindow *win) { m_winLastFocused = win; } - wxWindow *GetLastFocus() const { return m_winLastFocused; } + // called from wxWidgets code itself only when the pending focus, i.e. the + // element which should get focus when this TLW is activated again, changes + virtual void WXDoUpdatePendingFocus(wxWindow* win) wxOVERRIDE + { + m_winLastFocused = win; + } // translate wxWidgets flags to Windows ones virtual WXDWORD MSWGetStyle(long flags, WXDWORD *exstyle) const wxOVERRIDE; diff --git a/include/wx/msw/window.h b/include/wx/msw/window.h index dd8741dcd7..35df830c3f 100644 --- a/include/wx/msw/window.h +++ b/include/wx/msw/window.h @@ -576,6 +576,14 @@ public: // Return true if the button was clicked, false otherwise. static bool MSWClickButtonIfPossible(wxButton* btn); + // This method is used for handling wxRadioButton-related complications, + // see wxRadioButton::SetValue(). It calls WXDoUpdatePendingFocus() for + // this window and all its parents up to the enclosing TLW, recursively. + void WXSetPendingFocus(wxWindow* win); + + // Should be overridden by all classes storing the "last focused" window. + virtual void WXDoUpdatePendingFocus(wxWindow* WXUNUSED(win)) {} + protected: // this allows you to implement standard control borders without // repeating the code in different classes that are not derived from diff --git a/src/msw/radiobut.cpp b/src/msw/radiobut.cpp index 00c3fa93b4..84a16ddbfa 100644 --- a/src/msw/radiobut.cpp +++ b/src/msw/radiobut.cpp @@ -103,24 +103,15 @@ void wxRadioButton::SetValue(bool value) // reselected automatically, if a parent window loses the focus and regains // it. wxWindow * const focus = FindFocus(); - wxTopLevelWindow * const - tlw = wxDynamicCast(wxGetTopLevelParent(this), wxTopLevelWindow); - wxCHECK_RET( tlw, wxT("radio button outside of TLW?") ); - wxWindow * const focusInTLW = tlw->GetLastFocus(); const wxWindowList& siblings = GetParent()->GetChildren(); wxWindowList::compatibility_iterator nodeThis = siblings.Find(this); wxCHECK_RET( nodeThis, wxT("radio button not a child of its parent?") ); - // this will be set to true in the code below if the focus is in our TLW - // and belongs to one of the other buttons in the same group + // this will be set to true in the code below if the focus belongs to one + // of the other buttons in the same group bool shouldSetFocus = false; - // this will be set to true if the focus is outside of our TLW currently - // but the remembered focus of this TLW is one of the other buttons in the - // same group - bool shouldSetTLWFocus = false; - // if it's not the first item of the group ... if ( !HasFlag(wxRB_GROUP) ) { @@ -146,8 +137,6 @@ void wxRadioButton::SetValue(bool value) if ( btn == focus ) shouldSetFocus = true; - else if ( btn == focusInTLW ) - shouldSetTLWFocus = true; btn->SetValue(false); @@ -179,16 +168,25 @@ void wxRadioButton::SetValue(bool value) if ( btn == focus ) shouldSetFocus = true; - else if ( btn == focusInTLW ) - shouldSetTLWFocus = true; btn->SetValue(false); } if ( shouldSetFocus ) + { + // Change focus immediately, we can't do anything else in this case as + // leaving it to the other radio button would put it in an impossible + // state: a radio button can't have focus and be unchecked. SetFocus(); - else if ( shouldSetTLWFocus ) - tlw->SetLastFocus(this); + } + else + { + // Don't change focus right now, this could be unexpected, but do + // ensure that when our parent regains focus, it goes to this button + // and not another one, which would result in this one losing its + // checked status. + GetParent()->WXSetPendingFocus(this); + } } bool wxRadioButton::GetValue() const diff --git a/src/msw/window.cpp b/src/msw/window.cpp index 8ef8e639e3..9b717bb3d9 100644 --- a/src/msw/window.cpp +++ b/src/msw/window.cpp @@ -1026,6 +1026,17 @@ void wxWindowMSW::MSWUpdateUIState(int action, int state) ::SendMessage(GetHwnd(), WM_CHANGEUISTATE, MAKEWPARAM(action, state), 0); } +void wxWindowMSW::WXSetPendingFocus(wxWindow* win) +{ + for ( wxWindow* parent = this; parent; parent = parent->GetParent() ) + { + parent->WXDoUpdatePendingFocus(win); + + if ( parent->IsTopLevel() ) + break; + } +} + // --------------------------------------------------------------------------- // scrolling stuff // --------------------------------------------------------------------------- diff --git a/tests/controls/radiobuttontest.cpp b/tests/controls/radiobuttontest.cpp index df50b969f5..535cc17e25 100644 --- a/tests/controls/radiobuttontest.cpp +++ b/tests/controls/radiobuttontest.cpp @@ -16,7 +16,10 @@ #ifndef WX_PRECOMP #include "wx/app.h" + #include "wx/button.h" + #include "wx/panel.h" #include "wx/radiobut.h" + #include "wx/sizer.h" #include "wx/stattext.h" #endif // WX_PRECOMP @@ -195,4 +198,58 @@ void RadioButtonTestCase::Single() CHECK(ngradio->GetValue()); } +TEST_CASE("wxRadioButton::Focus", "[radiobutton][focus]") +{ + // Create a container panel just to be able to destroy all the windows + // created here at once by simply destroying it. + wxWindow* const tlw = wxTheApp->GetTopWindow(); + wxScopedPtr parentPanel(new wxPanel(tlw)); + + // Create a panel containing 2 radio buttons and another control outside + // this panel, so that we could give focus to something different and then + // return it back to the panel. + wxPanel* const radioPanel = new wxPanel(parentPanel.get()); + wxRadioButton* const radio1 = new wxRadioButton(radioPanel, wxID_ANY, "1"); + wxRadioButton* const radio2 = new wxRadioButton(radioPanel, wxID_ANY, "2"); + wxSizer* const radioSizer = new wxBoxSizer(wxHORIZONTAL); + radioSizer->Add(radio1); + radioSizer->Add(radio2); + radioPanel->SetSizer(radioSizer); + + wxButton* const dummyButton = new wxButton(parentPanel.get(), wxID_OK); + + wxSizer* const sizer = new wxBoxSizer(wxVERTICAL); + sizer->Add(radioPanel, wxSizerFlags(1).Expand()); + sizer->Add(dummyButton, wxSizerFlags().Expand()); + parentPanel->SetSizer(sizer); + + parentPanel->SetSize(tlw->GetClientSize()); + parentPanel->Layout(); + + // Initially the first radio button should be checked. + radio1->SetFocus(); + CHECK(radio1->GetValue()); + CHECK(wxWindow::FindFocus() == radio1); + + // Switching focus from it shouldn't change this. + dummyButton->SetFocus(); + CHECK(radio1->GetValue()); + + // Checking another radio button should make it checked and uncheck the + // first one. + radio2->SetValue(true); + CHECK(!radio1->GetValue()); + CHECK(radio2->GetValue()); + + // While not changing focus. + CHECK(wxWindow::FindFocus() == dummyButton); + + // And giving the focus to the panel shouldn't change radio button + // selection. + radioPanel->SetFocus(); + CHECK(wxWindow::FindFocus() == radio2); + CHECK(!radio1->GetValue()); + CHECK(radio2->GetValue()); +} + #endif //wxUSE_RADIOBTN