func (s *Scroll) ScrollToBottom() not scrolling

[williambrode]

Describe the bug:

I used ScrollToBottom and found it didn't actually scroll to the bottom at all. I was able to work around the issue by calling a couple of the lines that ScrollToBottom calls without the unnecessary stuff:

scrollable.Offset.Y = scrollable.Content.MinSize().Height - scrollable.Size().Height
scrollable.Base.Refresh()

To Reproduce:

Here is the code that creates a container with scrollable text. Using scrollable.ScrollToBottom() didn't work for me (it loads scrolled to the top).

func GetLogsTab(ctx context.Context) fyne.CanvasObject {
	scrollable := container.NewVScroll(widget.NewLabel(config.LogBuffer.String()))
	scrollable.ScrollToBottom()
	return container.NewMax(scrollable)
}

Device (please complete the following information):

• OS: Windows 10
• Version: 10.0.19044
• Go version: 1.16.5
• Fyne version: v2.1.4

[Jacalz]

Do you experience the same thing using the Table widget? I suspect that #2918 might be a duplicate of this issue.

[williambrode]

I haven't tried with table. That other post is confusing as they simply said:
"When the table is short and is displayed in full, then ScrollToBottom fails." But what does fails mean when it wouldn't do anything in the first place?

[Jacalz]

Indeed, it is kind of strange.

[andydotxyz]

I wonder if this relates to whether it is visible or not.
Perhaps we have a bug that before it is rendered it does not know how much to scroll and when it is shown it does not remember that the scroll had been requested.
I guess that if you put a short delay in it may function correctly (not suggesting this, just brain storming).

[williambrode]

Since I have a reproducible case and there isn't much code run, I'll try to debug and find out more.

[williambrode]

@andydotxyz I found that Size() on both content and container were returning 0 (probably as you said because it wasn't rendered yet) which caused the refresh to put the position back to 0. Using the MinSize() if the Size is 0 fixed my issue. Not sure if there are other problems with this fix though:

func (s *Scroll) updateOffset(deltaX, deltaY float32) bool {
	contentWidth := fyne.Max(s.Content.Size().Width, s.Content.MinSize().Width)
	containerWidth := fyne.Max(s.Size().Width, s.MinSize().Width)
	contentHeight := fyne.Max(s.Content.Size().Height, s.Content.MinSize().Height)
	containerHeight := fyne.Max(s.Size().Height, s.MinSize().Height)
	if contentWidth <= containerWidth && contentHeight <= containerHeight {
		if s.Offset.X != 0 || s.Offset.Y != 0 {
			s.Offset.X = 0
			s.Offset.Y = 0
			return true
		}
		return false
	}
...

If you want this fix I can send a PR.

[andydotxyz]

Thanks for this. I think that the required change is probably smaller - it only needs to check that the Size() is non-zero, not the content as well.
Thinking about it though, the MinSize of a scroll container is not very informative, so I am surprised that this works - if the size expands after rendering logically it isn't scrolled to the bottom any more... do you follow?

[williambrode]

Yeah, the Height of MinSize of the scroll container was only 32 - so the Y offset starts out too large. But when we Resize we call Refresh - which calls updateOffset and the code at the end of that does resizing that accounts for a position that goes beyond the limit of the container:

        s.Offset.X = computeOffset(s.Offset.X, -deltaX, s.Size().Width, s.Content.MinSize().Width)
	s.Offset.Y = computeOffset(s.Offset.Y, -deltaY, s.Size().Height, s.Content.MinSize().Height)

So that should correct the Y offset to be container size - content size

[andydotxyz]

Thanks for the explanation. In terms of the minimum change is it possible to handle the edge case in an if (i.e. if x or y is 0 then use min size) so it is easier to understand than Max which implies a different intent.

[williambrode]

Sure, I imagine anything that side-steps the code setting the offset back to 0 would work. I actually thought Max made the most sense because we want to use Size() and it should always be >= MinSize().

[andydotxyz]

I see your logic - but is this not a special case where 0 (or less) is found but not expected?
Max makes sense when we literally expect two reasonable values and want the larger of the two.

[the-xentropy]

For anyone bumping into this, here's a way to accomplish this (adapted from initial post) but also not scroll unnecessarily if the size is OK already , so you can safely call it on every update even if nothing needs to be done.

if (scrollContainer.Content.MinSize().Height) > scrollContainer.Size().Height {
  scrollContainer.Offset.Y = scrollContainer.Content.MinSize().Height - scrollContainer.Size().Height
  scrollContainer.Base.Refresh()
}

Would be nice if this was fixed, obviously, but since this is a helper function I can understand why it's not a high priority.

[lorypelli]

For anyone bumping into this, here's a way to accomplish this (adapted from initial post) but also not scroll unnecessarily if the size is OK already , so you can safely call it on every update even if nothing needs to be done.

if (scrollContainer.Content.MinSize().Height) > scrollContainer.Size().Height {
  scrollContainer.Offset.Y = scrollContainer.Content.MinSize().Height - scrollContainer.Size().Height
  scrollContainer.Base.Refresh()
}

Would be nice if this was fixed, obviously, but since this is a helper function I can understand why it's not a high priority.

Thanks, you helped me fixing the issue...

[lorypelli]

I had to open a new one because I made an error