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

Prevent viewport.showOnScreen from scrolling the viewport if the specified Rect is already visible. #56413

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
652577a
RenderViewport.showOnScreen: skip if already visible.
LongCatIsLooong May 5, 2020
749f9d9
Handling Floating Headers
LongCatIsLooong May 5, 2020
bc33a39
prevent `EditableText`'s showOnScreen from exceed its own paintBounds.
LongCatIsLooong May 5, 2020
197ac2a
review
LongCatIsLooong May 6, 2020
ce6b502
make it configurable
LongCatIsLooong May 11, 2020
cf2e39a
Merge branch 'master' of github.com:flutter/flutter into fix-viewport…
LongCatIsLooong May 11, 2020
666727c
doc update
LongCatIsLooong May 12, 2020
0174eca
more tests
LongCatIsLooong May 13, 2020
75b7c3d
Merge branch 'master' of github.com:flutter/flutter into fix-viewport…
LongCatIsLooong May 13, 2020
5732292
update SliverAppBar configuration
LongCatIsLooong May 13, 2020
0955d3a
review
LongCatIsLooong May 14, 2020
34b397c
simplify
LongCatIsLooong Jun 9, 2020
cad2f5e
Merge remote-tracking branch 'upstream/master' into fix-viewport-show…
LongCatIsLooong Jun 9, 2020
83e97c9
per use lint
LongCatIsLooong Jun 9, 2020
2f3713d
review
LongCatIsLooong Jun 9, 2020
563c72e
Merge remote-tracking branch 'upstream/master' into fix-viewport-show…
LongCatIsLooong Jul 7, 2020
371ee55
analyzer
LongCatIsLooong Jul 7, 2020
132c82c
Merge remote-tracking branch 'upstream/master' into fix-viewport-show…
LongCatIsLooong Jul 10, 2020
5f394ca
Merge remote-tracking branch 'upstream/master' into fix-viewport-show…
LongCatIsLooong Jul 22, 2020
cfdb749
remove the onScreen check
LongCatIsLooong Jul 22, 2020
929b666
Revert "remove the onScreen check"
LongCatIsLooong Jul 23, 2020
5cff319
This reverts commit 929b66653dcc82dfd0dc9989055fd81bfe67246f.
LongCatIsLooong Jul 24, 2020
3e27f05
leadingScrollOffset calc
LongCatIsLooong Jul 28, 2020
29c5d7d
typo
LongCatIsLooong Jul 28, 2020
bc5d5c3
tests
LongCatIsLooong Jul 28, 2020
c79ebfa
Merge remote-tracking branch 'upstream/master' into fix-viewport-show…
LongCatIsLooong Jul 28, 2020
b40787e
update
LongCatIsLooong Jul 29, 2020
9772b0a
docs
LongCatIsLooong Jul 29, 2020
722493f
more tests
LongCatIsLooong Jul 29, 2020
930a944
remove lint ignore
LongCatIsLooong Aug 14, 2020
075d5d9
Merge remote-tracking branch 'upstream/master' into fix-viewport-show…
LongCatIsLooong Aug 14, 2020
fcdaac7
docs fix
LongCatIsLooong Aug 14, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
117 changes: 117 additions & 0 deletions packages/flutter/lib/src/rendering/sliver_persistent_header.dart
Expand Up @@ -196,6 +196,68 @@ abstract class RenderSliverPersistentHeader extends RenderSliver with RenderObje
_lastStretchOffset = stretchOffset;
}

