Skip to content

content/msglist: Horizontal scrollbar in code block sometimes has wrong vertical position #736

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

Closed
chrisbobbe opened this issue Jun 14, 2024 · 4 comments · Fixed by #738
Closed
Labels
a-content Parsing and rendering Zulip HTML content, notably message contents a-msglist The message-list screen, except what's label:a-content

Comments

@chrisbobbe
Copy link
Collaborator

In the code block in this screenshot, the scroll bar should be positioned lower than it is:

I think it might have to do with the bottom inset. I seem to be seeing it in the "Combined feed" but not any other message-list views. "Combined feed" behaves differently about padding the bottom inset because it doesn't show the compose box.

@chrisbobbe chrisbobbe added a-content Parsing and rendering Zulip HTML content, notably message contents a-msglist The message-list screen, except what's label:a-content labels Jun 14, 2024
@chrisbobbe
Copy link
Collaborator Author

chrisbobbe commented Jun 14, 2024

Looks like this broke in this commit:

6a8cf5c msglist [nfc]: Unpack to a CustomScrollView of a single sliver

From CustomScrollView's dartdoc:

/// [CustomScrollView]s don't automatically avoid obstructions from [MediaQuery]
/// like [ListView]s do. To reproduce the behavior, wrap the slivers in
/// [SliverSafeArea]s.

Here's before and after that commit (which was in January, so the appearance won't be current in either screenshot):

Before After
C6CEB09A-7695-4087-87AA-D1089732AC8A 2D3A22ED-DA45-4920-9E2D-0BCDDB59DAF5

@chrisbobbe
Copy link
Collaborator Author

We could fix it in main by wrapping our SliverStickyHeaderList sliver in a SliverSafeArea, when the narrow is "Combined feed". (And add a TODO(#311) like the ones we have already.)

@gnprice, would this approach disrupt your work toward #82?

Then for a regression test, maybe we render a CodeBlock in the message list on the "Combined feed" page, and check that it receives a MediaQuery.padding.bottom of zero.

@gnprice
Copy link
Member

gnprice commented Jun 15, 2024

Hmm interesting, good catch.

Wrapping in a SliverSafeArea would mean that the actual list stays out of the bottom inset area, right? That doesn't seem like the desired behavior — I think it's expected that the messages scroll visibly through that bottom area.

A MediaQuery.removePadding might be a good fix. That would mean that the list forgets that there is a bottom inset.

@chrisbobbe
Copy link
Collaborator Author

Wrapping in a SliverSafeArea would mean that the actual list stays out of the bottom inset area, right? That doesn't seem like the desired behavior — I think it's expected that the messages scroll visibly through that bottom area.

Hmm, this isn't what I'm seeing: screenshots at PR #738.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-content Parsing and rendering Zulip HTML content, notably message contents a-msglist The message-list screen, except what's label:a-content
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants