feat(core): allow focusing of ancestors of renderables#930
feat(core): allow focusing of ancestors of renderables#930garthdw wants to merge 7 commits intoanomalyco:mainfrom
Conversation
garthdw
requested review from
Hona,
kommander,
msmps and
simonklee
as code owners
|
Dammit, nothing worse than that quick code change before a PR to break tests and then make you end up with more brutal code. :/ |
|
Interesting. I think that makes sense. I don't know how opentui users currently use it. The renderer knows only about exactly one focused renderable. It could however be an additional tree attribute that is propagated? Wonder how the browser and apps there handle that. If you want a box around a Textarea to react to the Textarea focus, it can subscribe to that state. I consider only Renderables that handle key events to be focusable. So style/ui only elements would not set focusable=true on themselves. |
|
So looks like html has a Screen.Recording.2026-04-07.at.21.05.55.movI'll explore some alternatives to how to represent this behaviour. |
garthdw
force-pushed
the
feat/support-hierarchy-focusing
branch
from
41f4134 to
41ed48c
Compare
|
I have implemented it a different way. I'm toying between doing it as just |
packages/core/src/Renderable.ts
Outdated
| while (parent) { | ||
| if (parent._hasFocusedDescendant !== hasFocus) { | ||
| parent._hasFocusedDescendant = hasFocus | ||
| parent.requestRender() |
There was a problem hiding this comment.
It should be enough to call this once after the while loop, as multiple calls will just be ignored anyway when a render was already requested.
There was a problem hiding this comment.
I was also cautious about the multiple calls. The benefit we get is the mark as dirty, but if we don't need that and can't foresee any other issues, I'm happy to move it out of the loop and call it on itself.
| public blur(): void { | ||
| if (!this._focused || !this._focusable) return | ||
|
|
||
| this._ctx.blurRenderable(this) |
There was a problem hiding this comment.
I realised earlier this week that renderer.focusedRenderable basically never gets reset. It's also not reset when the renderable is destroyed. So in the renderable destroy method it should probably be blurred as well. Would be cool to get this new blur behaviour for renderer level focus/blur test covered.
There was a problem hiding this comment.
So "luckily" this change will help with the destroy. It calls blur when destroying.
Let me add test coverage though.
Only focusable ancestors are focused. Opt in behaviour.
We now just build the focusedRenderables from scratch While knowing if we are initiator of a focus or just part of it.
Walk the tree and set if it has dependants. Remove the focus state in renderer.ts
garthdw
force-pushed
the
feat/support-hierarchy-focusing
branch
from
50662f2 to
86122d4
Compare
Actually check we are blurring.
2 participants
Only focusable ancestors are focused.
Opt in behaviour at the renderer level and needing to mark direct ancestors as focusable.
Screencast_20260406_145554.mp4
For #929