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

fix for flicker #4315

Merged
merged 8 commits into from
Mar 23, 2024
Merged

fix for flicker #4315

merged 8 commits into from
Mar 23, 2024

Conversation

willmcgugan
Copy link
Collaborator

@willmcgugan willmcgugan commented Mar 20, 2024

Fixes #4141

Fix for flicker where scrollbars "ping pong" between two states. The root of the problem was where both scrollbars were "auto".

Removed the "stabilized_scrollbar" attribute. That may have worked at some point, but didn't really do anything.

@willmcgugan willmcgugan marked this pull request as draft March 20, 2024 11:50
@willmcgugan willmcgugan marked this pull request as ready for review March 20, 2024 12:08
Copy link
Contributor

@rodrigogiraoserrao rodrigogiraoserrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't handle the cases where both scrollbars are auto and they both need to appear and the second one only appears on the second pass (the one that's guarded behind the extra if this PR adds).
For example, take the app below:

from textual.app import App, ComposeResult
from textual.widgets import Label


class A(App[None]):
    CSS = """
    Screen {
        overflow: auto auto;
    }
    #one, #two {
        width: 80;
    }
    #one {
        height: 1;
        background: red;
    }

    #two {
        height: 100%;
        background: blue;
    }
    """

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


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

If you set your terminal to have width equal to 80 (or set width: xxx to the width of your terminal) then we should have a vertical scrollbar and a horizontal one.
If you make the terminal wider, then you have only the vertical scrollbar which is correct.
If you shorten the terminal, then the horizontal scrollbar shows up but the vertical one disappears!
The recording below shows this.

Screen.Recording.2024-03-20.at.15.12.53.mov

@willmcgugan
Copy link
Collaborator Author

#executivedecision

@rodrigogiraoserrao If you can break this, let me know.

@willmcgugan willmcgugan merged commit 9b6ce77 into main Mar 23, 2024
20 checks passed
@willmcgugan willmcgugan deleted the ping-pong-fix branch March 23, 2024 11:11
@rodrigogiraoserrao
Copy link
Contributor

rodrigogiraoserrao commented Mar 25, 2024

It's definitely an improvement but now there's flickering... 😢

E.g., run this app and press A. More often than not you can see the horizontal scrollbar flicker.
Do I open a follow-up issue?

Like I said before, I'm sure there's an issue with the way in which we compute whether scrollbars are needed or not. 🤷

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")

    def key_a(self) -> None:
        self.mount(Widget(id="two"))


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

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

Successfully merging this pull request may close these issues.

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