Ensure settings are scrolled to using tab navigation #12300
Conversation
See test results for failed build of commit 11634169e7 |
|
Hello I have tested the snapshot in Doc formatting, Speech, Braille, Vision and Advanced panels. |
|
Thanks @CyrilleB79 - that issue seems to be from an unrelated cause so I have fixed it on #12307 |
source/gui/nvdaControls.py
Outdated
| def ScrollChildIntoView(self, child: wx.Window) -> None: | ||
| """ | ||
| Uses the same logic as super().ScrollChildIntoView(self, child) | ||
| except cr = self.GetChildRectRelativeToSelf(child) instead of cr = GetRect() | ||
| """ | ||
| sppu_x, sppu_y = self.GetScrollPixelsPerUnit() | ||
| vs_x, vs_y = self.GetViewStart() | ||
| cr = self.GetChildRectRelativeToSelf(child) | ||
| clntsz = self.GetClientSize() | ||
| new_vs_x, new_vs_y = -1, -1 |
There was a problem hiding this comment.
I would love it if there was a way to avoid having to duplicate the majority of this function.
There was a problem hiding this comment.
how does this new logic look? I am not a fan of overriding this function but it makes the actual change that we are doing clearer.
See test results for failed build of commit a3954250c6 |
See test results for failed build of commit 89a2c64a2e |
source/gui/nvdaControls.py
Outdated
| """ | ||
| oldChildGetRectFunction = child.GetRect | ||
| child.GetRect = lambda: self.GetChildRectRelativeToSelf(child) | ||
| super().ScrollChildIntoView(child) |
There was a problem hiding this comment.
Could you please place the super call in a try block, and the next line in a finally block. This will ensure that the function will be set back to its original value even if super throws an exception.
| child.GetRect = lambda: self.GetChildRectRelativeToSelf(child) | ||
| try: | ||
| super().ScrollChildIntoView(child) | ||
| finally: | ||
| # ensure child.GetRect is reset properly even if super().ScrollChildIntoView throws an exception | ||
| child.GetRect = oldChildGetRectFunction |
There was a problem hiding this comment.
I avoided suggesting this because it's a little dangerous. If any of the other method calls within super().ScrollChildIntoView rely on the absolute value for rect for the child, this will be broken in a subtle way.
Worse, by the time we update wxPython, we may have forgotten about this.
There was a problem hiding this comment.
child is only used once in super().ScrollChildIntoView, and that is to calculate the incorrect child.GetRect https://github.com/wxWidgets/Phoenix/blob/wxPython-4.1.1/wx/lib/scrolledpanel.py#L190. Do we only update wxPython with compatibility breaking releases? That way I can mark this as deprecated for 2022.1 so it won't get lost.
There was a problem hiding this comment.
I think it makes sense that we only update on compat breaking releases. Let's mark it as deprecated.
seanbudd
added
the
deprecated/2022.1
Link to issue number:
Closes #12224
Summary of the issue:
Navigating through settings using tabbing does not visually scroll to the focused control.
wxPython
ScrolledPanels calculate the position to scroll to based on the relative position to a focus elements parent, rather than the relative position to theScrolledPanelitself.When fixing right-to-left issues in #12181, another layer of nesting was introduced for controls in our settings panels, which caused the controls to no longer get scrolled to.
Description of how this pull request fixes the issue:
A patched version of
ScrolledPanelis created, which calculates relative position to theScrolledPanel. A PR to wxPython has been created which implements this fix: wxWidgets/Phoenix#1950.Testing strategy:
Ensure tabbing through long SettingsPanels such as "Document Formatting" scrolls the panel correctly.
Known issues with pull request:
None
Change log entry:
None, fixes regression
Code Review Checklist: