Changes the offset computation to first item for RenderSliverMainAxisGroup #154688
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Piinks
reviewed
Sep 11, 2024
Contributor
Piinks
left a comment
There was a problem hiding this comment.
@Mairramer can you share why the values of the existing tests changed?
victorsanni
reviewed
Sep 14, 2024
Contributor
victorsanni
left a comment
There was a problem hiding this comment.
LGTM with nits and pending further review.
Contributor
Author
|
@Piinks @victorsanni Here are the reasons for the changes in testing: In |
victorsanni
approved these changes
Sep 24, 2024
Contributor
I think what @Piinks is asking for is, specifically, what was the previous test doing (e.g moving some widget, then scrolling a certain way, and expecting some header to be visible, etc) and how is this PR going to change that behavior? (also for some reason this reads to me like an llm-generated response). |
victorsanni
requested changes
Sep 24, 2024
| expect(tester.getTopLeft(find.byKey(key)), const Offset(0, 300)); | ||
| }); | ||
|
|
||
| testWidgets('SliverMainAxisGroup scrolls to the correct position when focusing on a text field within a header', (WidgetTester tester) async { |
Contributor
There was a problem hiding this comment.
This test passes on master.
Contributor
There was a problem hiding this comment.
What was incorrect about the test previously that was fixed?
Contributor
There was a problem hiding this comment.
@Mairramer this was resolved without an answer.
Contributor
Author
There was a problem hiding this comment.
@Piinks Ah, sorry, I forgot to mention. I was checking a parameter of the ScrollController that would work even without any changes I made.
Piinks
reviewed
Sep 24, 2024
victorsanni
reviewed
Sep 24, 2024
victorsanni
reviewed
Sep 24, 2024
victorsanni
reviewed
Sep 25, 2024
victorsanni
reviewed
Sep 30, 2024
victorsanni
approved these changes
Oct 1, 2024
Piinks
reviewed
Oct 15, 2024
| // If the current sliver is the first child, add its scrollExtent once to the scrollOffset | ||
| // to account for only the current sliver. | ||
| if (childBefore(current) != null) { | ||
| childScrollOffset += 2 * current.geometry!.scrollExtent; |
Contributor
There was a problem hiding this comment.
2 feels like a magic number here. Shouldn't it depends on the preceding child's size?
Contributor
Author
There was a problem hiding this comment.
Yes, you're right. I adjusted the code to have the expected behavior and avoid 2 as the magic number, in addition to preventing it from scrolling further than expected.
Contributor
|
Rebasing to fix CI |
9 tasks
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Oct 25, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Oct 25, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Oct 25, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Oct 25, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Oct 25, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Oct 26, 2024
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Oct 26, 2024
auto-submit bot
pushed a commit
to flutter/packages
that referenced
this pull request
Oct 26, 2024
Roll Flutter from 4faa4a4 to 5a11904 (67 revisions) flutter/flutter@4faa4a4...5a11904 2024-10-26 30870216+gaaclarke@users.noreply.github.com Relands "Wide gamut framework gradient test (#153976)" (flutter/flutter#157643) 2024-10-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7c5c5fe5c84d to c9b8ac96f6ce (3 revisions) (flutter/flutter#157662) 2024-10-26 32538273+ValentinVignal@users.noreply.github.com Add test for `navigator_state.restorable_push_and_remove_until.0.dart` (flutter/flutter#157595) 2024-10-26 matanlurey@users.noreply.github.com Tighten up `throwToolExit`, explain when to use it. (flutter/flutter#157561) 2024-10-26 matanlurey@users.noreply.github.com Remove extraneous `throw`. (flutter/flutter#157658) 2024-10-26 32538273+ValentinVignal@users.noreply.github.com Add tests for `navigator.restorable_push.0.dart` (flutter/flutter#157492) 2024-10-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from 43e4d9a30666 to 7c5c5fe5c84d (1 revision) (flutter/flutter#157644) 2024-10-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5061358e255f to 43e4d9a30666 (1 revision) (flutter/flutter#157637) 2024-10-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from eb867e055790 to 5061358e255f (2 revisions) (flutter/flutter#157623) 2024-10-25 polinach@google.com Create flutter specific leak troubleshooting guidance. (flutter/flutter#157396) 2024-10-25 katelovett@google.com Update CupertinoNavigationBar to support large layout (flutter/flutter#157133) 2024-10-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from 38e9be1f74fa to eb867e055790 (3 revisions) (flutter/flutter#157613) 2024-10-25 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Wide gamut framework gradient test (#153976)" (flutter/flutter#157615) 2024-10-25 30870216+gaaclarke@users.noreply.github.com Wide gamut framework gradient test (flutter/flutter#153976) 2024-10-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from b413d9996c86 to 38e9be1f74fa (2 revisions) (flutter/flutter#157604) 2024-10-25 engine-flutter-autoroll@skia.org Roll Packages from a556f0f to e0c4f55 (2 revisions) (flutter/flutter#157605) 2024-10-25 jonahwilliams@google.com Support backdrop key in flutter framework. (flutter/flutter#157278) 2024-10-25 reidbaker@google.com Add 3.24.4 changelog to master (flutter/flutter#157600) 2024-10-25 mohellebiabdessalem@gmail.com Update flutter.groovy to catch unknown task exception when finding api task (flutter/flutter#157282) 2024-10-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from c4b0184c8783 to b413d9996c86 (1 revision) (flutter/flutter#157580) 2024-10-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from b1c2ba8c4d52 to c4b0184c8783 (1 revision) (flutter/flutter#157578) 2024-10-25 32538273+ValentinVignal@users.noreply.github.com Add test for `build_owner.0.dart` (flutter/flutter#157499) 2024-10-25 32538273+ValentinVignal@users.noreply.github.com Add tests for `focusable_action_detector.0.dart` (flutter/flutter#157575) 2024-10-25 32538273+ValentinVignal@users.noreply.github.com Add test for `navigator.restorable_push_and_remove_until.0.dart` (flutter/flutter#157487) 2024-10-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from 29440ed1e225 to b1c2ba8c4d52 (1 revision) (flutter/flutter#157572) 2024-10-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from 88716d804aef to 29440ed1e225 (1 revision) (flutter/flutter#157569) 2024-10-25 engine-flutter-autoroll@skia.org Roll Flutter Engine from b8b28c80a737 to 88716d804aef (2 revisions) (flutter/flutter#157567) 2024-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 48ff670d256b to b8b28c80a737 (2 revisions) (flutter/flutter#157564) 2024-10-24 matanlurey@users.noreply.github.com Use discenrable characters (replace `' � � '` in error logs) (flutter/flutter#157548) 2024-10-24 matanlurey@users.noreply.github.com Remove unused `PubDependenciesProjectValidator`. (flutter/flutter#157516) 2024-10-24 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Upgrade tests to AGP 8.7/Gradle 8.10.2/Kotlin 1.8.10 (#157032)" (flutter/flutter#157559) 2024-10-24 jonahwilliams@google.com Mark mac impeller as bringup. (flutter/flutter#157551) 2024-10-24 tessertaha@gmail.com Deprecate `ThemeData.dialogBackgroundColor` in favor of `DialogThemeData.backgroundColor` (flutter/flutter#155072) 2024-10-24 34871572+gmackall@users.noreply.github.com Upgrade tests to AGP 8.7/Gradle 8.10.2/Kotlin 1.8.10 (flutter/flutter#157032) 2024-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 246344f26edc to 48ff670d256b (2 revisions) (flutter/flutter#157544) 2024-10-24 matanlurey@users.noreply.github.com Allow opting out of `.flutter-plugins`, opt-out in `refreshPluginsList`. (flutter/flutter#157527) 2024-10-24 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reverts "Added a warning if `flutter.groovy` uses a `.flutter-plugins` file. (#157388)" (#157541)" (flutter/flutter#157549) 2024-10-24 50643541+Mairramer@users.noreply.github.com Changes the offset computation to first item for RenderSliverMainAxisGroup (flutter/flutter#154688) 2024-10-24 engine-flutter-autoroll@skia.org Roll Packages from 5e03bb1 to a556f0f (7 revisions) (flutter/flutter#157539) 2024-10-24 737941+loic-sharma@users.noreply.github.com Add partial test for flutter build ios-framework on non-module (flutter/flutter#157482) 2024-10-24 737941+loic-sharma@users.noreply.github.com Add example to SafeArea docs (flutter/flutter#157228) 2024-10-24 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Added a warning if `flutter.groovy` uses a `.flutter-plugins` file. (#157388)" (flutter/flutter#157541) 2024-10-24 matanlurey@users.noreply.github.com Added a warning if `flutter.groovy` uses a `.flutter-plugins` file. (flutter/flutter#157388) 2024-10-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from be56084344d1 to 246344f26edc (2 revisions) (flutter/flutter#157504) 2024-10-24 104349824+huycozy@users.noreply.github.com Add ability to disable CupertinoSegmentedControl (flutter/flutter#152813) 2024-10-24 tessertaha@gmail.com Update `Tab.height` parameter doc for tab height lower than default (flutter/flutter#157443) ...
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 12, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 13, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Feb 13, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 6, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 7, 2025
navaronbracke
added a commit
to navaronbracke/flutter
that referenced
this pull request
May 7, 2025
…MainAxisGroup (flutter#154688)" This reverts commit 8cc862c.
navaronbracke
added a commit
to navaronbracke/flutter
that referenced
this pull request
May 7, 2025
…MainAxisGroup (flutter#154688)" This reverts commit 8cc862c.
LucasPessamilio
added a commit
to LucasPessamilio/flutter
that referenced
this pull request
May 7, 2025
…MainAxisGroup (flutter#154688)" This reverts commit 8cc862c.
navaronbracke
added a commit
to navaronbracke/flutter
that referenced
this pull request
May 15, 2025
…MainAxisGroup (flutter#154688)" This reverts commit 8cc862c.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 3, 2025
…MainAxisGroup" (#154688) (#168450) This PR reverts commit 8cc862c I also added a regression test for #167801 so that we do not break that again when revisiting the issue that is reopened. Fixes #167801 Reopens #154615 *List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.* *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Victor Sanni <victorsanniay@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #154615
When scrolling back to the top, the position of the first visible item should have its scrollExtent subtracted. Otherwise, the accumulated position may be retained, which may result in the first visible item being hidden in some cases.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.