Add support for clipping items within clipped items (scroll containers)

Fixes fyne-io#1682

CHANGELOG.md

@@ -14,6 +14,7 @@ More detailed release notes can be found on the [releases page](https://github.c
 * Fix possible race conditions for canvas capture
 * Improvements to `fyne get` command downloader
 * Fix tree, so it refreshes visible nodes on Refresh()
+* Incorrect clipping behaviour with nested scroll containers (#1682)
 
 
 ## 1.4.2 - 9 December 2020

internal/clip.go

@@ -0,0 +1,81 @@
+package internal
+
+import "fyne.io/fyne"
+
+// ClipStack keeps track of the areas that should be clipped when drawing a canvas.
+// If no clips are present then adding one will be added as-is.
+// Subsequent items pushed will be completely within the previous clip.
+type ClipStack struct {
+	clips []*ClipItem
+}
+
+// Pop removes the current top clip and returns it.
+func (c *ClipStack) Pop() *ClipItem {
+	if len(c.clips) == 0 {
+		return nil
+	}
+
+	ret := c.clips[len(c.clips)-1]
+	c.clips = c.clips[:len(c.clips)-1]
+	return ret
+}
+
+// Length  returns the number of items in this clip stack. 0 means no clip.
+func (c *ClipStack) Length() int {
+	return len(c.clips)
+}
+
+// Push a new clip onto this stack at position and size specified.
+// The returned clip item is the result of calculating the intersection of the requested clip and it's parent.
+func (c *ClipStack) Push(p fyne.Position, s fyne.Size) *ClipItem {
+	outer := c.Top()
+	inner := outer.Intersect(p, s)
+
+	c.clips = append(c.clips, inner)
+	return inner
+}
+
+// Top returns the current clip item - it will always be within the bounds of any parent clips.
+func (c *ClipStack) Top() *ClipItem {
+	if len(c.clips) == 0 {
+		return nil
+	}
+
+	return c.clips[len(c.clips)-1]
+}
+
+// ClipItem represents a single clip in a clip stack, denoted by a size and position.
+type ClipItem struct {
+	pos  fyne.Position
+	size fyne.Size
+}
+
+// Rect returns the position and size parameters of the clip.
+func (i *ClipItem) Rect() (fyne.Position, fyne.Size) {
+	return i.pos, i.size
+}
+
+// Intersect returns a new clip item that is the intersection of the requested parameters and this clip.
+func (i *ClipItem) Intersect(p fyne.Position, s fyne.Size) *ClipItem {
+	ret := &ClipItem{p, s}
+	if i == nil {
+		return ret
+	}
+
+	if ret.pos.X < i.pos.X {
+		ret.pos.X = i.pos.X
+		ret.size.Width -= i.pos.X - p.X
+	}
+	if ret.pos.Y < i.pos.Y {
+		ret.pos.Y = i.pos.Y
+		ret.size.Height -= i.pos.Y - p.Y
+	}
+
+	if p.X+s.Width > i.pos.X+i.size.Width {
+		ret.size.Width = (i.pos.X + i.size.Width) - ret.pos.X
+	}
+	if p.Y+s.Height > i.pos.Y+i.size.Height {
+		ret.size.Height = (i.pos.Y + i.size.Height) - ret.pos.Y
+	}
+	return ret
+}

internal/clip_test.go

@@ -0,0 +1,62 @@
+package internal
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"fyne.io/fyne"
+)
+
+func TestClipStack_Intersect(t *testing.T) {
+	p1 := fyne.NewPos(5, 25)
+	s1 := fyne.NewSize(100, 100)
+	c := &ClipStack{
+		clips: []*ClipItem{
+			{p1, s1},
+		},
+	}
+
+	p2 := fyne.NewPos(25, 0)
+	s2 := fyne.NewSize(50, 50)
+	i := c.Push(p2, s2)
+
+	assert.Equal(t, fyne.NewPos(25, 25), i.pos)
+	assert.Equal(t, fyne.NewSize(50, 25), i.size)
+	assert.Equal(t, 2, len(c.clips))
+
+	_ = c.Pop()
+	p2 = fyne.NewPos(50, 50)
+	s2 = fyne.NewSize(150, 50)
+	i = c.Push(p2, s2)
+
+	assert.Equal(t, fyne.NewPos(50, 50), i.pos)
+	assert.Equal(t, fyne.NewSize(55, 50), i.size)
+	assert.Equal(t, 2, len(c.clips))
+}
+
+func TestClipStack_Pop(t *testing.T) {
+	p := fyne.NewPos(5, 5)
+	s := fyne.NewSize(100, 100)
+	c := &ClipStack{
+		clips: []*ClipItem{
+			{p, s},
+		},
+	}
+
+	i := c.Pop()
+	assert.Equal(t, p, i.pos)
+	assert.Equal(t, s, i.size)
+	assert.Equal(t, 0, len(c.clips))
+}
+
+func TestClipStack_Push(t *testing.T) {
+	c := &ClipStack{}
+	p := fyne.NewPos(5, 5)
+	s := fyne.NewSize(100, 100)
+
+	i := c.Push(p, s)
+	assert.Equal(t, p, i.pos)
+	assert.Equal(t, s, i.size)
+	assert.Equal(t, 1, len(c.clips))
+}

internal/driver/glfw/canvas.go

@@ -405,6 +405,7 @@ func (c *glCanvas) overlayChanged() {
 }
 
 func (c *glCanvas) paint(size fyne.Size) {
+	clips := &internal.ClipStack{}
 	if c.Content() == nil {
 		return
 	}
@@ -413,18 +414,21 @@ func (c *glCanvas) paint(size fyne.Size) {
 
 	paint := func(node *renderCacheNode, pos fyne.Position) {
 		obj := node.obj
-		// TODO should this be somehow not scroll container specific?
-		if _, ok := obj.(*widget.ScrollContainer); ok {
-			c.painter.StartClipping(
-				fyne.NewPos(pos.X, c.Size().Height-pos.Y-obj.Size().Height),
-				obj.Size(),
-			)
+		if _, ok := obj.(fyne.Scrollable); ok {
+			inner := clips.Push(pos, obj.Size())
+			c.painter.StartClipping(inner.Rect())
 		}
 		c.painter.Paint(obj, pos, size)
 	}
 	afterPaint := func(node *renderCacheNode) {
-		if _, ok := node.obj.(*widget.ScrollContainer); ok {
-			c.painter.StopClipping()
+		if _, ok := node.obj.(fyne.Scrollable); ok {
+			clips.Pop()
+			if top := clips.Top(); top != nil {
+				c.painter.StartClipping(top.Rect())
+			} else {
+				c.painter.StopClipping()
+
+			}
 		}
 	}
 

internal/driver/gomobile/driver.go

@@ -5,22 +5,21 @@ import (
 	"strconv"
 	"time"
 
-	"fyne.io/fyne"
-	"fyne.io/fyne/canvas"
-	"fyne.io/fyne/internal"
-	"fyne.io/fyne/internal/driver"
-	"fyne.io/fyne/internal/painter"
-	pgl "fyne.io/fyne/internal/painter/gl"
-	"fyne.io/fyne/theme"
-	"fyne.io/fyne/widget"
-
 	"github.com/fyne-io/mobile/app"
 	"github.com/fyne-io/mobile/event/key"
 	"github.com/fyne-io/mobile/event/lifecycle"
 	"github.com/fyne-io/mobile/event/paint"
 	"github.com/fyne-io/mobile/event/size"
 	"github.com/fyne-io/mobile/event/touch"
 	"github.com/fyne-io/mobile/gl"
+
+	"fyne.io/fyne"
+	"fyne.io/fyne/canvas"
+	"fyne.io/fyne/internal"
+	"fyne.io/fyne/internal/driver"
+	"fyne.io/fyne/internal/painter"
+	pgl "fyne.io/fyne/internal/painter/gl"
+	"fyne.io/fyne/theme"
 )
 
 const tapSecondaryDelay = 300 * time.Millisecond
@@ -186,6 +185,7 @@ func (d *mobileDriver) onStop() {
 }
 
 func (d *mobileDriver) paintWindow(window fyne.Window, size fyne.Size) {
+	clips := &internal.ClipStack{}
 	canvas := window.Canvas().(*mobileCanvas)
 
 	r, g, b, a := theme.BackgroundColor().RGBA()
@@ -194,19 +194,20 @@ func (d *mobileDriver) paintWindow(window fyne.Window, size fyne.Size) {
 	d.glctx.Clear(gl.COLOR_BUFFER_BIT)
 
 	paint := func(obj fyne.CanvasObject, pos fyne.Position, _ fyne.Position, _ fyne.Size) bool {
-		// TODO should this be somehow not scroll container specific?
-		if _, ok := obj.(*widget.ScrollContainer); ok {
-			canvas.painter.StartClipping(
-				fyne.NewPos(pos.X, canvas.Size().Height-pos.Y-obj.Size().Height),
-				obj.Size(),
-			)
+		if _, ok := obj.(fyne.Scrollable); ok {
+			inner := clips.Push(pos, obj.Size())
+			canvas.painter.StartClipping(inner.Rect())
 		}
 		canvas.painter.Paint(obj, pos, size)
 		return false
 	}
 	afterPaint := func(obj, _ fyne.CanvasObject) {
-		if _, ok := obj.(*widget.ScrollContainer); ok {
+		if _, ok := obj.(fyne.Scrollable); ok {
 			canvas.painter.StopClipping()
+			clips.Pop()
+			if top := clips.Top(); top != nil {
+				canvas.painter.StartClipping(top.Rect())
+			}
 		}
 	}
 

internal/painter/gl/painter.go

@@ -53,7 +53,7 @@ func (p *glPainter) Clear() {
 
 func (p *glPainter) StartClipping(pos fyne.Position, size fyne.Size) {
 	x := p.textureScaleInt(pos.X)
-	y := p.textureScaleInt(pos.Y)
+	y := p.textureScaleInt(p.canvas.Size().Height - pos.Y - size.Height)
 	w := p.textureScaleInt(size.Width)
 	h := p.textureScaleInt(size.Height)
 	p.glScissorOpen(int32(x), int32(y), int32(w), int32(h))