Fix focus chain code #1034
Fix focus chain code #1034
Conversation
Fix bug where parts of the focus chain would disappear when widgets updated. Make Button accept focus. Make Prose not accept focus. Add TextEvent constructors.
| // FIXME - Handle this properly | ||
| if ctx.is_focus_target() { | ||
| border_color = &BorderColor { | ||
| color: Color::WHITE, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Drawing the focus outline should be done centrally in the paint pass. Not inside of the widget.
There was a problem hiding this comment.
Disagree. I think it should ultimately be stylable, once we add pseudo-classes to the styling system.
There was a problem hiding this comment.
I strongly think widgets shouldn't have to do anything to get a focus outline. I think that's important for accessibility, or otherwise there is bound to be widgets where it has been forgotten. I think it could still be stylable even if provided outside of the Widget impl.
There was a problem hiding this comment.
That's a valid point. I'd recommend opening an issue about it, because I don't think this needs to be resolved in this PR.
There was a problem hiding this comment.
My point is that globally providing this by default is more important than making it customizable, so I'd rather see this implemented in the paint pass now and implementing customization afterwards.
There was a problem hiding this comment.
There's a bunch of complications in implementing "paint an outline by default on any focused widget" that I don't really want to deal with right now.
Seeing as that the status quo is that outline-by-default isn't implemented and this PR doesn't make it any less likely to get implemented, I'd rather avoid adding a drive-by requirement to it.
In other words, outline-by-default should be implemented in a separate PR. This PR fixes an important accessibility issue even without adding outline-by-default it.
There was a problem hiding this comment.
I had implemented this locally in 10 lines without noticing any complications (might not be perfect but still better than nothing) ... but sure I can make a PR for that.
| const SPACE: &str = " "; | ||
| match event { | ||
| TextEvent::Keyboard(event) | ||
| if event.key == Key::Character(SPACE.into()) && event.state.is_up() => |
| @@ -796,7 +796,7 @@ impl<const EDITABLE: bool> Widget for TextArea<EDITABLE> { | |||
| } | |||
|
|
|||
| fn accepts_focus(&self) -> bool { | |||
| true | |||
| EDITABLE | |||
There was a problem hiding this comment.
Can you confirm that accepts_focus is only related to the focus chain? If so, I believe this behaviour to be correct.
The one complication is that it should ideally still be possible to select text inside a prose from only the keyboard, but I'm happy to defer that as a longer-term consideration.
Fix bug where parts of the focus chain would disappear when widgets updated. Make Button accept focus.
Make Prose not accept focus.
Add TextEvent constructors.