Skip to content

content/msglist: Fix vertical position of horizontal scrollbar in codeblock #738

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

Merged
merged 2 commits into from
Jun 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 44 additions & 35 deletions lib/widgets/message_list.dart
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,49 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
Widget _buildListView(BuildContext context) {
final length = model!.items.length;
const centerSliverKey = ValueKey('center sliver');

Widget sliver = SliverStickyHeaderList(
headerPlacement: HeaderPlacement.scrollingStart,
delegate: SliverChildBuilderDelegate(
// To preserve state across rebuilds for individual [MessageItem]
// widgets as the size of [MessageListView.items] changes we need
// to match old widgets by their key to their new position in
// the list.
//
// The keys are of type [ValueKey] with a value of [Message.id]
// and here we use a O(log n) binary search method. This could
// be improved but for now it only triggers for materialized
// widgets. As a simple test, flinging through Combined feed in
// CZO on a Pixel 5, this only runs about 10 times per rebuild
// and the timing for each call is <100 microseconds.
//
// Non-message items (e.g., start and end markers) that do not
// have state that needs to be preserved have not been given keys
// and will not trigger this callback.
findChildIndexCallback: (Key key) {
final valueKey = key as ValueKey<int>;
final index = model!.findItemWithMessageId(valueKey.value);
if (index == -1) return null;
return length - 1 - (index - 2);
},
childCount: length + 2,
(context, i) {
// To reinforce that the end of the feed has been reached:
// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1680603
if (i == 0) return const SizedBox(height: 36);

if (i == 1) return MarkAsReadWidget(narrow: widget.narrow);

final data = model!.items[length - 1 - (i - 2)];
return _buildItem(data, i);
}));

if (widget.narrow is CombinedFeedNarrow) {
// TODO(#311) If we have a bottom nav, it will pad the bottom
// inset, and this shouldn't be necessary
sliver = SliverSafeArea(sliver: sliver);
}

return CustomScrollView(
// TODO: Offer `ScrollViewKeyboardDismissBehavior.interactive` (or
// similar) if that is ever offered:
Expand All @@ -318,41 +361,7 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
center: centerSliverKey,

slivers: [
SliverStickyHeaderList(
headerPlacement: HeaderPlacement.scrollingStart,
delegate: SliverChildBuilderDelegate(
// To preserve state across rebuilds for individual [MessageItem]
// widgets as the size of [MessageListView.items] changes we need
// to match old widgets by their key to their new position in
// the list.
//
// The keys are of type [ValueKey] with a value of [Message.id]
// and here we use a O(log n) binary search method. This could
// be improved but for now it only triggers for materialized
// widgets. As a simple test, flinging through Combined feed in
// CZO on a Pixel 5, this only runs about 10 times per rebuild
// and the timing for each call is <100 microseconds.
//
// Non-message items (e.g., start and end markers) that do not
// have state that needs to be preserved have not been given keys
// and will not trigger this callback.
findChildIndexCallback: (Key key) {
final valueKey = key as ValueKey<int>;
final index = model!.findItemWithMessageId(valueKey.value);
if (index == -1) return null;
return length - 1 - (index - 2);
},
childCount: length + 2,
(context, i) {
// To reinforce that the end of the feed has been reached:
// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Mark-as-read/near/1680603
if (i == 0) return const SizedBox(height: 36);

if (i == 1) return MarkAsReadWidget(narrow: widget.narrow);

final data = model!.items[length - 1 - (i - 2)];
return _buildItem(data, i);
})),
sliver,

// This is a trivial placeholder that occupies no space. Its purpose is
// to have the key that's passed to [ScrollView.center], and so to cause
Expand Down
16 changes: 16 additions & 0 deletions test/widgets/message_list_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,22 @@ void main() {
return scrollView.controller;
}

group('presents message content appropriately', () {
// regression test for https://github.com/zulip/zulip-flutter/issues/736
testWidgets('content in "Combined feed" not asked to consume insets (including bottom)', (tester) async {
const fakePadding = FakeViewPadding(left: 10, top: 10, right: 10, bottom: 10);
tester.view.viewInsets = fakePadding;
tester.view.padding = fakePadding;

await setupMessageListPage(tester, narrow: const CombinedFeedNarrow(),
messages: [eg.streamMessage(content: ContentExample.codeBlockPlain.html)]);

final element = tester.element(find.byType(CodeBlock));
final padding = MediaQuery.of(element).padding;
check(padding).equals(EdgeInsets.zero);
});
});

group('fetch older messages on scroll', () {
int? itemCount(WidgetTester tester) =>
tester.widget<CustomScrollView>(find.byType(CustomScrollView)).semanticChildCount;
Expand Down
Loading