Fix wxGenericCollapsiblePane focus and duplicate accessible name (PR #27049)
Quin Gillespie
Two problems with wxGenericCollapsiblePane for keyboard and screen reader users under MSW:
- The pane window used for the contents accepted focus itself when it had no focusable children, for example when it only contains a wxStaticText as in the generic about dialog. Tab then moved focus to an empty panel, and screen readers only announced its internal name. The panel now never takes focus itself. Focusable children still get focus as before.
- The pane returned the label of its header from GetLabel(), so both the pane and the header had the same accessible name and screen readers read it twice. The pane now has an empty accessible name.
Tested with NVDA on Windows 11.
This is independent of #27048 but was found while testing the same dialog.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/27049
Commit Summary
- 3ef89b1 fix wxGenericCollapsiblePane focus and duplicate accessible name
File Changes
(2 files)
- M include/wx/generic/collpaneg.h (4)
- M src/generic/collpaneg.cpp (52)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/27049.patch
- https://github.com/wxWidgets/wxWidgets/pull/27049.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Quin Gillespie
@trypsynth pushed 1 commit.
- fa7a23b fix wxGenericCollapsiblePane contents panel taking focus
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#27049)Maybe we should just refuse focus by default for a window with some children that all refuse focus? I can't think of any realistic scenario in which focus would be useful in this case. I.e. typically either the window handles keyboard input and then it doesn't have any children or it does have children and then some of them may or not handle keyboard input, but the window doesn't need it itself.
I think a more general fix could solve the same problem in other places too.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Quin Gillespie
@trypsynth pushed 1 commit.
- 7560f90 fix containers with only non-focusable children accepting focus
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Quin Gillespie
trypsynth left a comment (wxWidgets/wxWidgets#27049)Thanks, that's a better fix. I've replaced the collapsible pane specific change with it: wxControlContainerBase::AcceptsFocus() now returns false for a window which has children, but none of them can accept focus. Windows without children still accept focus as before. I also changed UpdateParentCanFocus() to use the same rule, so that the native focus handling in wxGTK stays consistent with it.
The only case I can see this breaking is a window which handles keyboard input itself and also has some non-focusable children, e.g. a custom canvas with a wxStaticBitmap on top of it, which seems rare. I added a unit test for it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#27049)Unfortunately this results in a real test failure in wxOSX CI job:
-------------------------------------------------------------------------------
Window::ContainerFocus
With focusable children
-------------------------------------------------------------------------------
./controls/windowtest.cpp:505
...............................................................................
./controls/windowtest.cpp:510: FAILED:
CHECK( panel->AcceptsFocusFromKeyboard() )
with expansion:
false
Not sure what is wrong with it, but this needs to be fixed before this PR can be merged.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Quin Gillespie
trypsynth left a comment (wxWidgets/wxWidgets#27049)Thanks, I've found the problem: under macOS a native button only accepts focus from keyboard if Full Keyboard Access is on, which it isn't on the CI machines. So the panel in this test really had no focusable children there and the new check was working as intended. I've changed the test to use wxTextCtrl, which always accepts focus, instead of wxButton.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Quin Gillespie
@trypsynth pushed 1 commit.
- e27bf5a fix container focus test failing under macOS
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#27049)Sorry, after thinking more about this in a less sleepy state, I'm not sure if this is really a good idea as the idea of a wxPanel accepting keyboard input and having a wxStaticText somewhere inside it is not that ridiculous... Worse, there is no way to search for something like this and I can't even draft a good paragraph for the "incompatible changes" section of the change log explaining what needs to be done if anybody is affected by this because there doesn't seem to be any good workaround.
What about a lesser change: could we make just AcceptsFocusFromKeyboard() return false in this case? This should still fix the original problem but be much less disruptive.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Quin Gillespie
trypsynth left a comment (wxWidgets/wxWidgets#27049)You're right, and I agree that the behaviour change is too subtle to be worth it: I'll restrict this to AcceptsFocusFromKeyboard(), which still fixes the original problem, as TAB won't stop on such windows any more, while SetFocus() and clicking still work exactly as before. This also means the change in UpdateParentCanFocus() isn't needed any more, as it only existed to keep the native focus handling in wxGTK consistent with AcceptsFocus(), so I'll revert it too.
For the change log, this could be described as "TAB doesn't stop on the windows whose children all refuse focus, such as a wxPanel containing just a wxStaticText, any more", and the workaround for anybody affected by it is to call SetFocus() explicitly or to make one of the children focusable.
I'd like to mention why this matters in practice: screen reader users tabbing through wxWidgets dialogs regularly land on windows which are announced just as "panel", with nothing else to say and nothing to do there, and several of the blind users I've talked to consider this a wxWidgets bug. This PR started from exactly such a case in the generic about dialog.
One case I'd like to handle in this version: a wxScrolled window whose children all refuse focus, e.g. a scrolled panel containing just a long wxStaticText, is currently focusable from keyboard and this is the only way to scroll it with the arrow keys, see wxAnyScrollHelperBase::HandleOnChar(). This doesn't matter for the screen reader users, as the text is read from the accessibility tree anyhow, but it does for the sighted people using the keyboard only. So I plan to keep accepting focus from keyboard if the window can be scrolled:
bool wxControlContainerBase::AcceptsFocusFromKeyboard() const { // A window with children none of which can accept focus doesn't need to // be focused from keyboard itself, unless it can be scrolled using it. if ( HasAnyClientAreaChildren() && !HasAnyFocusableChildren() && !m_winParent->CanScroll(wxVERTICAL) && !m_winParent->CanScroll(wxHORIZONTAL) ) return false; return AcceptsFocusRecursively(); }
Does this look right to you, or would you rather keep it simpler and skip the scrolling check?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Quin Gillespie
@trypsynth pushed 1 commit.
- 516fa8d Don't stop TAB navigation on windows without focusable children
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Quin Gillespie
trypsynth left a comment (wxWidgets/wxWidgets#27049)Updated: this now only changes AcceptsFocusFromKeyboard(), as you suggested, and keeps accepting focus from keyboard for the scrollable windows. The unit test covers both cases and all the existing [window] and [focus] tests still pass.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#27049)CI failure looks due to the same gcc bug we already workaround in other places, you may add
#if wxCHECK_GCC_VERSION(16, 0) && !wxCHECK_GCC_VERSION(16, 3) wxGCC_WARNING_SUPPRESS(array-bounds) #endif
to avoid it. It doesn't look like this one is going to be fixed any time soon :-(
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#27049)Thanks, I think it's good to go like this.
Does anybody else have any thoughts about this? It's still a backwards-incompatible change but I think it should be fine in practice.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Quin Gillespie
@trypsynth pushed 1 commit.
- ca41a6c Don't stop TAB navigation on windows without focusable children
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.