search

element-hq / element-web

A glossy Matrix collaboration client for the web.
https://element.io
Apache License 2.0
10.74k stars 1.89k forks source link

Threads Beta — Design implementation review #21502

Closed janogarcia closed 1 year ago

janogarcia commented 2 years ago

Resources

Thread list

Thread list: Hover state

Thread list: Empty state

Refer to mockups.

Thread view

Thread search: Keyword match in thread reply

Thread search: Keyword match in thread root message

Refer to the annotations on how the flow should work for “Keyword match in root message”:

t3chguy commented 2 years ago

Align thread list to the top (screenshot).

Align timeline to the top of the timeline container.

This is non-trivial as this is part of our ScrollPanel pagination mechanism - https://github.com/matrix-org/matrix-react-sdk/blob/develop/docs/scrolling.md#bacat-bottom-aligned-clipped-at-top-scrolling

The approach taken instead is to vertically align the timeline tiles to the bottom of the scroll container (using flexbox) and give the timeline inside the scroll container an explicit height, initially set to a multiple of the PAGE_SIZE (400px at time of writing) as needed by the content.

t3chguy commented 2 years ago

Vertical spacing between message titles: Apply the same spacing as in the main timeline (screenshot).

This already appears to be the case, we don't put any spacing between messages but have an inflated line-height to cause it.

image image

t3chguy commented 2 years ago

Make the border radius 8px (instead of 4px).

This already appears to be 8px, can you confirm?

nadonomy commented 2 years ago
Screenshot 2022-03-25 at 11 59 52

@janogarcia @t3chguy @gsouquet can I add these visual glitches to your list pls? (develop, latest):

  1. Display name split across multiple lines. I assume/suggest it should always be single line and use text-overflow: ellipsis; or similar.
  2. At certain responsive sizes, read receipts can overlay the thread summary. I assume/suggest the thread summary should be less wide at this breakpoint.
t3chguy commented 2 years ago

@nadonomy 1. is https://github.com/vector-im/element-web/issues/21472

t3chguy commented 2 years ago

Root messages in search results should display the thread summary in the footer. (“From a thread” it’s only for thread replies)

This is a limitation of the spec, the search API doesn't tell us they're thread roots: https://github.com/matrix-org/matrix-doc/pull/3666

A client-side workaround would be really expensive as it'd involve assuming every single search result/context event is a thread and ask the server for relations for each and every single one.

We will only be able to show the thread summary until the spec issue is fixed where we have already loaded that thread prior to encountering this thread root as a search result.

t3chguy commented 2 years ago

Regarding “From a thread” we can probably improve it, given that if that footer comes up then it may result in multiple messages with the same footer if the thread had quick subsequent replies and one of them matches the query.

image

If the result is a thread reply then any possible shown context around it will be also so no point each and every event having its own "From a thread"

janogarcia commented 2 years ago

This is non-trivial as this is part of our ScrollPanel pagination mechanism - https://github.com/matrix-org/matrix-react-sdk/blob/develop/docs/scrolling.md#bacat-bottom-aligned-clipped-at-top-scrolling

@t3chguy Thanks for sharing the technical background on the current timeline scrolling implementation. 👍

That said, we shouldn’t expect all scrolling lists in the app to behave the same way, as those are dependent on the content. What makes complete sense for the room timeline may not be the most usable pattern for other contexts. I know this is from a completely different context, with its own constraints, but we’ve implemented it already for iOS/Android and need this to work consistently across platforms.

So, what if we make some tradeoffs to make it work?

→ For example, let’s say we’re able to get the desired scrolling behavior TACAB (top-aligned, clipped-at-bottom), but we don’t try to prevent the list from jumping when new elements are dynamically added/removed? Would that make it any easier? Jumps in the thread list, while not an ideal UX, would be far less frequent than jumps in a timeline which gets constantly updated.

t3chguy commented 2 years ago

That said, we shouldn’t expect all scrolling lists in the app to behave the same way, as those are dependent on the content. What makes complete sense for the room timeline may not be the most usable pattern for other contexts. I know this is from a completely different context, with its own constraints, but we’ve implemented it already for iOS/Android and need this to work consistently across platforms.

Unfortunately maintaining two scrolling implementations will be very maintenance expensive, preventing scroll jumps in infinite-scrolling lists is non-trivial.

