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

Horizontal scroll bar disappeared in 0.87.1 #5284

Closed
pablogsal opened this issue Nov 25, 2024 · 20 comments · Fixed by #5296
Closed

Horizontal scroll bar disappeared in 0.87.1 #5284

pablogsal opened this issue Nov 25, 2024 · 20 comments · Fixed by #5296

Comments

@pablogsal
Copy link
Contributor

We have a bunch of test failures due to textual in the 0.87.1 version. It seems that textual 0.87.1 has some regression that makes the horizontal scroll bar disappear. Here is some screenshots from our

BEFORE

Screenshot 2024-11-25 at 15 56 01

AFTER

Screenshot 2024-11-25 at 15 56 15

This also happens in the main memray app and makes impossible for users to scroll horizontally the table:

Screenshot 2024-11-25 at 15 57 10

Copy link

Thank you for your issue. Give us a little time to review it.

PS. You might want to check the FAQ if you haven't done so already.

This is an automated reply, generated by FAQtory

@pablogsal
Copy link
Contributor Author

@pablogsal
Copy link
Contributor Author

After some investigation seems that the horizontals scrollbar only appears when the external scrollbar is at the bottom:
Screenshot 2024-11-25 at 15 59 19

and after scrolling:
Screenshot 2024-11-25 at 15 59 31

Some users have mentioned that this is a bit confusing because unless you scroll the second scrollbar in our app it looks like you can't scroll horizontally.

@willmcgugan
Copy link
Collaborator

I can't think of anything off the top of my head that would cause this. What was the last known working version?

@pablogsal
Copy link
Contributor Author

I think this happened after #5000

@pablogsal
Copy link
Contributor Author

What was the last known working version?

0.81

pablogsal added a commit to pablogsal/memray that referenced this issue Nov 25, 2024
The new textual release has some changes:

* There are some changes that we need to do to get the rendered version
  of some textual objects as now render() returns RichVisual instances
* Textual has a new theme so we need to regenerate our snapshots.
* There is something going on with horizontal scroll bars. We have
  opened Textualize/textual#5284 but looks
  like this may be expected.
@willmcgugan
Copy link
Collaborator

That release did unfortunately break snapshots, but I don't think that is the issue here.

You have the following rule in your TCSS:

.narrow AllocationTable {
  height: 100%;
}

It looks like narrow class is added when the width is less than 80, to make the UI responsive.

The rule makes the table 100% of the container. There are other things in the container, so the total height is greater than 100%, which forces the screen to scroll. If the table also has to scroll, then you would get the confusing double scrollbars.

If you remove that rule, then it works more naturally. Maybe add a keybinding to maximize the table?

@godlygeek
Copy link

Ah, the change here isn't whether or not the horizontal scrollbar is visible, it's the position of the vertical scrollbar, and how focusing widgets works. The horizontal scrollbar for the grid was never visible when the outermost vertical scrollbar was scrolled up, but we used to wake up with the AllocationTable focused and the outermost vertical scrollbar scrolled to the bottom:

image

And now we wake up with that vertical scrollbar scrolled to the top instead:

image

Bisecting tells me that this change came in e157c6b - you can reproduce for yourself with a pip install memray and then

memray run --live -c "import mmap; mmap.mmap(-1, 4096)"

in a 80x24 terminal window.

Moreover, pressing <Tab> should toggle between having the AllocationTable focused and having the Header focused. Prior to e157c6b the first time you pressed <Tab> the outermost vertical scrollbar would jump to the top (scrolling to the start of the Header widget), and pressing it a second time would cause the outermost vertical scrollbar to jump to the bottom (scrolling to the start of the AllocationTable widget, and returning what was shown when the TUI first started).

After e157c6b the first time you press <Tab> has no visual effect whatsoever - the outer vertical scrollbar doesn't move at all. But pressing it a second time causes the AllocationTable to be focused and scrolled to the way we want it to be, and future tabs toggle between the two widgets as we'd expect.

@godlygeek
Copy link

The AllocationTable calls table.focus() in its compose(), which is the thing that has up until now been successfully causing the .narrow styled TUI to wake up with the AllocationTable taking up 100% of the screen on wakeup, with the outer vertical scrollbar scrolled to the bottom. But e157c6b seems to have broken that (by delaying the resize, I suppose?)

@willmcgugan
Copy link
Collaborator

Quite possibly. The compose method is generally too early for UI updates. Could you set AUTO_FOCUS="AllocationTable" on your screen?

@godlygeek
Copy link

That doesn't seem to work. I've added

*:focus {
  background: magenta;
}

to the TCSS to make it immediately obvious which widget is focused, and putting AUTO_FOCUS = "DataTable" into the screen (DataTable, not AllocationTable, since the .focus() we were calling explicitly up until now was on the DataTable contained within the AllocationTable), I see the exact same behavior as we're getting from calling .focus() - the data table widget is focused, but we're not scrolled to it as we should be.

@godlygeek
Copy link

godlygeek commented Nov 25, 2024

In fact, AUTO_FOCUS doesn't do what we want even in older versions of Textual - it does focus the widget, but doesn't seem to scroll so that the widget takes up 100% of the screen like we want (and like manually tabbing to focus the widget does), even before e157c6b.

@godlygeek
Copy link

Any other suggestions, @willmcgugan?

@willmcgugan
Copy link
Collaborator

What appears to be happening is that the resize event is processed after the widget has been given focus. I'm not sure what the answer is to that ATM. A few options.

In the meantime, you could use this workaround:

Remove the call focus inside compose, and add the following method:

    def on_show(self) -> None:
        self.call_after_refresh(self.query_one("DataTable").focus)

That should ensure that when the app runs in a narrow window, it will focus and scroll to it.

@godlygeek
Copy link

Prior to e157c6b that seems to work 100% of the time, but after e157c6b that seems to sometimes work, and sometimes not. It seems to be racy... the widget always winds up focused, but it's only scrolled to about 75% of the time in my testing.

Presumably it's still the same issue - the focus() call is sometimes happening before the data table has been resized to the height we want...

@godlygeek
Copy link

And unfortunately it seems to work 100% of the time if I textual run --dev, which reinforces the idea that it's still racy, but makes it harder to confirm...

@TomJGooding
Copy link
Contributor

Here's my attempt at creating an MRE in case it helps someone...

from textual import events
from textual.app import App, ComposeResult
from textual.screen import Screen
from textual.widgets import Static


class Placeholder(Static, can_focus=True):
    DEFAULT_CSS = """
    Placeholder {
        &:focus {
            background: $success;
        }
    }
    """


class ExampleScreen(Screen):
    def compose(self) -> ComposeResult:
        yield Placeholder("Header", id="header")
        yield Placeholder("Table", id="table")

    def on_show(self) -> None:
        self.call_after_refresh(self.query_one("#table").focus)


class ExampleApp(App):
    CSS = """
    #header {
      height: 7;
    }

    .narrow #header {
      height: 11;
    }

    #table {
      height: 1fr;
    }

    .narrow #table {
      height: 100%;
    }
    """

    def on_mount(self) -> None:
        self.push_screen(ExampleScreen())

    def on_resize(self, event: events.Resize) -> None:
        self.set_class(0 <= event.size.width < 81, "narrow")


if __name__ == "__main__":
    app = ExampleApp()
    app.run()

@willmcgugan
Copy link
Collaborator

The App resize event is being sent after the auto focus. I'm working on a fix.

A workaround, which won't require the call_after_refresh would be to move the resize handler from App to your Screen. Or you could wait a day or two for a new release.

Copy link

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

@godlygeek
Copy link

I can confirm that #5296 fixes this for us. Thanks for the help!

godlygeek pushed a commit to pablogsal/memray that referenced this issue Nov 27, 2024
The new textual release has some changes:

* We need to change how our tests get the rendered version of some
  Textual objects as now `render()` returns `RichVisual` instances.
* Textual has a new theme so we need to regenerate our snapshots.
* There was a regression in how focusing a widget while the app is still
  starting up behaves. This has been fixed upstream in
  Textualize/textual#5284 but our snapshots
  will not pass until the fix makes it into a release.

Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
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 a pull request may close this issue.

4 participants