Closed rodrigogiraoserrao closed 1 year ago
An interesting finding comes from comparing a similar app that sets scrollbar_gutter
to "stable"
on click (see app below).
The two apps signal that the screen needs to be refreshed, but the gutter app refresh will eventually call Widget._size_updated
on the Vertical
and the virtual_size
passed in as an argument will not match the current Vertical.virtual_size
, which is what ultimately triggers the call to _scroll_update
that prints the scrollbars.
On the other hand, the overflow app will still call the Widget._size_updated
on the Vertical
, but the virtual size will be unchanged, which makes me think that the bug might be in how the size of Vertical
is computed.
from textual.app import App
from textual.containers import Vertical
from textual.widgets import Label
class MyApp(App[None]):
def compose(self):
yield Vertical(Label("bananas!"))
def on_mount(self):
self.query_one(Label).styles.width = "100%"
self.query_one(Label).styles.background = "red"
def on_click(self):
self.query_one(Vertical).styles.scrollbar_gutter = "stable"
if __name__ == "__main__":
MyApp().run()
I found a partial fix assuming the overflow_x
and overflow_y
properties have been initialised in styles.py
as in 19780db, forcing a layout (and children) refresh when the property is set.
To do the partial fix, all it takes is to go into widget.py
and edit this section:
This must be edited to assume that scrollbars are shown if they were already flagged as to be shown OR if the respective overflow is set to "scroll"
:
def _get_scrollable_region(self, region: Region) -> Region:
# ...
show_horizontal = show_horizontal_scrollbar or (
self.styles.overflow_x == "scroll"
)
show_vertical = show_vertical_scrollbar or (self.styles.overflow_y == "scroll")
if show_horizontal and show_vertical:
(region, _, _, _) = region.split(
-scrollbar_size_vertical,
-scrollbar_size_horizontal,
)
elif show_vertical:
region, _ = region.split_vertical(-scrollbar_size_vertical)
elif show_horizontal:
region, _ = region.split_horizontal(-scrollbar_size_horizontal)
return region
To verify this works, use the app shown below. Run the app and press s
to set the horizontal overflow to "scroll"
. Notice how the horizontal scrollbar gutter is now shown. Try pressing h
to set the horizontal overflow to "hidden"
and notice how nothing apparent happens. Try pressing r
to force a full layout refresh of the screen and nothing happens. Only by resizing the app will we see the scrollbar gutter disappear.
It seems that some methods that have to do with scrollbars are being called before the scrollbars are refreshed.
Consider all of these statements that were found to be true:
self._refresh_scrollbars()
as the very first statement of _get_scrollable_region
fixes our problems."scroll"
nor the value "hidden"
trigger a call to _refresh_scrollbars
."scroll"
triggers a call to _refresh_scrollbars
but only AFTER we've been to _get_scrollable_region
and faked the value of show_horizontal_scrollbar
but then setting back to "hidden"
DOES NOT trigger such a call to _refresh_scrollbars
.There must be a fault in the logic that decides when to call _refresh_scrollbars
and it is highly unlikely that the fix for this issue includes the edit shown in the previous comment.
As it stands, the only location that calls the method Widget._refresh_scrollbars
is the method Widget._scroll_update
around line 2130.
This must be called from Widget._size_updated
. (Because the only other call is in ScrollView._size_updated
and we are not dealing with widgets that inherit from ScrollView
.)
On the other hand, Widget._size_updated
is being called from Widget._refresh_layout
.
I did lots of digging and investigation and I managed to sprinkle some print
statements that led me to conclude that the fix is likely to be adding a call to _refresh_scrollbars
at the top of _get_scrollable_region
.
Sadly, I can't articulate very well what led me to this conclusion, but it is in part due to the findings that follow.
After many prints and following the code flow, I figured I wanted to keep track of the only call to compositor.reflow
in the whole codebase, which happens inside Screen._refresh_layout
.
I figured this out because later in _refresh_layout
we call the function _size_updated
, and that call takes the virtual size of the widget as an argument.
Now, for the scrollbars to start/stop displaying when we change the overflow_x
/overflow_y
, I needed that virtual size to have been updated properly and that depends on _get_scrollable_region
, because the virtual size is smaller if we have to have space for scrollbars.
So, tracking down the call to reflow
, I went down to the call to Compositor._arrange_root
which is were size calculations are actually made.
It is _inside _arrange_root
_ that we call _get_scrollable_region
to help figure out how much space a given widget has.
Now, the problem is that _get_scrollable_region
assumes our scrollbar status is completely updated, but it isn't. That is why the partial fix above works: because we force _get_scrollable_region
to take into account whether or not the overflow style has been changed to "scroll"
in between last call to _refresh_scrollbars
and now.
Similarly, we can get a full fix by also checking if the overflow style has now been changed to "hidden"
in between the last call to _refresh_scrollbars
and now.
Something like what is shown here, but in a better Python style:
def _get_scrollable_region(self, region: Region) -> Region:
# ...
show_horizontal_scrollbar |= self.styles.overflow_x == "scroll"
show_horizontal_scrollbar &= self.styles.overflow_x != "hidden"
show_vertical_scrollbar |= self.styles.overflow_y == "scroll"
show_vertical_scrollbar &= self.styles.overflow_y != "hidden"
if show_horizontal_scrollbar and show_vertical_scrollbar:
(region, _, _, _) = region.split(
-scrollbar_size_vertical,
-scrollbar_size_horizontal,
)
elif show_vertical_scrollbar:
region, _ = region.split_vertical(-scrollbar_size_vertical)
elif show_horizontal_scrollbar:
region, _ = region.split_horizontal(-scrollbar_size_horizontal)
return region
So, the fix is either adding this extra check in _get_scrollable_region
, or forcing a full scrollbar refresh with a call to _refresh_scrollbars
.
In the next comment I will share some instructions in case you want to see the final debugging that I made that helped me reach this conclusion.
Check this comment if you want to reproduce my final debugging steps:
myapp_overflow.py
.src/textual/_compositor.py
with this.src/textual/screen.py
with this.src/textual/widget.py
with this.Experiment A:
overflow_x
/overflow_y
won't even trigger a layout refresh.textual console -x EVENT
in it.textual run --dev myapp_overflow.py
.r
(to force a full layout refresh with a "private" method)
s
to set overflow_x
to "scroll"
and notice that you get the exact same prints: the same structure and the exact same values.r
again and notice the same prints as before, with exactly the same structure but with a tiny difference in the height
of some of the Region
objects that are printed.h
to set overflow_x
to "hidden"
and notice that you get the exact same prints as the ones you did just now.Experiment B:
Exact same setup, but include the partial fix of checking if the overflow is "scroll"
inside Widget._get_scrollable_region
(but don't check for "hidden"
).
r
to get the baseline set of prints.s
to set overflow_x
to "scroll"
and you will get many more prints this time.You care about the sections enclosed in the green prints that start with "Refreshing" and end with "Done.". The first of those that happened after you pressed s
will have a red print from within _size_updated
which gets printed because the virtual size of the Vertical
was computed correctly thanks to the correct return value of _get_scrollable_region
.
If you look up, you can see the call to _get_scrollable_region
and its arguments in the line that starts with > _get_scrollable_region
and you can see the return value in the line that starts with < _get_scrollable_region
. Notice that the Region
in and the Region
out are almost identical, up the the height
value that decreased slightly because the horizontal scrollbar was detected. Because this is the first time this difference happens,
_size_updated
to _scroll_update
Vertical.virtual_size
being updated (find the debug print that says "Virtual size from ..."r
to get the standard set of prints when nothing happens.h
to set overflow_x
to "hidden"
and nothing happens, the prints are the same as the ones from the previous step, and the Region
that comes out of _get_scrollable_region
is still shorter than the Region
that goes in (in height
) because the app hasn't picked up the fact that there are no scrollbars.Don't forget to star the repository!
Follow @textualizeio for Textual updates.
If you programmatically change the styles
overflow_x
oroverflow_y
, the layout of the app doesn't get updated properly and the scrollbar is likely to stick around (or not show, depending on what setting you had and what you changed it to).For example, if you run the app below, the horizontal scrollbar starts by being shown because
overflow-x: scroll
is set in the default CSS. If you click the app anywhere, the handler will setoverflow_x = "hidden"
for the container and yet the scrollbar will remain there. The scrollbar only disappears after you force a full refresh of the app, for example by resizing the terminal window.This is very similar to #1607 but there is something extra here because the strategy employed in #1610 to fix #1607 doesn't cut it for
overflow_x
/overflow_y
. I'm opening this issue to keep track of my investigation and to keep different things separated.