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

Conversation

LongCatIsLooong
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong commented May 6, 2020

Description

  • Check if the viewport needs to scroll at all in RenderViewport.showOnScreen before calculating the target scroll offset. The check is done via paint transform so it does not make assumptions about the slivers.
  • Check if the outmost sliver has a greater-than-zero maxScrollObstructionExtent.
  • Trim the Rect in RenderSliverPersistentHeader (and its subclasses) to prevent showOnScreen from unpinning the header in the viewport.
  • Expands the RenderSliverFloatingPersistentHeader and RenderSliverFloatingPinnedPersistentHeader when instructed by showOnScreen.

Related Issues

Fixes #25507

Tests

I added the following tests:

A pinned persistent header should not scroll when its descendant EditableText gains focus, etc.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

);
_editableKey.currentContext.findRenderObject().showOnScreen(

final Rect inflatedRect = widget.scrollPadding
Copy link
Contributor Author

@LongCatIsLooong LongCatIsLooong May 6, 2020

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.

@@ -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

@LongCatIsLooong LongCatIsLooong May 6, 2020

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

@goderbauer goderbauer May 7, 2020

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

@LongCatIsLooong LongCatIsLooong May 13, 2020

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.

Copy link
Contributor

@justinmc justinmc left a comment

LGTM from a quick review, but I'm no sliver expert, so I'll definitely defer to other reviewers.

// 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

@justinmc justinmc May 6, 2020

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

@LongCatIsLooong LongCatIsLooong May 6, 2020

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

@justinmc justinmc May 7, 2020

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?

_controller ??= AnimationController(vsync: snapConfiguration.vsync, duration: duration);
_controller.duration = duration;
_animation = _controller
.drive(
Copy link
Contributor

@justinmc justinmc May 6, 2020

Choose a reason for hiding this comment

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

This should be indented according to the style guide, if I'm not mistaken.

Duration duration = Duration.zero,
Curve curve = Curves.ease,
}) {
assert(child != null);
Copy link
Member

@goderbauer goderbauer May 7, 2020

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.

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

@goderbauer goderbauer May 7, 2020

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?

Duration duration = Duration.zero,
Curve curve = Curves.ease,
}) {
assert(child != null);
Copy link
Member

@goderbauer goderbauer May 7, 2020

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.

? MatrixUtils.transformRect(descendant.getTransformTo(child), rect ?? descendant.paintBounds)
: null;

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

@goderbauer goderbauer May 7, 2020

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

@LongCatIsLooong LongCatIsLooong May 8, 2020

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.

.drive(
Tween<double>(
begin: _effectiveScrollOffset,
end: maxExtent - minTargetExtent ,
Copy link
Member

@goderbauer goderbauer May 7, 2020

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?

}

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

@goderbauer goderbauer May 7, 2020

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

@LongCatIsLooong LongCatIsLooong May 8, 2020

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

@LongCatIsLooong LongCatIsLooong Jul 10, 2020

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

@LongCatIsLooong LongCatIsLooong Jul 22, 2020

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

@LongCatIsLooong LongCatIsLooong Jul 22, 2020

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.

@@ -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
Member

@goderbauer goderbauer May 7, 2020

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?

final Finder pinnedHeaderContent = find.descendant(
of: find.byWidget(children[10], skipOffstage: false),
matching: find.byKey(headerKey, skipOffstage: false),
skipOffstage: false,
Copy link
Member

@goderbauer goderbauer May 7, 2020

Choose a reason for hiding this comment

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

If the header is still visible (as said below), why do you need skipOffstage: false?

final Finder pinnedHeaderContent = find.descendant(
of: find.byWidget(children[10], skipOffstage: false),
matching: find.byKey(headerKey, skipOffstage: false),
skipOffstage: false,
Copy link
Member

@goderbauer goderbauer May 7, 2020

Choose a reason for hiding this comment

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

same here.


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

@goderbauer goderbauer May 7, 2020

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?

@fluttergithubbot fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label May 8, 2020
@LongCatIsLooong LongCatIsLooong changed the title Prevent viewport.showOnScreen from scrolling the viewport if the specified Rect is already visible. [WIP] Prevent viewport.showOnScreen from scrolling the viewport if the specified Rect is already visible. May 11, 2020
@LongCatIsLooong LongCatIsLooong added the severe: API break Backwards-incompatible API changes. label May 12, 2020
@Piinks Piinks added f: scrolling Viewports, list views, slivers, etc. work in progress; do not review labels May 12, 2020
@LongCatIsLooong LongCatIsLooong force-pushed the fix-viewport-showOnScreen branch 3 times, most recently from 682965f to 827fbbe Compare May 13, 2020
@@ -17,6 +21,25 @@ import 'sliver.dart';
import 'viewport.dart';
import 'viewport_offset.dart';

// Trims the specified edges of the given `Rect` [original], so that they do not
// exceed the given values.
Copy link
Contributor Author

@LongCatIsLooong LongCatIsLooong May 13, 2020

Choose a reason for hiding this comment

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

I'm tempted to make this a private extension method to avoid the null check. Is using method extensions not recommended? It doesn't seem to be in the style guide.

Copy link
Contributor

@justinmc justinmc May 13, 2020

Choose a reason for hiding this comment

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

I'm curious too. I feel like we avoid this pattern because I never see it in our codebase.

Also a nit: are square brackets like [original] supposed to be used outside of public doc comments? I guess it doesn't matter if they are.

@LongCatIsLooong LongCatIsLooong changed the title [WIP] Prevent viewport.showOnScreen from scrolling the viewport if the specified Rect is already visible. Prevent viewport.showOnScreen from scrolling the viewport if the specified Rect is already visible. May 13, 2020
@fluttergithubbot fluttergithubbot added the f: material design flutter/packages/flutter/material repository. label May 13, 2020
Copy link
Contributor

@justinmc justinmc left a comment

LGTM but still deferring to others for sliver knowledge.

Your approach for getOffsetToReveal and revealInViewport seems like the right approach the way you describe it.

Piinks
Piinks approved these changes Aug 10, 2020
Copy link
Contributor

@Piinks Piinks left a comment

This LGTM. There may be some implications to consider later on in NestedScrollView, which currently does not support floating && snapping, just floating xor snapping. That's being tracked in #59189

LongCatIsLooong and others added 3 commits Aug 14, 2020
@fluttergithubbot fluttergithubbot merged commit 64d76f2 into flutter:master Aug 14, 2020
3 checks passed
@LongCatIsLooong LongCatIsLooong deleted the fix-viewport-showOnScreen branch Aug 14, 2020
mehmetf added a commit that referenced this pull request Aug 18, 2020
…the specified Rect is already visible. (#56413)"

This reverts commit 64d76f2.
mehmetf added a commit that referenced this pull request Aug 18, 2020
…the specified Rect is already visible. (#56413)" (#64091)

This reverts commit 64d76f2.
smadey pushed a commit to smadey/flutter that referenced this pull request Aug 27, 2020
smadey pushed a commit to smadey/flutter that referenced this pull request Aug 27, 2020
…the specified Rect is already visible. (flutter#56413)" (flutter#64091)

This reverts commit 64d76f2.
LongCatIsLooong added a commit to LongCatIsLooong/flutter that referenced this pull request Aug 29, 2020
fluttergithubbot pushed a commit that referenced this pull request Aug 29, 2020
…the specified Rect is already visible. (#56413)" reverted in #64091 (#64513)
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
…the specified Rect is already visible. (flutter#56413)" (flutter#64091)

This reverts commit 64d76f2.
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
amake added a commit to amake/orgro that referenced this pull request Nov 28, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. severe: API break Backwards-incompatible API changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CustomScrollView and TextField issue
6 participants