Bug FIx: Sticky mouseover highlight with juce::Button

General JUCE discussion
1

This code results in a stuck mouseover highlight on the button when right-clicking to see the menu.

Any suggestions as to how to avoid it?

The problem goes away if I choose setTriggeredOnMouseDown(true), but we don’t always want that!

    class B : public Button
    {
    public:
        B() : Button({})
        {
//            setTriggeredOnMouseDown(true);
        }

        void paintButton(Graphics & g, bool shouldDrawButtonAsHighlighted, bool shouldDrawButtonAsDown) override
        {
            g.setColour(Colours::white);

            if (shouldDrawButtonAsHighlighted)
                g.fillAll(Colours::white.withAlpha(0.2f));


            g.drawText("Button", getLocalBounds(), Justification::centred);
            g.drawRect(getLocalBounds(), 1);

        }


        void clicked() override
        {
            PopupMenu m;
            m.addItem("Item", []() {});
            m.showMenuAsync({});
        }
        
    };
2

Some notes on this bug. It’s a problem with Component or Button. It’s not a repaint issue. The internal state of Button is stuck in mouseOver. After clicking on the button no mouseExit event is ever sent to the Button even once the mouse has moved away from the popup menu.

3

The problem is fixed by moving updateState to be after the click event is triggered. And copying the “deletionWatcher” pattern from elsewhere in Button.

Suggested replacement code is below and would fix another long standing niggle :slight_smile:

void Button::mouseUp (const MouseEvent& e)
{
    WeakReference<Component> deletionWatcher (this);

    const bool wasDown = isDown();
    const bool wasOver = isOver();

    if (wasDown && wasOver && ! triggerOnMouseDown)
    {
        if (lastStatePainted != buttonDown)
            flashButtonState();

        internalClickCallback (e.mods);
    }

    if (deletionWatcher != nullptr)
        updateState (isMouseSourceOver (e), false);
}

This replaces the old code:

void Button::mouseUp (const MouseEvent& e)
{
    const bool wasDown = isDown();
    const bool wasOver = isOver();

    updateState (isMouseSourceOver (e), false);

    if (wasDown && wasOver && ! triggerOnMouseDown)
    {
        if (lastStatePainted != buttonDown)
            flashButtonState();

        internalClickCallback (e.mods);
    }  
}

Using setTriggeredOnMouseDown(…) doesn’t have the same problem with PopupMenu, the mouseOver state clears for different reasons. Although i wonder if it would be more consistent to make the same change to the mouseDown(…) call.

4

Can we get this patched in develop pls :slight_smile:

5

I think we’d still want to keep the first updateState() call as moving it might cause some subtle bugs due to the ordering of the state change message and internal click callback changing. updateState() will only send a change message if the state has actually changed so we don’t need to worry about duplicate messages, so something like this should work whilst maintaining the old order:

void Button::mouseUp (const MouseEvent& e)
{
    const auto wasDown = isDown();
    const auto wasOver = isOver();
    updateState (isMouseSourceOver (e), false);

    if (wasDown && wasOver && ! triggerOnMouseDown)
    {
        if (lastStatePainted != buttonDown)
            flashButtonState();

        WeakReference<Component> deletionWatcher (this);

        internalClickCallback (e.mods);

        if (deletionWatcher != nullptr)
            updateState (isMouseSourceOver (e), false);
    }
}

I don’t think a similar change is needed in mouseDown() since we always call updateState (true, true); so the second call after the click callback would be redundant.

Does that work for your use case?

6

I’m pretty confident that’ll fix it. As long as that second post-event updateState is triggered. This will be a great fix. Sticky highlights with PopupMenus have been a low level niggle of a problem for years with loads of JUCE projects :slight_smile:

7

Great, I’ve put this on develop now: