Skip to content

Commit 2eeb6af

Browse files
Revert "Changes the offset computation to first item for RenderSliverMainAxisGroup" (flutter#154688) (flutter#168450)
This PR reverts commit 8cc862c I also added a regression test for flutter#167801 so that we do not break that again when revisiting the issue that is reopened. Fixes flutter#167801 Reopens flutter#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 <[email protected]>
1 parent 82e00c8 commit 2eeb6af

File tree

2 files changed

+62
-58
lines changed

2 files changed

+62
-58
lines changed

packages/flutter/lib/src/rendering/sliver_group.dart

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -230,15 +230,7 @@ class RenderSliverMainAxisGroup extends RenderSliver
230230
double childScrollOffset = 0.0;
231231
RenderSliver? current = childBefore(child as RenderSliver);
232232
while (current != null) {
233-
// If the current child is not the first child, then we need to
234-
// add the scroll extent of the previous child to the current child's
235-
// scroll offset.
236-
if (childBefore(current) != null) {
237-
childScrollOffset +=
238-
childAfter(current)!.geometry!.scrollExtent + child.geometry!.scrollExtent;
239-
} else if (!(childAfter(child) != null && current.geometry!.hasVisualOverflow)) {
240-
childScrollOffset += current.geometry!.scrollExtent;
241-
}
233+
childScrollOffset += current.geometry!.scrollExtent;
242234
current = childBefore(current);
243235
}
244236
return childScrollOffset;

packages/flutter/test/widgets/sliver_main_axis_group_test.dart

Lines changed: 61 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -879,47 +879,78 @@ void main() {
879879
expect(tester.getTopLeft(find.byKey(key)), Offset.zero);
880880
});
881881

882+
// Regression test for https://github.com/flutter/flutter/issues/167801
882883
testWidgets(
883-
'SliverMainAxisGroup scrolls to the correct position when focusing on a text field within a header',
884+
'Nesting SliverMainAxisGroups does not break ShowCaretOnScreen for text fields inside nested SliverMainAxisGroup',
884885
(WidgetTester tester) async {
885-
final ScrollController controller = ScrollController();
886-
addTearDown(controller.dispose);
887-
final FocusNode textFieldFocus = FocusNode();
888-
addTearDown(textFieldFocus.dispose);
889-
final FocusNode textFieldFocus2 = FocusNode();
890-
addTearDown(textFieldFocus2.dispose);
891-
const ValueKey<int> firstTextFieldKey = ValueKey<int>(1);
892-
893-
await tester.pumpWidget(
894-
_buildSliverMainAxisGroup(
895-
controller: controller,
896-
slivers: <Widget>[
897-
SliverPersistentHeader(
898-
pinned: true,
899-
delegate: _SliverTitleDelegate(
900-
child: Container(color: Colors.red, height: 60.0),
901-
height: 60.0,
886+
// The number of groups and items per group needs to be high enough to reproduce the bug.
887+
const int sliverGroupsCount = 3;
888+
const int sliverGroupItemsCount = 60;
889+
// To make working with the scroll offset easier, each item is a fixed height.
890+
const double itemHeight = 72.0;
891+
892+
final ScrollController scrollController = ScrollController();
893+
addTearDown(scrollController.dispose);
894+
895+
final Widget widget = MaterialApp(
896+
theme: ThemeData(
897+
inputDecorationTheme: const InputDecorationTheme(
898+
focusedBorder: OutlineInputBorder(borderSide: BorderSide(color: Color(0xFF1489FD))),
899+
enabledBorder: OutlineInputBorder(borderSide: BorderSide(color: Color(0xFFB1BDC5))),
900+
),
901+
),
902+
home: Scaffold(
903+
body: CustomScrollView(
904+
controller: scrollController,
905+
slivers: <Widget>[
906+
SliverMainAxisGroup(
907+
slivers: <Widget>[
908+
for (int i = 1; i <= sliverGroupsCount; i++)
909+
SliverMainAxisGroup(
910+
slivers: <Widget>[
911+
SliverList.builder(
912+
itemCount: sliverGroupItemsCount,
913+
itemBuilder: (_, int index) {
914+
final String label = 'Field $i.${index + 1}';
915+
916+
return SizedBox(
917+
height: itemHeight,
918+
child: Padding(
919+
// This extra padding is to make visually debugging the test app a bit better,
920+
// othwerwise the label text clips the text field above.
921+
padding: const EdgeInsets.symmetric(vertical: 8),
922+
child: TextField(
923+
key: ValueKey<String>(label),
924+
decoration: InputDecoration(labelText: label),
925+
),
926+
),
927+
);
928+
},
929+
),
930+
],
931+
),
932+
],
902933
),
903-
),
904-
SliverToBoxAdapter(
905-
child: Material(child: TextField(key: firstTextFieldKey, focusNode: textFieldFocus)),
906-
),
907-
SliverToBoxAdapter(child: Container(color: Colors.green, height: 500)),
908-
SliverToBoxAdapter(child: Material(child: TextField(focusNode: textFieldFocus2))),
909-
],
934+
],
935+
),
910936
),
911937
);
912938

913-
await tester.pumpAndSettle();
914-
await tester.pumpAndSettle();
939+
await tester.pumpWidget(widget);
940+
941+
// Scroll down to the first field in the second group, so that it is at the top of the screen.
942+
const double offset = sliverGroupItemsCount * itemHeight;
943+
scrollController.jumpTo(offset);
915944

916-
textFieldFocus2.requestFocus();
917945
await tester.pumpAndSettle();
918946

919-
textFieldFocus.requestFocus();
947+
// Tap the field so that it gains focus and requests the scrollable to scroll it into view.
948+
// However, since the field is at the top of the screen, far away from the keyboard,
949+
// the scroll position should not change.
950+
await tester.tap(find.byKey(const ValueKey<String>('Field 2.1')));
920951
await tester.pumpAndSettle();
921952

922-
expect(tester.getTopLeft(find.byKey(firstTextFieldKey)), const Offset(0, 60));
953+
expect(scrollController.offset, offset);
923954
},
924955
);
925956

@@ -1118,22 +1149,3 @@ class TestDelegate extends SliverPersistentHeaderDelegate {
11181149
@override
11191150
bool shouldRebuild(TestDelegate oldDelegate) => true;
11201151
}
1121-
1122-
class _SliverTitleDelegate extends SliverPersistentHeaderDelegate {
1123-
_SliverTitleDelegate({required this.height, required this.child});
1124-
final double height;
1125-
final Widget child;
1126-
1127-
@override
1128-
double get minExtent => height;
1129-
@override
1130-
double get maxExtent => height;
1131-
1132-
@override
1133-
Widget build(BuildContext context, double shrinkOffset, bool overlapsContent) {
1134-
return child;
1135-
}
1136-
1137-
@override
1138-
bool shouldRebuild(_SliverTitleDelegate oldDelegate) => true;
1139-
}

0 commit comments

Comments
 (0)