A Widget with {margin: 1} style in a ScrollableContainer causes infinite resizes and scrollbars flicker

[labo-China]

When presenting a ScrollableContainer with a Widget that has styles above in it, there will have a infinite resizes. Example:

from textual.containers import ScrollableContainer
from textual.widget import Widget
from textual.widgets import Static

class SuspiciousWidget(Widget):
    DEFAULT_CSS = 'SuspiciousWidget {margin: 1}'

class A(App):
    def compose(self) -> ComposeResult:
        with ScrollableContainer():
            yield Static()
            yield SuspiciousWidget()


A().run()

Screen Record:

'textual diagnose' result:

Textual Diagnostics

Versions

Name	Value
Textual	0.50.0
Rich	13.7.0

Python

Name	Value
Version	3.11.6
Implementation	CPython
Compiler	GCC 13.2.1 20230801
Executable	/home/labo/prog/NCSbox/venv/bin/python

Operating System

Name	Value
System	Linux
Release	6.7.4-zen1-1-zen
Version	(HASH)1 ZEN SMP PREEMPT_DYNAMIC Mon, 05 Feb 2024 22:07:37 +0000

Terminal

Name	Value
Terminal Application	Unknown
TERM	xterm-256color
COLORTERM	truecolor
FORCE_COLOR	Not set
NO_COLOR	Not set

Rich Console options

Name	Value
size	width=75, height=27
legacy_windows	False
min_width	1
max_width	75
is_terminal	True
encoding	utf-8
max_height	27
justify	None
overflow	None
no_wrap	False
highlight	None
markup	None
height	None

[labo-China]

By the way, this problem happened since Textual 0.48.0. Versions below it are OK.

[TomJGooding]

I found the scrollbars flicker even if you remove the margin - unfortunately this might be a bug introduced with #4037?

[rodrigogiraoserrao]

The underlying error actually seems to go way back.

Take this app:

from textual.app import App, ComposeResult
from textual.widget import Widget

class A(App[None]):
    CSS = """
    Screen {
        overflow: auto auto;
    }
    #one {
        height: 1;
    }
    #two {
        height: 100%;
    }
    """

    def compose(self) -> ComposeResult:
        yield Widget(id="one")
        yield Widget(id="two")

if __name__ == "__main__":
    A().run()

At v0.19.0 this app doesn't generate this flickering error but it does at v0.19.1.

[rodrigogiraoserrao]

The source of the issue was commit 4f7b2d0.

Reverting that back now reveals two failed tests:

FAILED tests/snapshot_tests/test_snapshots.py::test_css_property[min_width.py] - AssertionError: assert False
FAILED tests/snapshot_tests/test_snapshots.py::test_auto_table - AssertionError: assert False

It looks like the reason those two tests fail is that some unnecessary scrollbar gutters stick around on app startup.

[rodrigogiraoserrao]

Ok, I've banged my head against the wall for a while and now I understand what the source of the issue is / what needs to be done.

Consider the app below and suppose the terminal size is 80 x 24.

from textual.app import App, ComposeResult
from textual.widget import Widget

class A(App[None]):
    CSS = """
    Screen {
        overflow: auto auto;
    }
    #one {
        height: 1;
    }
    #two {
        height: 100%;
    }
    """

    def compose(self) -> ComposeResult:
        yield Widget(id="one")
        yield Widget(id="two")

if __name__ == "__main__":
    A().run()

When the two widgets are mounted the screen is reflowed and we eventually end up at Widget._refresh_scrollbars. At this time, #one is set to have a height of 1 and #two is set to have a height of 24. Because the heights of these two are greater than 24, which is the virtual height, we determine that we will (rightfully) need a vertical scrollbar:

https://github.com/Textualize/textual/blob/c7370f33203497d15ce65528d6f92ffbc58200a0/src/textual/widget.py#L1463-L1464

A bit further down, we have a second check to see if we need a horizontal scrollbar. Before, the widths of the widgets were at 80 (100% of the container) which fits perfectly, so we didn't need a scrollbar. But now, we do a second check because the horizontal space just decreased a bit. This will make it look like there is need for a horizontal scrollbar:

https://github.com/Textualize/textual/blob/c7370f33203497d15ce65528d6f92ffbc58200a0/src/textual/widget.py#L1467-L1470

However, this isn't needed because the children of this container have relative sizes and as soon as the vertical scrollbar gets added, they will be resized to take up less horizontal space. Therefore, a possible fix is to change the check shown above and make sure we only show the horizontal if the children cannot be resized to fit within the slightly reduced space.

However, this isn't all. Suppose that we add width: 30 to our two widgets. Now, their widths are way under the 80 terminal width but the flickering will still occur. Why?

Again, when we get to _refresh_scrollbars, we'll have that the virtual size of the screen is 80 x 24. The heights will be too big and so we'll need a vertical scrollbar. Then, in the second check for the horizontal scrollbar, we'll have the same situation because the virtual width of the screen will be 80. In this case, we won't need a horizontal scrollbar because the children are short enough and because it's only the virtual width of the screen itself that is making it look like we're running out of space, when the virtual width of the screen will be happy to go down to 68 to accomodate the scrollbars.

[rodrigogiraoserrao]

Given the above, it looks like the fix is to go through the children of the widget that is having its scrollbars refreshed and checking if any of them is:

1. exactly the size of the dimension we're looking at; and
2. if that dimension can be updated automatically.

A rough implementation of this (that probably doesn't handle edge cases) would be to replace

https://github.com/Textualize/textual/blob/c7370f33203497d15ce65528d6f92ffbc58200a0/src/textual/widget.py#L1467-L1470

with something like

        if overflow_x == "auto" and show_vertical and not show_horizontal:
            show_horizontal = any(
                child.size.width == width
                and child.styles.width
                and (child.styles.width.is_cells or child.styles.width.is_auto)
                for child in self.children
            )

Running the tests shows a single failing test that has to do with RichLog. Rich logs have no children and whether they get scrollbars or not depends on the content written to the rich log. Adding an extra checks seems to work:

        if overflow_x == "auto" and show_vertical and not show_horizontal:
            if self.children:
                show_horizontal = any(
                    child.size.width == width
                    and child.styles.width
                    and (child.styles.width.is_cells or child.styles.width.is_auto)
                    for child in self.children
                )
            else:
                show_horizontal = self.virtual_size.width > (
                    width - styles.scrollbar_size_vertical
                )

However this feels like patching the symptom instead of fixing the underlying issue. Perhaps, widgets like RichLog – where the existence of scrollbars doesn't depend on sub-widgets but on the content itself – should grow needs_horizontal_scrollbar and needs_vertical_scrollbar methods that can be called in this situation.

[github-actions[bot]]

Don't forget to star the repository!

Follow @textualizeio for Textual updates.