tab navigation doesn't wrap around back to a wxNotebook like is standard behavior (Issue #25443)
Quin Gillespie
trypsynth created an issue (wxWidgets/wxWidgets#25443)Description
Bug description:
When putting some controls on a panel for a notebook and then trying to tab through them, my focus gets stuck on the last control, and I'm unable to press tab again to wrap back around to the notebook, like is standard win32 behavior.
Expected vs observed behaviour:
The tab navigation should work normally, like it would in any other Windows app. If I press tab from the second button, it should wrap back around and put my focus on the notebook.
Patch or snippet allowing to reproduce the problem:
#include <wx/wx.h>
#include <wx/notebook.h>
class MyApp : public wxApp {
public:
bool OnInit() override {
auto* frame = new wxFrame(nullptr, wxID_ANY, "Notebook", wxDefaultPosition, wxSize(300, 200));
auto* notebook = new wxNotebook(frame, wxID_ANY);
auto* panel = new wxPanel(notebook, wxID_ANY);
auto* sizer = new wxBoxSizer(wxVERTICAL);
sizer->Add(new wxButton(panel, wxID_ANY, "Button 1"), 0, wxALL | wxEXPAND, 5);
sizer->Add(new wxButton(panel, wxID_ANY, "Button 2"), 0, wxALL | wxEXPAND, 5);
panel->SetSizer(sizer);
notebook->AddPage(panel, "Tab");
frame->Show();
return true;
}
};
wxIMPLEMENT_APP(MyApp);
To Reproduce:
- Run NVDA Or Narrator.
- Run the provided sample.
- Tab around.
Platform and version information
- wxWidgets version you use: latest from master
- wxWidgets port you use: wxMsw
- OS and its version: Windows 10 21H2 (AMD64) build 19044.5854
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Quin Gillespie
trypsynth left a comment (wxWidgets/wxWidgets#25443)Can confirm it also happens with wxPython:
import wx
class MyApp(wx.App):
def OnInit(self):
frame = wx.Frame(None, title="Notebook", size=(300, 200))
notebook = wx.Notebook(frame)
panel = wx.Panel(notebook)
sizer = wx.BoxSizer(wx.VERTICAL)
sizer.Add(wx.Button(panel, label="Button 1"), 0, wx.ALL | wx.EXPAND, 5)
sizer.Add(wx.Button(panel, label="Button 2"), 0, wx.ALL | wx.EXPAND, 5)
panel.SetSizer(sizer)
notebook.AddPage(panel, "Tab")
frame.Show()
return True
if __name__ == "__main__":
app = MyApp(False)
app.MainLoop()
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Quin Gillespie
trypsynth left a comment (wxWidgets/wxWidgets#25443)Found this in a [url=https://github.com/accessibleapps/gui_builder]wxPython wrapper[/url]. I don't know much about this codebase, but perhaps something like this would work?
# Fix: ReadOnly TextCtrl's fail to appear in tab order.
self.control.AcceptsFocusFromKeyboard = lambda: True
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Quin Gillespie
trypsynth left a comment (wxWidgets/wxWidgets#25443)Seemingly this was reported 15 years ago, and it's still not fixed?
I truly hope it's just because it slipped under the radar and people forgot about it, because this makes certain types of wxWidgets apps incredibly hard to use with a screen reader, and from my standpoint currently it looks like the devs simply don't care. I'll happily be proven wrong, though.
#11909
A [url=https://github.com/accessibleapps/gui_builder]wxPython wrapper[/url] has a workaround for this, is it possible for something like this to be done in wxWidgets core?
[code]
# Now, we shall have much hackyness to work around WX bug 11909
if not list(self.field.get_all_children()):
return
def on_focus(evt):
evt.Skip()
last_child = item.field.get_last_enabled_descendant()
if (
last_child is not None
and evt.GetWindow() == last_child.widget.get_control()
):
last_child._was_focused = True
def on_navigation_key(evt):
last_child = item.field.get_last_enabled_descendant()
if last_child is None:
return
if evt.GetDirection() and getattr(last_child, "_was_focused", False):
self.set_focus()
else:
evt.Skip()
last_child._was_focused = False
first_child = item.field.get_first_child()
last_child = item.field.get_last_child()
item.bind_event(wx.EVT_CHILD_FOCUS, on_focus)
item.bind_event(wx.EVT_NAVIGATION_KEY, on_navigation_key)
[/code]
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25443)Seemingly this was reported 15 years ago, and it's still not fixed? I truly hope it's just because it slipped under the radar and people forgot about it, because this makes certain types of wxWidgets apps incredibly hard to use with a screen reader, and from my standpoint currently it looks like the devs simply don't care.
This didn't seem like a high priority bug to me, true. I thought using Shift-Tab was a simple enough workaround, but I didn't realize it has implications for screen readers.
Unfortunately even if this is more important than I thought it doesn't mean I (or somebody else) somehow magically gets more time/energy to work on it. There are a lot of other things needing attention too and I can spend only so much time on wxWidgets and other open source projects. Luckily, anybody else can contribute fixes for the problems they care about too and they would be very welcome.
A wxPython wrapper (https://github.com/accessibleapps/gui_builder) has a workaround for this, is it possible for something like this to be done in wxWidgets core?
I can't really say. If anybody can propose a PR fixing this based on this code, I'd be glad to apply it, of course.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
MaartenBent left a comment (wxWidgets/wxWidgets#25443)I didn't read/understand the exact reasoning in wxNotebook::OnNavigationKey(), but it is caused by this code:
https://github.com/wxWidgets/wxWidgets/blob/04310f42bd5102d64423c75d219e5539039a90a9/src/msw/notebook.cpp#L1656-L1664
And can be fixed by replacing it with just:
SetFocus();It was originally added in d9506e7 and b8bdaa7
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25443)FWIW we do want to allow TAB-bing out of the notebook if there are more controls after it. But, in theory, if there are no other controls in the panel, it should wrap around and the focus should get back to the notebook itself. I didn't have time to check why this doesn't happen.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
MaartenBent left a comment (wxWidgets/wxWidgets#25443)I tested some more, with an extra button below the notebook.
Using the current code, tabbing does not work when both wxNotebook and wxButton are direct children of the wxFrame. But it does work when the controls are children of a wxPanel and that is placed in the frame.
Extended example that does seem to work:
#include <wx/wx.h> #include <wx/notebook.h> class MyApp : public wxApp { public: bool OnInit() override
{
auto* frame = new wxFrame(nullptr, wxID_ANY, "Notebook", wxDefaultPosition, wxSize(600, 400));
auto* panel = new wxPanel(frame, wxID_ANY);
auto* notebook = new wxNotebook(panel, wxID_ANY);
auto* panel1 = new wxPanel(notebook, wxID_ANY);
auto* panel2 = new wxPanel(notebook, wxID_ANY);
auto* sizer1 = new wxBoxSizer(wxVERTICAL);
sizer1->Add(new wxButton(panel1, wxID_ANY, "Button 1a"), wxSizerFlags(0).Expand().Border());
sizer1->Add(new wxButton(panel1, wxID_ANY, "Button 2a"), wxSizerFlags(0).Expand().Border());
auto* sizer2 = new wxBoxSizer(wxVERTICAL);
sizer2->Add(new wxButton(panel2, wxID_ANY, "Button 1b"), wxSizerFlags(0).Expand().Border());
sizer2->Add(new wxButton(panel2, wxID_ANY, "Button 2b"), wxSizerFlags(0).Expand().Border());
panel1->SetSizer(sizer1);
panel2->SetSizer(sizer2);
notebook->AddPage(panel1, "TabA");
notebook->AddPage(panel2, "TabB");
auto* sizer = new wxBoxSizer
(wxVERTICAL);
sizer->Add(notebook, wxSizerFlags(1).Expand());
sizer->Add(new wxButton(panel, wxID_ANY, "Button"), wxSizerFlags(0).Expand().Border());
panel->SetSizer(sizer);
frame->Show();
return true;
}
};
wxIMPLEMENT_APP(MyApp);
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25443)Oh, right, for what I described to work, parent must handle TAB navigation too, but wxFrame doesn't do this.
So I think this issue (as well as the other one probably) is just invalid — and easy to work around by creating an extra panel.
OTOH there could be a legitimate argument for making wxFrame a wxNavigationEnabled<wxTopLevelWindow> too because we probably don't care about the extra overhead (this was questionable 30 years ago and seems like a ridiculous thing to worry about now) and people clearly expect this to work.
Anyhow, I think this can be closed, right?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
MaartenBent left a comment (wxWidgets/wxWidgets#25443)Anyhow, I think this can be closed, right?
Not yet, I found one more issue. You could apply the fix if you agree with it.
When I disable the extra button outside the notebook (so notebook is only child of the panel), I can't tab forward to the notebook anymore.
This can be fixed with the following patch (which would also fix the initially reported problem):
diff --git a/src/msw/notebook.cpp b/src/msw/notebook.cpp index 53292c3e083..3bda3da4121 100644 --- a/src/msw/notebook.cpp +++ b/src/msw/notebook.cpp @@ -1660,7 +1660,12 @@ void wxNotebook::OnNavigationKey(wxNavigationKeyEvent& event) else if ( parent ) { event.SetCurrentFocus(this); - parent->HandleWindowEvent(event); + if ( !parent->HandleWindowEvent(event) ) + { + // Not handled, set focus to the notebook. + event.Skip(false); + SetFocus(); + } } } }
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25443)This looks correct, thanks! I'll apply this to master and backport to 3.2 too.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Quin Gillespie
Closed #25443 as completed.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Quin Gillespie
trypsynth left a comment (wxWidgets/wxWidgets#25443)Can confirm the fix, thanks so much!
Sorry if my wording came off as slightly harsh, that wasn't my intent and I definitely understand it not seeming like a high priority bug to sighted users. Being a keyboard only user can definitely give you a different perception. In any case, thanks for your quick fix!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Quin Gillespie
trypsynth left a comment (wxWidgets/wxWidgets#25443)Hi,
Reopening this, as this behavior is still slightly buggy. Sorry if it would've been better to open a new issue for this, but the exact same sample code works to reproduce the issue using the latest wxWidgets 3.31. Try tabbing backwards from the notebook. I'd expect it to land me on the last tabbable item of the panel, but it does nothing.
To be clear: the original issue is fixed, but there's an edge case that wasn't accounted for.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Quin Gillespie
Reopened #25443.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
MaartenBent left a comment (wxWidgets/wxWidgets#25443)Backward tabbing is indeed handled separately. We can add something similar there, and handle the backwards tab on the page when the main notebook doesn't handle it:
diff --git "a/src/msw/notebook.cpp" "b/src/msw/notebook.cpp" index bcbfea8e030..8ee167bc417 100644 --- "a/src/msw/notebook.cpp" +++ "b/src/msw/notebook.cpp" @@ -1639,7 +1639,11 @@ void wxNotebook::OnNavigationKey(wxNavigationKeyEvent& event) // focus is currently on notebook tab and should leave // it backwards (Shift-TAB) event.SetCurrentFocus(this);
- parent->HandleWindowEvent(event); + if ( !parent->HandleWindowEvent(event) ) + {
+ wxWindow* page = m_pages[m_selection]; + page->HandleWindowEvent(event); + } } else if ( isFromParent || isFromSelf ) {
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Quin Gillespie
trypsynth left a comment (wxWidgets/wxWidgets#25443)Wow Martin, you're a legend! Hopefully that'll be merged soon.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25443)Thanks Maarten, will push soon (and backport later).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
MaartenBent left a comment (wxWidgets/wxWidgets#25443)I didn't add a check for m_selection != wxNOT_FOUND. It is used in the code below, so probably needed here as well.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Closed #25443 as completed via 329162e.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.