Buttons "normal" stylebox content margins override other styleboxes content margins

[Atovange]

Godot version

v4.1.3.stable.official [f06b6836a]

System information

Godot v4.1.3.stable - Windows 10.0.22621 - Vulkan (Compatibility) - AMD Radeon RX 6600 (Advanced Micro Devices, Inc.; 31.0.21029.6002) - AMD Ryzen 5 5600 6-Core Processor (12 Threads)

Issue description

https://github.com/godotengine/godot/assets/32620115/f1a53d34-e003-464b-9f4c-d1fde1c0b59f

Adding a "normal" stylebox is affecting the content margins while the button is in "pressed" mode, which should only be affected by "pressed" styleboxes.

In this case the "normal" stylebox has more content margin than the "pressed" one.

Steps to reproduce

Setup a button and two styleboxes with different content margins

Minimal reproduction project

Trivial reproduction

[Calinou]

I think this is expected, otherwise your other buttons would suddenly reflow when a button is hovered, pressed or focused. This leads to poor user experience as buttons would suddenly start moving for no apparent reason.

If you want to enlarge a button when hovered, use expand margins instead of content margins. This could be coupled with https://github.com/godotengine/godot-proposals/issues/4338 to make buttons have a larger hover area that matches the expanded area when the button is hovered.

[Atovange]

Ok I get it though I would personally prefer to have the freedom to either have it behave like this (by setting the content margins of all styleboxes to the same) or make each content margin independent, which would help in my use case of wanting a button to move down when pressed without using a script.

Should I close the issue?

[Calinou]

Should I close the issue?

No, as this behavior should be documented.

[YuriSizov]

I think this is expected, otherwise your other buttons would suddenly reflow when a button is hovered, pressed or focused. This leads to poor user experience as buttons would suddenly start moving for no apparent reason.

We do this in some other controls, so I don't think we necessarily abide by this logic. I would say this is something that should be safely left in the hands of the developer rather than enforced by the engine. In my experience it always seems like an oversight when I find such logic in the codebase.

[kaluluosi]

I have found out why.

void Button::_notification(int p_what) {
                       ....

                        // Here style variable is assigned to normal stylebox
            Ref<StyleBox> style = theme_cache.normal; 
            bool rtl = is_layout_rtl();
            const bool is_clipped = clip_text || overrun_behavior != TextServer::OVERRUN_NO_TRIMMING;
            // And unbelivable, there is no mode called "DRAW_FOCUS".
                       // It means Focus is parallel with other states.
                       // While you focus on button , the draw_mode still act on it mode.
            switch (get_draw_mode()) {
                case DRAW_NORMAL: {
                    if (rtl && has_theme_stylebox(SNAME("normal_mirrored"))) {
                        style = theme_cache.normal_mirrored;
                    } else {
                        style = theme_cache.normal;
                    }

                    if (!flat) {
                        style->draw(ci, Rect2(Point2(0, 0), size));
                    }

                    // Focus colors only take precedence over normal state.
                    if (has_focus()) {
                        color = theme_cache.font_focus_color;
                        if (has_theme_color(SNAME("icon_focus_color"))) {
                            color_icon = theme_cache.icon_focus_color;
                        }
                    } else {
                        color = theme_cache.font_color;
                        if (has_theme_color(SNAME("icon_normal_color"))) {
                            color_icon = theme_cache.icon_normal_color;
                        }
                    }
                } break;
                case DRAW_HOVER_PRESSED: {
                    // Edge case for CheckButton and CheckBox.
                    if (has_theme_stylebox("hover_pressed")) {
                        if (rtl && has_theme_stylebox(SNAME("hover_pressed_mirrored"))) {
                            style = theme_cache.hover_pressed_mirrored;
                        } else {
                            style = theme_cache.hover_pressed;
                        }

                        if (!flat) {
                            style->draw(ci, Rect2(Point2(0, 0), size));
                        }
                        if (has_theme_color(SNAME("font_hover_pressed_color"))) {
                            color = theme_cache.font_hover_pressed_color;
                        }
                        if (has_theme_color(SNAME("icon_hover_pressed_color"))) {
                            color_icon = theme_cache.icon_hover_pressed_color;
                        }

                        break;
                    }
                    [[fallthrough]];
                }
                case DRAW_PRESSED: {
                    if (rtl && has_theme_stylebox(SNAME("pressed_mirrored"))) {
                        style = theme_cache.pressed_mirrored;
                    } else {
                        style = theme_cache.pressed;
                    }

                    if (!flat) {
                        style->draw(ci, Rect2(Point2(0, 0), size));
                    }
                    if (has_theme_color(SNAME("font_pressed_color"))) {
                        color = theme_cache.font_pressed_color;
                    } else {
                        color = theme_cache.font_color;
                    }
                    if (has_theme_color(SNAME("icon_pressed_color"))) {
                        color_icon = theme_cache.icon_pressed_color;
                    }

                } break;
                case DRAW_HOVER: {
                    if (rtl && has_theme_stylebox(SNAME("hover_mirrored"))) {
                        style = theme_cache.hover_mirrored;
                    } else {
                        style = theme_cache.hover;
                    }

                    if (!flat) {
                        style->draw(ci, Rect2(Point2(0, 0), size));
                    }
                    color = theme_cache.font_hover_color;
                    if (has_theme_color(SNAME("icon_hover_color"))) {
                        color_icon = theme_cache.icon_hover_color;
                    }

                } break;
                case DRAW_DISABLED: {
                    if (rtl && has_theme_stylebox(SNAME("disabled_mirrored"))) {
                        style = theme_cache.disabled_mirrored;
                    } else {
                        style = theme_cache.disabled;
                    }

                    if (!flat) {
                        style->draw(ci, Rect2(Point2(0, 0), size));
                    }
                    color = theme_cache.font_disabled_color;
                    if (has_theme_color(SNAME("icon_disabled_color"))) {
                        color_icon = theme_cache.icon_disabled_color;
                    } else {
                        color_icon.a = 0.4;
                    }

                } break;
            }

                        // And Here， seems that programmer just want focus style box  to draw border  box.
                        // It create a style2 and draw it itself. It no override style varibale.
                        // So focus stylebox is only use to draw box.
            if (has_focus()) {
                Ref<StyleBox> style2 = theme_cache.focus;
                style2->draw(ci, Rect2(Point2(), size));
            }

                        // Below , use the style to draw content (icon and text).
                        // So the content about properties of focust stylebox not effect the icon and text.
                        ...

The Focus state is a superimposed state parallel with other states. When in Focus, it's also possible to enter states like Hover Pressed. Handling the superimposed Focus state can indeed be tricky, as inattentiveness can easily result in the Focus state continuously overriding other states.

[zhengxiaoyao0716]

I found this problem still exist on 4.2 version. I used some images as the StyleBoxTexture of the styles, but the image of the focus status would override all the other status.