@override
void showOnScreen({
RenderObject descendant,
Rect rect,
Duration duration = Duration.zero,
Curve curve = Curves.ease,
}) {
assert(child != null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

child must only be != null if a descendent was provided, right? You could call showOnScreen on a child-less header to bring the entire header back on screen.

final Rect childBounds = descendant != null
? MatrixUtils.transformRect(descendant.getTransformTo(child), rect ?? descendant.paintBounds)
: null;

// Trims the part of `rect` that protrudes `child`'s leading edge.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does it need to stay in the bounds of the child? Doesn't it just have to stay within my own bounds?

If that is the case, could this all be simplified to paintBounds.intersect(boundsOfDescendantInMyOwnCoordinateSystem)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right we only need to override showOnScreen for pinned headers where it's always the case (I'm overriding the wrong class). But I'm not sure which approach we should take, relying on the caller to pass us the right rect, or trim the rect the caller passed down? The latter sounds a bit hacky to me.

//
// If the `rect` a descendant specified in its showOnScreen call exceeds
// the leading edge of this sliver (which is usually the same as that of
// `child`), the viewport will move towards the leading edge (reduce its
// scroll offset) to unpin the persistent header. This is almost always
// undesirable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me awhile to understand this comment, so I tried to clarify a bit. Definitely double check this and make sure I understood correctly, though.

Trims the given Rect original so that it fits within the boundaries given by top, right, bottom, and left.

This is used to prevent the case where rect or descendant specified in showOnScreen exceed the leading edge of this sliver. If this were to happen, the viewport would move towards the leading edge (reducing its scroll offset) in order to unpin the persistent header. This is almost always undesirable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I'll put the comment above the showOnScreen method instead.

Is it OK to limit the size of the caret's scroll padding instead, so that it will never exceed the leading edge (see the comment in "editable_text.dart" file)? That makes a little bit more sense to me (in case the caller of showOnScreen really expects us to respect the rect specified). Or maybe we should reduce the default scrollPadding (EdgeInsets.all(20.0)) and document this edge case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, but would it be a breaking change in normal situations?

//
// See: https://github.com/flutter/flutter/issues/25507.
Rect trim(Rect original, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: instead of defining this inline, this code just be a private method on this object to declutter this method, no?

double top = -double.infinity,
double right = double.infinity,
double bottom = double.infinity,
double left = -double.infinity,
}) {
if (original == null)
return null;

return Rect.fromLTRB(
math.max(original.left, left),
math.max(original.top, top),
math.min(original.right, right),
math.min(original.bottom, bottom),
);
}

Rect newRect;
switch (applyGrowthDirectionToAxisDirection(constraints.axisDirection, constraints.growthDirection)) {
case AxisDirection.up:
newRect = trim(childBounds ?? rect, bottom: childExtent);
break;
case AxisDirection.right:
newRect = trim(childBounds ?? rect, left: 0);
break;
case AxisDirection.down:
newRect = trim(childBounds ?? rect, top: 0);
break;
case AxisDirection.left:
newRect = trim(childBounds ?? rect, right: childExtent);
break;
}

super.showOnScreen(
descendant: descendant == null ? this : child,
rect: newRect,
duration: duration,
curve: curve,
);
}

/// Returns the distance from the leading _visible_ edge of the sliver to the
/// side of the child closest to that edge, in the scroll axis direction.
///
Expand Down Expand Up @@ -585,6 +647,61 @@ abstract class RenderSliverFloatingPersistentHeader extends RenderSliverPersiste
_lastActualScrollOffset = constraints.scrollOffset;
}

@override
void showOnScreen({
RenderObject descendant,
Rect rect,
Duration duration = Duration.zero,
Curve curve = Curves.ease,
}) {
assert(child != null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same child comment as above.

final Rect childBounds = descendant != null
? MatrixUtils.transformRect(descendant.getTransformTo(child), rect ?? descendant.paintBounds)
: null;

double minTargetExtent;
switch (applyGrowthDirectionToAxisDirection(constraints.axisDirection, constraints.growthDirection)) {
case AxisDirection.up:
minTargetExtent = childExtent - (childBounds?.top ?? 0);
break;
case AxisDirection.right:
minTargetExtent = childBounds?.right ?? childExtent;
break;
case AxisDirection.down:
minTargetExtent = childBounds?.bottom ?? childExtent;
break;
case AxisDirection.left:
minTargetExtent = childExtent - (childBounds?.left ?? 0);
break;
}

minTargetExtent = minTargetExtent.clamp(childExtent, maxExtent) as double;
// Expands the header if needed, with animation if possible.
if (minTargetExtent > childExtent) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I missed it, but did you have tests for when the header animates and for when it doesn't?

if (snapConfiguration != null) {
_controller ??= AnimationController(vsync: snapConfiguration.vsync, duration: duration);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

who starts this AnimationController? The only place I see calling "forward" on this sets a new animation to _animation overwriting the one set below?

_controller.duration = duration;
_animation = _controller
.drive(
Tween<double>(
begin: _effectiveScrollOffset,
end: maxExtent - minTargetExtent ,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't the idea of a snapping header that it will always snap to its max extend?

).chain(CurveTween(curve: curve)),
);
} else {
_effectiveScrollOffset = maxExtent - minTargetExtent;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the app bar just jumps from one size to the next in one frame? That seems ugly.

markNeedsLayout();
}
}

super.showOnScreen(
descendant: descendant == null ? this : child,
rect: childBounds,
duration: duration,
curve: curve,
);
}

@override
double childMainAxisPosition(RenderBox child) {
assert(child == this.child);
Expand Down
62 changes: 53 additions & 9 deletions packages/flutter/lib/src/rendering/viewport.dart
Expand Up @@ -783,7 +783,6 @@ abstract class RenderViewportBase<ParentDataClass extends ContainerParentDataMix
final double offsetDifference = offset.pixels - targetOffset;

final Matrix4 transform = target.getTransformTo(this);
applyPaintTransform(child, transform);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the paint transform that does child -> this is applied twice here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. Did you add a test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed. Adding this in a different PR.

Rect targetRect = MatrixUtils.transformRect(transform, rect);

switch (axisDirection) {
Expand Down Expand Up @@ -986,16 +985,61 @@ abstract class RenderViewportBase<ParentDataClass extends ContainerParentDataMix
);
}

final Rect newRect = RenderViewportBase.showInViewport(
descendant: descendant,
viewport: this,
offset: offset,
rect: rect,
duration: duration,
curve: curve,
RenderObject child = descendant;
RenderSliver childSliver;

do {
if (child is RenderSliver) {
childSliver = child;
}

final RenderObject parent = child.parent as RenderObject;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if descendent (and thus child) is null?

child = parent;
assert(child != null, '$descendant must be a descendant of $this.');
} while (child != this);

assert(childSliver.parent == this);
Rect localRect = MatrixUtils.transformRect(
(descendant ?? this).getTransformTo(this),
rect ?? descendant?.paintBounds ?? paintBounds,
);

final double extentOfPinnedSlivers = maxScrollObstructionExtentBefore(childSliver);
assert(extentOfPinnedSlivers >= 0);

bool canSkipScrolling = true;
switch (applyGrowthDirectionToAxisDirection(axisDirection, childSliver.constraints.growthDirection)) {
case AxisDirection.up:
canSkipScrolling = canSkipScrolling && localRect.top >= 0;
canSkipScrolling = canSkipScrolling && localRect.bottom <= size.height - extentOfPinnedSlivers;
break;
case AxisDirection.right:
canSkipScrolling = canSkipScrolling && localRect.left >= extentOfPinnedSlivers;
canSkipScrolling = canSkipScrolling && localRect.right <= size.width;
break;
case AxisDirection.down:
canSkipScrolling = canSkipScrolling && localRect.top >= extentOfPinnedSlivers;
canSkipScrolling = canSkipScrolling && localRect.bottom <= size.height;
break;
case AxisDirection.left:
canSkipScrolling = canSkipScrolling && localRect.left >= 0;
canSkipScrolling = canSkipScrolling && localRect.right <= size.width - extentOfPinnedSlivers;
break;
}

if (!canSkipScrolling) {
localRect = RenderViewportBase.showInViewport(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

showInViewport already skips scrolling if the descendant is fully in view. What case does the logic above cover?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

showInViewport uses getOffsetToReveal (which assumes the target sliver moves linearly) to determine whether a sliver is still visible. With pinned headers we can get false negatives (the scroll offset indicates the sliver is not in the viewport but it is), with floating headers it may take less scroll offset than getOffsetToReveal suggested to reveal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The added logic does assume the layout is up to date with the current offset. I'll add a check to invalidate the skip logic if the assumption does not hold.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@goderbauer got rid of the visibility check because the viewport could be scrolling / about to scroll. Could you take a look?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry nvm that broke some tests. Fixing.

descendant: descendant,
viewport: this,
offset: offset,
rect: rect,
duration: duration,
curve: curve,
);
}

super.showOnScreen(
rect: newRect,
rect: localRect,
duration: duration,
curve: curve,
);
Expand Down
12 changes: 5 additions & 7 deletions packages/flutter/lib/src/widgets/editable_text.dart
Expand Up @@ -1686,13 +1686,11 @@ class EditableTextState extends State<EditableText> with AutomaticKeepAliveClien
bottomSpacing,
);
}
final Rect inflatedRect = Rect.fromLTRB(
newCaretRect.left - widget.scrollPadding.left,
newCaretRect.top - widget.scrollPadding.top,
newCaretRect.right + widget.scrollPadding.right,
newCaretRect.bottom + bottomSpacing,
);
_editableKey.currentContext.findRenderObject().showOnScreen(

final Rect inflatedRect = widget.scrollPadding
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative to the trimming in RenderSliverPersistentHeader.showOnScreen is to confine inflatedRect so that it doesn't exceed renderEditable's paintBounds. But that breaks around 6 tests.

.copyWith(bottom: bottomSpacing)
.inflateRect(newCaretRect);
renderEditable.showOnScreen(
rect: inflatedRect,
duration: _caretAnimationDuration,
curve: _caretAnimationCurve,
Expand Down