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

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

Closed
labo-China opened this issue Feb 9, 2024 · 7 comments · Fixed by #4315
Assignees
Labels
bug Something isn't working Task

Comments

@labo-China
Copy link

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:
Peek 2024-02-09 16-54

'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
Copy link
Author

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

@TomJGooding
Copy link
Contributor

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

@Textualize Textualize deleted a comment from github-actions bot Feb 27, 2024
@rodrigogiraoserrao rodrigogiraoserrao self-assigned this Feb 27, 2024
@rodrigogiraoserrao
Copy link
Contributor

rodrigogiraoserrao commented Feb 27, 2024

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 rodrigogiraoserrao added bug Something isn't working Task labels Feb 27, 2024
@rodrigogiraoserrao
Copy link
Contributor

rodrigogiraoserrao commented Feb 27, 2024

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
Copy link
Contributor

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:

textual/src/textual/widget.py

Lines 1463 to 1464 in c7370f3

elif overflow_y == "auto":
show_vertical = self.virtual_size.height > height

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:

textual/src/textual/widget.py

Lines 1467 to 1470 in c7370f3

if overflow_x == "auto" and show_vertical and not show_horizontal:
show_horizontal = self.virtual_size.width > (
width - styles.scrollbar_size_vertical
)

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
Copy link
Contributor

rodrigogiraoserrao commented Mar 12, 2024

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

textual/src/textual/widget.py

Lines 1467 to 1470 in c7370f3

if overflow_x == "auto" and show_vertical and not show_horizontal:
show_horizontal = self.virtual_size.width > (
width - styles.scrollbar_size_vertical
)

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.

Copy link

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants