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

Programatic scrolling should work without scrollbars (overflow hidden). #1201

Closed
willmcgugan opened this issue Nov 17, 2022 · 7 comments · Fixed by #1550
Closed

Programatic scrolling should work without scrollbars (overflow hidden). #1201

willmcgugan opened this issue Nov 17, 2022 · 7 comments · Fixed by #1550
Assignees
Labels
enhancement New feature or request Task

Comments

@willmcgugan
Copy link
Collaborator

Currently if there are no scrollbars, and you call Widget.scroll_to, the scrolling us suppressed. This is so that moving the scroll wheel doesn't scroll when the CSS has disabled scrolling.

However, some times you may to scroll a container programatically. Suggest we need to differentiate scrolling initiated from mouse / keyboard input and programatic scrolling.

@davep davep added enhancement New feature or request Task labels Jan 10, 2023
@davep
Copy link
Contributor

davep commented Jan 11, 2023

Quick test code to visually confirm the issue (using scroll_to_widget, which I imagine is part of this issue too?):

from textual.app        import App, ComposeResult
from textual.containers import Vertical
from textual.widgets    import Header, Footer, Button
from textual.binding    import Binding

class ScrollToTester( App[ None ] ):

    CSS = """
    Vertical {
        overflow-y: hidden;
    }

    Button {
        margin: 1;
        width: 100%;
    }
    """

    BINDINGS = [
        Binding( "j", "jump", "Jump to a button off the screen" ),
    ]

    def compose( self ) -> ComposeResult:
        yield Header()
        yield Vertical(
            *[ Button( str( n ), id=f"btn-{n}" ) for n in range( 200 ) ]
        )
        yield Footer()

    def action_jump( self ) -> None:
        btn = self.query_one( "#btn-150", Button )
        self.query_one( Vertical ).scroll_to_widget( btn )
        btn.focus()

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

Pressing j should bring button 150 into view.

@davep
Copy link
Contributor

davep commented Jan 12, 2023

Note to self: diving into the code, in the above situation, scroll_to gets called and returns True; so as far as it's concerned it did scroll. Whatever needs changing, it looks like it's down in the animation code, most likely.

@davep
Copy link
Contributor

davep commented Jan 12, 2023

Tracing further, a quick fix to this is this:

diff --git a/src/textual/widget.py b/src/textual/widget.py
index 7808f1a0..67c140b7 100644
--- a/src/textual/widget.py
+++ b/src/textual/widget.py
@@ -811,8 +811,8 @@ class Widget(DOMNode):
     def watch_scroll_y(self, old_value: float, new_value: float) -> None:
         if self.show_vertical_scrollbar:
             self.vertical_scrollbar.position = round(new_value)
-            if round(old_value) != round(new_value):
-                self._refresh_scroll()
+        if round(old_value) != round(new_value):
+            self._refresh_scroll()

     def validate_scroll_x(self, value: float) -> float:
         return clamp(value, 0, self.max_scroll_x)

I'm not sure at the moment if this is the way to do it, but this has the effect I'm after here and would seem to narrow it down to watch_scroll_y (likely also need to consider the same for watch_scroll_x too).

@davep
Copy link
Contributor

davep commented Jan 12, 2023

Testing/checking more, this does seem to have exactly the effect we want, but confirming it with @willmcgugan he's said it would possibly be beneficial to have a method of optionally blocking by-code scroll as well (the above change would still block input-based scrolling, but always allow by-code scrolling).

Need to find a bit of time to dive into this further to understand the use-case for that.

Meanwhile I'll draft a PR with the above changes as a placeholder for what we want.

davep added a commit to davep/textual that referenced this issue Jan 12, 2023
See Textualize#1201.

See
Textualize#1201 (comment)
though -- there has been internal discussion about how there may be a need
to even inhibit by-code scrolling (as opposed to by-input) scrolling, in
some situations. This would likely require that there's something
higher-level, perhaps, that doesn't even attempt to animate `scroll_x` or
`scroll_y` in the first place.

Consider this a placeholder approach for the moment.
@davep
Copy link
Contributor

davep commented Jan 12, 2023

Following on a little more, it's been noted that Widget has allow_horizontal_scroll and allow_vertical_scroll properties, which watch_scroll_x and watch_scroll_y are bypassing. It would seem that they should make use of them.

@davep
Copy link
Contributor

davep commented Jan 12, 2023

Further to the above: in further discussion, it's been suggested that all of the scroll_-prefixed method that have the side-effect of making a scroll happen should have a force keyword parameter, which will be used to override the relevant allow_ property. Following this through, to help try and grok this, let's look at what would happen if scroll_visible where called.

I still need to check other paths but I imagine that all roads lead to scroll_to. The next thing to figure out is how we can know what direction we're scrolling in at this point. Both x and y can be None to indicate "no change" but at this point I'm not confidant that that's all there is to this.

davep added a commit to davep/textual that referenced this issue Jan 12, 2023
@github-actions
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
enhancement New feature or request Task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants