Skip to content
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 Issues #226

Closed
CasualYT31 opened this issue Dec 16, 2023 · 1 comment
Closed

Focus Issues #226

CasualYT31 opened this issue Dec 16, 2023 · 1 comment

Comments

@CasualYT31
Copy link
Contributor

I've come across a small issue concerning setting focus onto an indirectly disabled EditBox or TextArea. This has consequences beyond those two widget types alone, but I was using a TextArea when I came across this issue so I'll use it for the sake of an example.

If I perform the following (which I will call the RootContainer example):

Gui gui{ previouslySetUpWindow };
const auto t = TextArea::create();
gui.add(t);
gui.getContainer()->setEnabled(false);
t->setFocused(true);
gui.getContainer()->setEnabled(true);

The TextArea receives focus as normal. I can immediately begin typing and the widget will receive the key events.

However, when I perform the following:

const auto g = Group::create();
gui.add(g);

const auto t = TextArea::create();
g->add(t);

g->setEnabled(false);
t->setFocused(true);
g->setEnabled(true);

The TextArea enters a quasi-focused state where the caret will blink, but it won't receive key events. I can receive focus properly if I click off the TextArea and then click back on it.

Taking a closer look at the code I can see the problem:

    void TextArea::setFocused(bool focused)
    {
        if (focused)
        {
            m_caretVisible = true;
            m_animationTimeElapsed = {};
        }
        else // Unfocusing
        {
            // If there is a selection then undo it now
            if (m_selStart != m_selEnd)
            {
                m_selStart = m_selEnd;
                updateSelectionTexts();
            }
        }

#if defined (TGUI_SYSTEM_ANDROID) || defined (TGUI_SYSTEM_IOS)
        if (focused)
            keyboard::openVirtualKeyboard(this, {{}, getSize()});
        else
            keyboard::closeVirtualKeyboard();
#endif

        Widget::setFocused(focused);
    }

The TextArea will always make the caret visible, etc., even if it can't technically receive the focus. This is a problem in itself since if I keep the Group disabled, it is counter-intuitive for the TextArea to have a visible caret when the user cannot type in it, even if this state was not entered into by the user by clicking on the TextArea.

But the main problem is that judging by the RootContainer example, it seems as though the Group should be able to recover if it is re-enabled, but it won't (via a simple setEnabled(true) call, anyway).

Taking a deeper dive via my debugger, I can see that once the TextArea invokes the Widget::setFocused() method, and it tries to invoke childWidgetFocused() on the Group, the Group successfully updates its m_focusedWidget pointer (which is why the RootContainer example above works), but when it tries to set the focus onto itself, it can't since it is disabled. And so the RootContainer does not have the pointer to the Group, meaning the TextArea can't receive key events.


I am not wholly certain what the intended behaviours of the above scenarios are. But I have found that using this version of the canGainFocus() method:

bool Widget::canGainFocus() const
{
    return m_enabled && m_visible && m_focusable && (m_parent ? m_parent->canGainFocus() : true);
}

Will prevent the TextArea from gaining any kind of focus if it is indirectly disabled, and the caret will not show (I assume this is because its immediate parent no longer has a focused widget? I am not sure though). Which I think makes the most sense, even if it might cause confusion if one attempts to invoke setFocused(true) on an indirectly disabled widget. I feel like it would be nice to be able to apply set focus once the Group is re-enabled, though I imagine this will be more complicated, and it may not even be a good idea.

What are your opinions on the above?

Thanks for your time.

@texus
Copy link
Owner

texus commented Dec 17, 2023

Thanks again for the detailed information on what happens and figuring out where the issue lies.

After looking into this I believed that you fix was the best option (although I was going to apply the fix directly in Widget::setFocused as the other location where canGainFocus is called doesn't require the recursive check). However, I accidentally fixed it in a completely different way.

I changed the order of operations in all widgets that override the setFocused function. Instead of letting TextArea set its m_caretVisible to true and then changing the focused state with the Widget::setFocused call, I decided to first call Widget::setFocused and only set m_caretVisible to true if m_focused actually became true (which wouldn't be the case with your canGainFocus patch). Doing the same in the Container class has a very surprising side-effect: failing to focus the container would unfocus its child widgets.

So the following now happens:

  • You call TextArea::setFocused
  • The text area tells its parent that is has been focused
  • The container tries to focus itself but fails because it is disabled
  • The container notices that the text area is focused and tells it to unfocus (at the end of Container::setFocused)

This means that edit box ends up unfocused, without changing how canGainFocus works. Hopefully the change I just made doesn't break a bunch of other situations though.

the caret will not show (I assume this is because its immediate parent no longer has a focused widget?

This is because Widget::setFocused won't set m_focused to true if canGainFocus returns false. The caret is only shown when both m_caretVisible and m_focused are true. Technically m_caretVisible is never supposed to be true when m_focused is false, but the value of m_caretVisible is simply ignored if m_focused is false so it still worked correctly even though m_caretVisible was wrong.

Which I think makes the most sense, even if it might cause confusion if one attempts to invoke setFocused(true) on an indirectly disabled widget.

It agree that not allowing the focus is the best solution. It's easy to explain why focusing doesn't work in that case, and it can be easily implemented.

I feel like it would be nice to be able to apply set focus once the Group is re-enabled, though I imagine this will be more complicated

Yeah, that would probably be too complex to implement, and the focusing code is already quite complicated.

@texus texus closed this as completed in b89892c Dec 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants