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

Implement CupertinoSearchTextField opacity fade on scroll #155025

Merged
merged 32 commits into from
Oct 15, 2024

Conversation

victorsanni
Copy link
Contributor

@victorsanni victorsanni commented Sep 11, 2024

Before:

364571986-9d3c59a3-5fda-47b2-993b-cc05b9eba596.1.mov

After:

Screen.Recording.2024-09-11.at.9.37.41.AM.mov

Native iOS:

364571955-39372e3d-e59a-43c7-a94b-b71038b1ee6e.mov

Fixes #154648

Needed for #18103

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository labels Sep 11, 2024
@victorsanni
Copy link
Contributor Author

Works only on mobile platforms... Maybe something to do with Scrollable.of(context)?

void didChangeDependencies() {
super.didChangeDependencies();
_ancestorScrollController?.removeListener(_onScroll);
_ancestorScrollController = Scrollable.maybeOf(context)?.widget.controller;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably why it is only working on mobile.
ScrollableState.widget.controller will only have a controller if the user has provided one, or the ancestor ScrollView decided to assign the PrimaryScrollController.
See the _effectiveScrollController in ScrollableState, which accounts for this. It may need to be made public for this feature, although, you could use ScrollableState.position and listen to that alternatively. :)

@victorsanni victorsanni marked this pull request as ready for review September 12, 2024 21:06
}

void _onScroll() {
if (_ancestorScrollPosition == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should check if the pixels property can be used, with hasPixels. The scroll position might not be attached.

@Piinks Piinks self-requested a review September 25, 2024 18:24
Copy link
Contributor

@MitchellGoodwin MitchellGoodwin left a comment

Choose a reason for hiding this comment

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

Overall, it looks good. I have some questions about behavior though.

For the partial scroll, so a scroll stops mid fade, can we put in snapping for this widget to start? It might be odd if it's partially shrunk and has faded text if the user tries to interact and type into it while it's partially shrunk.

setState(() { _fadeExtent = _calculateScrollOpacity(currentHeight, _maxHeight); });
}

static double _calculateScrollOpacity(double currentHeight, double maxHeight) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this assume that the search field is at the top of the scrollable content? What happens if it isn't?

Copy link
Contributor Author

@victorsanni victorsanni Sep 30, 2024

Choose a reason for hiding this comment

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

this is only useful when the search field is resizing (i.e it's height is reducing from its initial maximum height). The only reason it's called _calculateScrollOpacity is because this resizing in this case must happen due to a scroll (because of the if-block on line 425). So its scroll position isn't really relevant and is used only to check that this resizing effect is happening in the midst of a scroll.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TLDR we don't care about where the search field is in the scroll view, only that it is in a scroll view and resizing.

}

static double _calculateScrollOpacity(double currentHeight, double maxHeight) {
final double thresholdHeight = maxHeight * _kMinHeightBeforeTotalTransparency;
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: should this only take height into account, or should padding be a factor? It might be fine to not get too granular in this to start though.

Copy link
Contributor Author

@victorsanni victorsanni Sep 30, 2024

Choose a reason for hiding this comment

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

It's a valid concern. Testing locally with different paddings, I don't think it will be a problem (for now) though.

An additional benefit of making it dependent on the height alone is that it guarantees that by the time the search field is small enough (that the prefix/suffix icons overflow), the contents have faded out completely.

@victorsanni
Copy link
Contributor Author

For the partial scroll, so a scroll stops mid fade, can we put in snapping for this widget to start? It might be odd if it's partially shrunk and has faded text if the user tries to interact and type into it while it's partially shrunk.

I'm wondering if the snapping effect is more of a nav bar thing rather than a widget specific thing. Because in the settings app for instance, the padding/margin around the search bar also snaps along with the search bar itself. If we implement the snapping on the widget level, how would that play with the snapping on the nav bar level?

@@ -398,13 +421,35 @@ class _CupertinoSearchTextFieldState extends State<CupertinoSearchTextField>
}
}

void _onScroll() {
if (_ancestorScrollPosition == null || !_ancestorScrollPosition!.hasPixels) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Today we talked about setting _maxHeight here when _onScroll is called for the first time by looking up the render object as you are below. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, we may want to use the ScrollNotificationObserver. It would be more consistent with how the AppBar scrolledUnder behavior works, and we could get more information about what kind of scrolling is occurring.

packages/flutter/lib/src/cupertino/search_field.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@MitchellGoodwin MitchellGoodwin left a comment

Choose a reason for hiding this comment

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

Code LGTM, but I'm not very informed on scrolling.

For the snapping on partial scroll: it makes an amount of sense to have that in a separate PR. Maybe with the drawer/ bottom work? But I'm just wondering if anything about this PR may get in the way of that. Is there a rough idea of how that could be implemented?

@victorsanni
Copy link
Contributor Author

For the snapping on partial scroll: it makes an amount of sense to have that in a separate PR. Maybe with the drawer/ bottom work? But I'm just wondering if anything about this PR may get in the way of that. Is there a rough idea of how that could be implemented?

I don't think this PR will get in the way of that. For the snapping/autoscrolling, I think we can grab the ScrollController of the parent CustomScrollView, and animate the scroll by the necessary offset when we receive a ScrollEndNotification (and the title/bottom is partially scrolled under the edge of the viewport). This would probably be in its own PR.

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

LGTM to me too! 🎉

@victorsanni victorsanni added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 15, 2024
@auto-submit auto-submit bot merged commit 915985a into flutter:master Oct 15, 2024
74 checks passed
@victorsanni victorsanni deleted the search-field-opacity-fade branch October 17, 2024 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CupertinoSearchTextField placeholder should resize as the text field resizes instead of being closed over
5 participants