→ For example, let’s say we’re able to get the desired scrolling behavior TACAB (top-aligned, clipped-at-bottom), but we don’t try to prevent the list from jumping when new elements are dynamically added/removed? Would that make it any easier? Jumps in the thread list, while not an ideal UX, would be far less frequent than jumps in a timeline which gets constantly updated.

Yes it would but the maintenance burden would still be large, it'd cause a lot of code duplication and lead to a lot of potential for bugs due to the low test coverage we have and are able to have over complex interactions like infinite scrollers.

Jumps in the thread list, while not an ideal UX, would be far less frequent than jumps in a timeline which gets constantly updated.

A TACAB approach would have guaranteed scroll jumps every time you're back-paginating (scrolling up) due to the spinner rendering in & out.

janogarcia commented 2 years ago

This already appears to be the case, we don't put any spacing between messages but have an inflated line-height to cause it.

@t3chguy To be more specific, I mean the existing gap/padding between .mx_EventTile items, that aren't .mx_EventTile_continuation.

Google Chrome

Threads – Figma

janogarcia commented 2 years ago

This already appears to be 8px, can you confirm?

@t3chguy Let's say .mx_EventTile[data-shape=ThreadsList] has 8px applied to it, but it's not being rendered as intended/expected.

In the following example, to make it more obvious, I have set border-radius: 9999px to .mx_EventTile[data-shape=ThreadsList] and have changed the background colors of the split background issue. As you can see there's an awful mix of border radii values being applied over there.

Element 1 | 🧪 Playground Encrypted

janogarcia commented 2 years ago

This is a limitation of the spec, the search API doesn't tell us they're thread roots: https://github.com/matrix-org/matrix-spec-proposals/pull/3666

@t3chguy In that case, we can wait for the spec to be updated.

That said, the screenshot you shared with example search results is showing a thread summary on the first message. Has this been implemented somehow for search result context messages (but not for the actual search result matching messages)?

(iOS/Android are also showing thread summaries for root messages in search results. That’s why I assumed it was missing on Web. Unsure how they circumvented the current backend limitations and how performant it is.)

t3chguy commented 2 years ago

That said, the https://github.com/vector-im/element-web/issues/21502#issuecomment-1080622116 with example search results is showing a thread summary on the first message. Has this been implemented somehow for search result context messages (but not for the actual search result matching messages)?

As per my comment

We will only be able to show the thread summary until the spec issue is fixed where we have already loaded that thread prior to encountering this thread root as a search result.

In other words, its done on a best effort basis without having to hammer the server by asking for each event if it is a thread root. (i.e if user scrolled to it or it was recent or user opened the Thread Panel to cause all threads to get loaded in)

Edit: we could more eagerly fetch all threads (as soon as you open the room, or when you perform a search) but both can cause the server to get hammered more than necessary and cause a lot more memory & bandwidth to get consumed.

janogarcia commented 2 years ago

If the result is a thread reply then any possible shown context around it will be also so no point each and every event having its own "From a thread"

@t3chguy This can get quickly out of scope. That’s why I’d recommend not delving too much into how we display context for search results. The way we add context for search results is far from optimal in many ways, it’s not accessible (extremely low contrast), and it’s adding a lot of noise to search results. I’d even recommend not rendering the context at all, given its current state, but let’s postpone that until we have the chance to redesign search.

t3chguy commented 2 years ago

@t3chguy In that case, we can wait for the spec to be updated.

Tracked by https://github.com/vector-im/element-web/issues/21450

t3chguy commented 2 years ago

image

t3chguy commented 2 years ago

In the following example, to make it more obvious, I have set border-radius: 9999px to .mx_EventTile[data-shape=ThreadsList] and have changed the background colors of the split background issue. As you can see there's an awful mix of border radii values being applied over there.

Unfortunately the top and bottom radii must be on two different selectors as we can't use overflow: hidden due to the action bar floating out of the parent container. Have used a postcss variable to set both to 8px now.

janogarcia commented 2 years ago

@t3chguy Yeah, I was just answering your This already appears to be 8px, can you confirm? question by illustrating that while having 8px applied to it, it didn’t necessarily mean the resulting box would have 8px rounded corners.

It actually resulted in 3 different values (top corners for yellow background, right corners and left corners for orange background).

janogarcia commented 2 years ago

Update - April 1st, 2022

After further testing turns out the alternate approach works best for variable length content.

Display name split across multiple lines. I assume/suggest it should always be single line and use text-overflow: ellipsis; or similar.

@nadonomy Thanks for flagging, Nad. 👍

@t3chguy So, I've been experimenting a bit with the truncating behavior when there are long display names. I basically wanted to test two approaches, prioritizing display name over reply body vs. equal priority for display name and reply body.

After mocking it up on Figma and testing it live on the web app, I'd recommend going for the latter (equal priority for display name and reply body) because:

a) users gets more useful context in the summary b) worst case scenario looks worse when prioritizing the name (it can take up all the available space)

Neither approach is perfect, having their own pros/cons, but we can easily switch between both implementations (it's just a matter of toggling a CSS declaration).

The proposed behavior can be implemented by adding this ruleset:

.mx_ThreadInfo_sender {
    text-overflow: ellipsis;
    overflow: hidden;
    white-space: nowrap;
}

The alternate approach would be just a matter of adding this extra declaration:

.mx_ThreadInfo_content {
    flex: 1;
}

Web App

Element 1 | Threads P0 Delivery

Element 1 | Threads P0 Delivery

Figma

Threads – Figma

janogarcia commented 2 years ago

Actually, let's take the opposite approach (equal priority). Reason being that:

I'm editing my previous reply.

janogarcia commented 2 years ago

At certain responsive sizes, read receipts can overlay the thread summary. I assume/suggest the thread summary should be less wide at this breakpoint.

@t3chguy That shouldn't be happening. It seems .mx_ThreadInfo is not tracking/following the same width constraints as .mx_EventTile_line. Can we please make it track that constraint?

I think @jryans implemented the current responsive optimizations for the thread summary, maybe he can add some context for that difference, if intended for some reason.

Google Chrome

jryans commented 2 years ago

Thread view: The user name and message body should align to the same vertical line. (screenshot)

@t3chguy: The above seems marked as fixed, but it's still happening here...?

image
janogarcia commented 2 years ago

@jryans I think the related PR hasn’t been merged yet.

t3chguy commented 2 years ago

@jryans its fixed in the PR only

jryans commented 2 years ago

@t3chguy That shouldn't be happening. It seems .mx_ThreadInfo is not tracking/following the same width constraints as .mx_EventTile_line. Can we please make it track that constraint?

I think @jryans implemented the current responsive optimizations for the thread summary, maybe he can add some context for that difference, if intended for some reason.

Hmm no, it's not intentional... Both .mx_EventTile_line and .mx_ThreadInfo have a margin-right of 110px to allow room for the read receipts... but .mx_ThreadInfo is poking out of its parent box for some reason, so the margin isn't limiting the width properly... I'll see if I can spot a fix for this.

jryans commented 2 years ago

@jryans I think the related PR hasn’t been merged yet. @jryans its fixed in the PR only

Ah okay, I did look for a PR first... but may have only looked in the review requests list, whereas it's this draft PR. 😅

jryans commented 2 years ago

I'll see if I can spot a fix for this.

Looks like @t3chguy found a fix in parallel, so that should take care of it.

t3chguy commented 2 years ago

Vertical alignment for elements in the thread summary should behave the same way as thread summaries in the main timeline (the ones that Michael (t3chguy) recently fixed).

@janogarcia can you elaborate on this?

It is the only non-blocked point which is not tracked in another issue

janogarcia commented 2 years ago

@t3chguy Sure, we need to make sure we're applying the same vertical alignment properties/behavior for thread summaries in the thread list as those we defined a while ago for thread summaries in the main timeline. Ideally, we wouldn't need to define/update these styles in two separate places.

SCR-20220425-nax-2

novocaine commented 2 years ago

@janogarcia can you please confirm whether we have done enough here to close the ticket?

janogarcia commented 2 years ago

@novocaine @t3chguy

So, the only issue pending to be addressed is the infinite scrolling behavior. More specifically, the limitation of the current implementation (BACAT), that prevents us from ordering the scrollable events from top to bottom.

I’ve been thinking more about this lately and would like to share some updates:

From a UX perspective, we can’t presume that BACAT scrolling will work and serve best every possible scenario and interaction need (e.g., we already use the top-to-bottom scrolling pattern in the room list on the left sidebar and the people list on the right sidebar, although not for infinite items).

We consider top-to-bottom scrolling a core interaction pattern worth the effort of solving. It surely has a considerable implementation cost, but it also has a similar or higher product/UX cost not having it.

Now, given that we want to reuse this behavior in different contexts (mentions panel, files panel), meaning it’s not a Threads-specific limitation but a core interaction issue affecting our product in general, I think we could move this requirement out of the scope of the Exit Beta milestone for Threads and handle it separately.

So, to recap:

What do you think?

/cc @nadonomy CCing you as per a previous internal discussion.

janogarcia commented 2 years ago

@novocaine @t3chguy Feel free to close this issue if you agree with the stuff above.

t3chguy commented 2 years ago

I still consider the case for the thread list to be relevant, though (Align timeline to the top of the timeline container.). We (the design team) would still want users to have scrolling lists where the items are ordered from top to bottom. That doesn’t only make sense for the Thread List but other contexts as well, like the Mentions/Notifications panel or the Files panel.

The ordering isn't an issue to flip, the issue is the potential for a gap at the top due to how BACAT reserves space to prevent scroll jumps e.g by the spinner coming up and going away.

TACAB scrolling has guaranteed scroll-jumps for lazily/infinite loaded lists as the spinner renders and then vanishes.

We’d still want to order the thread list from most recently updated on the top to oldest on the bottom. We would handle it separately from the Threads project, treating it as a global app enhancement instead.

Great, though would be good to clarify whether by

We (the design team) would still want users to have scrolling lists where the items are ordered from top to bottom.

You mean just flipping the order or TACAB which would trade scroll jumps for having no gap

Thanks

janogarcia commented 2 years ago

We want to be able to flip the order, top to bottom, with the first item being aligned to the top of the scrolling container, without any extraneous/additional white space on the top, as in the mockup below (a filterable/sortable list of items).

I'd also expect no scroll position jumps when interacting with the scrolling behavior. If the spinner is introducing the problem, then we could think of a slightly different design for it.

Frame 6598
t3chguy commented 2 years ago

If we can replace the spinner with something that consumes no vertical space (to have to pop-in and out) then that would help

t3chguy commented 2 years ago

We can probably hack BACAT to remove the paging behaviour for the threads list, as long as we can turn the spinner/no-spinner into a constant height element. Like the old youtube top-bar horizontal animation which I cannot find a reference to atm

janogarcia commented 2 years ago

If we ca n replace the spinner with something that consumes no vertical space (to have to pop-in and out) then that would help

What if we did something like this?

Let's say Item 09 was already available on first load. Then, we'd draw the spinner over the existing content when fetching more items (Item 10 and up). Would that make it any easier?

Frame 6598
janogarcia commented 2 years ago

Like the old youtube top-bar horizontal animation which I cannot find a reference to atm

Can't recall that one, but would to see an example.

t3chguy commented 2 years ago

Yup an overlaid loading state would work too, anything that doesn't cause the vertical bounce

janogarcia commented 2 years ago

Alternatively, we could just show an animated loading bar on the bottom that would only take up 2 additional pixels. We could just keep that 2px gap on the bottom even when the loader is inactive. We could live with that, even if not ideal.

Frame 6598 (2)

We could also combine both for better feedback on what's being loaded, but it's probably getting a bit noisy.

Frame 6598 (1)
t3chguy commented 2 years ago

https://www.cssscript.com/demo/youtube-top-loader/ the loading bar was what I had in mind, combining both is also possible

janogarcia commented 2 years ago

https://www.cssscript.com/demo/youtube-top-loader/

Thanks for the example! Yeah, basically the same stuff I suggested in the previous example. 👍

t3chguy commented 2 years ago

I just noticed Github has such a minimal loading animation also :D

daniellekirkwood commented 1 year ago

believe this is completed and can therefore be closed...

janogarcia commented 1 year ago

@daniellekirkwood Related internal discussion on BACAT scrolling and sorting.

That said, we probably don't plan to act on the scrolling implementation soon, so we may want to consider that as a standalone enhancement (affecting the threads list, but also the mentions and files panels) that we can separate from the Threads project, as suggested previously.

daniellekirkwood commented 1 year ago

Created an issue to keep track of the scrolling things here

Will now close this issue.