Fix keyboard event routing in Tk backend (fixes #13484, #14081, and #22028) #22077
Conversation
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
|
Glad I added the test, looks like Tk is behaving differently on macOS. I'll need to set up a VM or see if I can get access to a Mac to figure out what exactly causes the failure. |
|
Looks like on macOS, |
2103a9f to
52fc942
Compare
There was a problem hiding this comment.
Thanks for the contribution!
The code and behavior of this patch are good, but the fact is that this is a breaking change for embedded users and we need to handle them appropriately. At a minimum the patch should be prominently featured in the release notes and not backported. It would also be nice to explicitly document this somewhere besides the example, somewhere that we already contrast other backend-specific quirks.
It may also be appropriate to do some kind of depreciation period before applying it. I'm happy to patch my own app fairly quickly but it may be more trouble for others?
|
@richardsheridan thanks for your feedback! I agree that a note is definitely needed somewhere. I was unsure initially about how much other embedded users rely on the current behaviour, as it's undocumented, different from what's expected from any other widget, and existing code should still work after the patch if an end user clicks the plot (which I was doing myself before I realised the canvas claimed the focus for itself). If we do go with a deprecation period, we should probably still add the |
|
This fixes #22255, and https://gitlab.com/eBardie/topplot/-/issues/46. Thanks @daniilS. |
52fc942 to
a1d7d6c
Compare
|
I've removed the backward-incompatible change that prevented the tk canvas from stealing the focus upon creation, and will submit a separate PR for that with proper deprecation, aimed at 3.6.0. For now, this just closes #22028, #13484, #22255, and this issue on Discourse. As it's just a fix now, I would propose it get included in 3.5.2. |
PR Summary
Currently, when using the Tk backend, Matplotlib sets the focus to the figure canvas upon its creation, as this is required to process keyboard events. As reported in #14081, this takes away the focus from other widgets when embedding a plot in a tkinter GUI, which creating a widget should never do (though the issue of other widgets being unable to take back the focus was fixed in Tk 8.6.11). Since a recent Python/Tk update, the focus doesn't get set to the canvas at all (#22028), though I cannot reproduce this outside of Matplotlib so can't tell what exactly caused this regression. Finally, the canvas has no visual indicator of having the focus, which makes it hard to get it to accept keyboard events again after tabbing out (#13484).
In this PR, the focus instead gets set to the canvas during the first call to
plt.show(). It also gets set when the canvas receives a mouse click event, as is standard for widgets responding to keyboard events (e.g. entry widgets).canvas.get_tk_widget().focus_set()), as with any other widgetI've also added a unit test, and updated the "embedding in tk" example to reflect this behaviour.
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).