Make has_focus reliable.

[xStrom]

This is a simpler implementation of parts of #811.

• has_focus no longer returns false positives and can be relied on.
• Focus cycling now works even when starting from a non-registered-for-focus widget that merely called request_focus. The cycle is still only between registered-for-focus widgets.
• Fixed propagation of RouteFocusChanged and FocusChanged. Previously they were propagated only to the focus losing widget, unless there was no previously focused widget. This behavior stumbled into somewhat working because the request_focus in BaseState didn't get cleared and was merged up again generating a new RouteFocusChanged where old and new are the same. This allowed clearing of request_focus but also generated a FocusChanged(false) to the widget that never received FocusChanged(true). When a 3rd widget gets focus, the 2nd one receives another FocusChanged(false).

[xStrom]

Added another fix for when there's only a single widget registered for focus.

Cycling to the "next" widget would cause a RouteFocusChanged with the same old and new but only a single FocusChanged was generated per event, FocusChanged(false).

Now a FocusChanged event gets generated only when there's actual change.

[luleyleo]

looks good to me, I feel like we need better example to properly test PRs like this though.

[xStrom]

I added a change that FocusChanged now only gets sent to the widget that specifically has their is_focused state changed. That is, if that widget just passes all their events down to children, WidgetPod will make sure to stop the propagation of FocusChanged. More discussion about this on zulip.

[cmyr]

This looks broadly good; I have a few comments.

I also wonder, in passing, if this might be something that it is worth writing a test against. This kind of stuff is what most of the current testing tools are intended for, and this is just tricky enough that it might be helpful to verify; it might also be a good opportunity to dig into some of the testing code. There's already a couple tests for focus behaviour, in druid/src/tests/mod.rs.

[cmyr]

this highlights a definite little rough spot. What should tab do if nothing is focused? A reasonable argument is that it should send focus to the first item in the focus chain.

This is related to what should happen when something that isn't in the focus chain is focused, and we end up in this code path.

Realistically, this shouldn't happen. To get here, a widget has to explicitly request next or previous focus. A widget that is doing that has also had the opportunity to register to participate in focus. Doing one without the other feels like maybe an API violation?

This again touches, to me, on the idea of 'ambient focus'. How I interpret this is basically that if no other widget has focus, the root widget does. A <tab> from the root widget should focus the next focusable widget, but the root widget itself should not be in the focus chain; it should only receive focus if another widget resigns focus without trying to pass it on, as might happen when someone presses <esc>.

So with all that said, I think that this implementation is reasonable. A widget has asked for next/previous focus, so we should focus something.

This would also then work for the root widget, if it had focus; if it sent focus_next then that would just grab the first item in the chain.

Given this, should 'previous' maybe take the last item?

[xStrom]

Regarding focus_prev yes I think it should take the last item, I'll change that. The reason is that I just checked that Chrome does work that way, so it's probably what users expect.

As for a widget being focused without registering for focus, I think this is not an API violation and something that is useful to have. It exists in HTML and can be seen in this example I set up. The use case would be a form that has rarely used fields. Then you can have a textbox which can be focused when clicked to change its contents, but in 95% of use cases the user doesn't need to modify it so it doesn't need to be in the tab cycle. However this would make life more difficult for keyboard only users.

This starts touching upon accessibility in general and being able to tab through buttons and dropdowns etc. I think there's major value in supporting keyboard navigation automatically in all of druid's included widgets. This is a rather large topic though. I think we should start a new issue to talk about this. It's not super clear what the answer is to add any ambient focus support in this PR right now.

[cmyr]

your logic here makes sense to me; and I think ambient focus makes sense as a separate patch.

I agree about better support for keyboard navigation, and also yes it's a big one.

[cmyr]

👍

[xStrom]

Regarding tests, yes I've been thinking about that as well. Because some of this stuff is hard to grasp from just glancing at the code.

I'm planning on writing test code for widget id verification soon, because I know for a fact that some of that stuff is broken and I want it to remain fixed.

After I've had the experience with the widget id tests I can rotate back to focus and write some more robust tests.

[cmyr]

Great, thanks!