Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stopwatch key presses stop working after remove with focus #939

Closed
aaronst opened this issue Oct 18, 2022 · 10 comments
Closed

stopwatch key presses stop working after remove with focus #939

aaronst opened this issue Oct 18, 2022 · 10 comments

Comments

@aaronst
Copy link
Contributor

aaronst commented Oct 18, 2022

From the css branch on version 0.2.0b7, it looks like there is a bug in the docs/example/tutorial/stopwatch.py example or maybe something to do with focus.

To reproduce, run stopwatch.py, use tab to focus the "Reset" button on the last stopwatch, and then press r to remove it. After this, the app no longer responds to any key presses, including ctrl+c. Using the mouse to click a button seems to get things working again.

@aaronst
Copy link
Contributor Author

aaronst commented Oct 18, 2022

This also happens if the "Stop" button on the last stopwatch has focus when it is removed.

@davep
Copy link
Contributor

davep commented Oct 18, 2022

Testing with 0.2.0b6 (I've not updated my beta sandbox locally yet), following the above, I get this crash instead:

╭──────────────────────────────────────────────────────────── Traceback (most recent call last) ─────────────────────────────────────────────────────────────╮
│ /Users/davep/develop/python/textual-beta/.venv/lib/python3.10/site-packages/textual/widget.py:1853 in set_focus                                            │
│                                                                                                                                                            │
│   1850 │   │                                                                                                                                               │
│   1851 │   │   def set_focus(widget: Widget):                                                                                                              │
│   1852 │   │   │   """Callback to set the focus."""                                                                                                        │
│ ❱ 1853 │   │   │   widget.screen.set_focus(self, scroll_visible=scroll_visible)                                                                            │
│   1854 │   │                                                                                                                                               │
│   1855 │   │   self.app.call_later(set_focus, self)                                                                                                        │
│   1856                                                                                                                                                     │
│                                                                                                                                                            │
│ ╭────────────────────────────────────── locals ───────────────────────────────────────╮                                                                    │
│ │ scroll_visible = True                                                               │                                                                    │
│ │           self = Button(id='stop', classes={'-error', '-default'}, variant='error') │                                                                    │
│ │         widget = Button(id='stop', classes={'-error', '-default'}, variant='error') │                                                                    │
│ ╰─────────────────────────────────────────────────────────────────────────────────────╯                                                                    │
│                                                                                                                                                            │
│ /Users/davep/develop/python/textual-beta/.venv/lib/python3.10/site-packages/textual/dom.py:280 in screen                                                   │
│                                                                                                                                                            │
│   277 │   │   while node and not isinstance(node, Screen):                                                                                                 │
│   278 │   │   │   node = node._parent                                                                                                                      │
│   279 │   │   if not isinstance(node, Screen):                                                                                                             │
│ ❱ 280 │   │   │   raise NoScreen(f"{self} has no screen")                                                                                                  │
│   281 │   │   return node                                                                                                                                  │
│   282 │                                                                                                                                                    │
│   283 │   @property                                                                                                                                        │
│                                                                                                                                                            │
│ ╭────────────────────────────────── locals ───────────────────────────────────╮                                                                            │
│ │   node = None                                                               │                                                                            │
│ │ Screen = <class 'textual.screen.Screen'>                                    │                                                                            │
│ │   self = Button(id='stop', classes={'-error', '-default'}, variant='error') │                                                                            │
│ ╰─────────────────────────────────────────────────────────────────────────────╯                                                                            │
╰────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
NoScreen: Button(id='stop', classes={'-error', '-default'}, variant='error') has no screen

On the surface it would appear it's a focus issue in Textual itself.

@davep
Copy link
Contributor

davep commented Oct 18, 2022

And just to confirm: once updated to b7 I get the same as @aaronst.

@davep
Copy link
Contributor

davep commented Oct 18, 2022

Also to add to the above, with b7: once the app is in the "won't respond to keyboard" state, none of the key bindings work either -- so no light/dark mode toggle, etc. As @aaronst says: once you've used the mouse to get some focus back everything works fine again.

@aaronst
Copy link
Contributor Author

aaronst commented Oct 18, 2022

Could be related to #936?

@darrenburns
Copy link
Member

@davep That was fixed in this PR (and b7) - https://github.com/Textualize/textual/pull/936/files

@davep
Copy link
Contributor

davep commented Oct 18, 2022

Aye, was just sort of suggesting that it might be something in Textual rather than in the stopwatch example.

@darrenburns
Copy link
Member

@davep Sorry, was referring to this crash:

Testing with 0.2.0b6 (I've not updated my beta sandbox locally yet), following the above, I get this crash instead:

davep added a commit to davep/textual that referenced this issue Oct 18, 2022
This change ensures that, for example, the widget remains in the
Screen.focus_chain so that it's easier to handle passing off focus on
removal.

To help address Textualize#939.
davep added a commit to davep/textual that referenced this issue Oct 18, 2022
Rather than call on Screen._reset_focus, which seems to only really consider
siblings, instead call on the being-removed widget's screen to find the
previous widget that can receive focus.

Addresses Textualize#939.
davep added a commit to davep/textual that referenced this issue Oct 19, 2022
This change ensures that, for example, the widget remains in the
Screen.focus_chain so that it's easier to handle passing off focus on
removal.

To help address Textualize#939.
davep added a commit to davep/textual that referenced this issue Oct 19, 2022
davep added a commit to davep/textual that referenced this issue Oct 19, 2022
Addresses the issue raised in Textualize#939. Here I rework Screen._reset_focus so
that, rather than deal with siblings, it deals with the focus chain. It now
also optionally takes a list of DOMNodes to avoid and will, as that
suggests, avoid them when looking for somewhere to move focus to.

As a preference the new version of this will seek to settle focus on the
nearest (in the chain) *previous* non-excluded DOMNode.

NOTE: Currently the return value of Widget.walk_children is cast to a list
to ensure that it doesn't get consumed (as of the time of commit, it's still
qan iterable). Once Textualize#952 has been
accepted I'll change this (remove the cast).
@davep
Copy link
Contributor

davep commented Oct 19, 2022

A fix for this is now in the CSS branch: #954

Thanks again @aaronst for catching this.

@davep davep closed this as completed Oct 19, 2022
@github-actions
Copy link

Did we solve your problem?

Consider buying the Textualize developers a coffee to say thanks.

Textualize

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants