-
Notifications
You must be signed in to change notification settings - Fork 568
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
Focus-chain update #1724
Focus-chain update #1724
Conversation
Update Local fork
…o hidden widgets anymore. Signed-off-by: xarvic <xarvix@web.de>
Signed-off-by: xarvic <xarvix@web.de>
Signed-off-by: xarvic <xarvix@web.de>
Signed-off-by: xarvic <xarvix@web.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused here, and will need to take a second look (or maybe we should have a quick chat about it) I'm having a slow time remembering the context for this. In any case, one question inline.
// currently focused is not part of the functional tree anymore | ||
// (Lifecycle::BuildFocusChain.should_propagate_to_hidden() is false!) and should | ||
// resign the focus. | ||
if had_focus && !self.state.has_focus { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused; we haven't done merge_up
yet, so how would has_focus
have changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The merge_up
of our children happened already in line 1058 / 1062. If someone hides a focused widget and then calls children_changed()
, we clear has_focus
in BuildFocusChain
if has_clear is not set after the calls in 1058 and 1062 we know, that the focused widget is not reachable
Co-authored-by: Colin Rofls <colin@cmyr.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that explanation works for me, thanks!
This is a fix of the part of #1716 @cmyr and i agreed on:
should_propagate_to_hidden()
now returnsfalse
forLifecycle::BuildFocusChain
Lifecycle::BuildFocusChain
requests a a resign of focus ifstate.has_focus
is set but neither one of its hidden children nor it self has focus.The other point: What should happen if try to focus a WidgetID which is not allowed to recieve focus like
ctx.set_focus(WidgetID::new())
or focusing a hidden widget is not addressed here.