[bug] Clicking on a Checkbox should give focus to the checkbox.

[rerdavies]

Clicking on a Checkbox should give focus to the checkbox. It's a particularly unpleasant UX experience, because of the current ambiguity between rendering of focus state and hover state in checkboxes.

Having TWO checkboxes in apparent focused state is unsettling to begin with (one actually actually focused, and the other actually in hover state). It would be mostly forgivable if the focus issue were to resolve itself after a mouse click. Failing to clear focus on the non-hover Checkbox after a mouse click feels like a serious bug, and it is. Because it happens so often, it violates users' trust, giving them the strong impression that they're dealing with buggy software that can't be relied upon..

It doubly sucks for me, because I'm using your library to perform initial setup of a large package, and the very first impression users are going to get is that of broken checkboxes that are controlling settins for kernel commandline arguments! If there were ever a situation where violation of trust really counts, that would be it.

A one line fix:

Checkbox.cpp:71

  if (event.mouse().button == Mouse::Left &&
        event.mouse().motion == Mouse::Pressed) {
      *checked = !*checked;
      TakeFocus();   // <--- ADD THIS LINE
      on_change();
      return true;
    }

Not immediately clear whether it should actually be handled in ComponentBase instead. I'll let you be the judge of that. But Button should probably take focus on a mouse click as well (it does not currently). And Radiobuttons. And dropdowns. &c. The advantage of doing it this way is that it's nice to get state changes out of the way before potentially firing any events (in future implementations).

Note that handling it in a CheckBoxOptions::on_change handler is unworkable, since there's no sensible way for the lambda to capture a pointer to a Component that has not yet been created. You might want to think about changing CheckboxOptions::on_change from

  std::function<void()> on_change = [] {};

to

  std::function<bool(ComponentBase::ptr component)> on_change = [] (ComponentBase::ptr component) { return false; };

if you ever do a major release that has breaking changes.

[ArthurSonzogni]

Hello @rerdavies

Swapping the lines:

      on_change();
      TakeFocus();

sounds like a good idea to me!

Would you like to make a PR for the elements you described?

---

Note that handling it in a CheckBoxOptions::on_change handler is unworkable, since there's no sensible way for the lambda to capture a pointer to a Component that has not yet been created. You might want to think about changing CheckboxOptions::on_change from

I don't want to change it, because it would make it difficult for users to assign a lambda.

Maybe we could introduce some global:

// The component that is currently calling a callback.
// For instance MenuOption::on_change.
static ComponentBase::Active();

and we would call it like this:

  ScopedActiveComponent(this);
  on_change();

[rerdavies]

I disagree that the order should be reversed. The on_change notification should either come before everything you do (and return a handled boolean) OR it should come after everything you do, giving the caller final authority to undo or redo anything that you have done. The order should definitely be: TakeFocus(); on_change(); as a matter of principle, and good practice. If client applications wants to control focus after the value has changed, they would not be able to do so if the calls are in the order you suggested. (Not sure why that would want to do that, but it's a general guiding principle that should apply for all events: either before any state change or after all state changes. Can I include problems with Radiobox focus in the same PR (which are significantly worse than checkbox when mixing mouse and keyboard actions). Or would you like a separate PR for that? I'm not sure what the fix is for the Radio boxes. The interactions between keyboard focus, and mouse hover/mouse focus is very strange and in your face. For what it's worth, my shipping implementation: 1) Re-does the renderinging of checkboxes. 2) Does forced TakeFocus to fix problesm with Checkbox focus/nav handling. 2) Because the RadioBox is unusable, wraps Checkbox to make Checkboxes that look like individual Radio buttons. 3) Makes a mess binding the three radio checkboxes masquerading as radio buttons together into a group. But at least the focus issues are fixable. Not so for the Radiobox. 3) Uses "global state" (actually points to method variables captured by lambas) to force focus changes every time any of the radio/check buttons change. 4) Dialog boxes.... huh. Is that not a feature that you think users would be interested in? The example is horrifying. And my code is 3x more horrifying because I have three dialogs. 5) Deep regrets over the various deficiencies that I was willing to live with just to get clear of the code.... Esc/Enter handling for dialogs for example. 6) Still have to check to see whether you've handled signals correctly. :-/ All in all, a bitter lesson in "Sunk Cost" fallacy. I should have walked away at (1). Sorry. I don't know how to put this more politely. This package should not be shipped in the state that it's in.

…

On Sun, Dec 1, 2024 at 4:26 AM Arthur Sonzogni ***@***.***> wrote: Hello @rerdavies <https://github.com/rerdavies> Swapping the lines: on_change(); TakeFocus(); sounds like a good idea to me! Would you like to make a PR for the elements you described? ------------------------------ Note that handling it in a CheckBoxOptions::on_change handler is unworkable, since there's no sensible way for the lambda to capture a pointer to a Component that has not yet been created. You might want to think about changing CheckboxOptions::on_change from I don't want to change it, because it would make it difficult for users to assign a lambda. Maybe we could introduce some global: // The component that is currently calling a callback.// For instance MenuOption::on_change.static ComponentBase::Active(); and we would call it like this: ScopedActiveComponent(this); on_change(); — Reply to this email directly, view it on GitHub <#959 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACXK2DBLUCKPPDUUE7P33ML2DLI3ZAVCNFSM6AAAAABSVGGO6KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMBZGY2TGNBYGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>