firefox-devtools / ux

Firefox DevTools UX Community
Mozilla Public License 2.0
103 stars 21 forks source link

WebSocket Inspector: Warnings that the messages table is truncated #83

Closed digitarald closed 5 years ago

digitarald commented 5 years ago

Follow up for #71. Bug.

Context: WebSocket messages need to be discarded so memory and render times stay within limits. The limit we picked for now is 1000, Chrome keeps 100.

User stories:

Initial UX: https://www.figma.com/file/OWflRU1bKHXMR6nh921AQiMD/Network-WebSocket-Inspector?node-id=25%3A2

Constrains:

  1. Prefer to not interrupt users; as most just want to see new messages come in
    • Inline with table and only visible when scrolling to the top (oldest messages), so easy to discover when looking for the oldest messages
    • Trade off more muted vs more alert style, given that this is only visible when scrolling up
  2. Make it easy to discover for users who look for older messages
  3. Include how many messages got discarded so far (related, status bar story from backlog, which shows stats for all/filtered messages)

cc @janodvarko

digitarald commented 5 years ago

cc @fvsch as well, who has been helping a lot with past UX on WS Inspector; and @nchevobbe who is reviewing patches while Honza is out.

fvsch commented 5 years ago

What does "discarded" mean precisely?

Sounds like we purged the data ("data loss"), but I want to be sure I'm understanding the opportunities and constraints.

digitarald commented 5 years ago

What does "discarded" mean precisely?

or we forgot about this data (purged it)?

Yes. As this is about the memory constrains of retaining messages, the data would be forgotten. Another word that we also used here was "truncated".

Just for posterity, one idea to explore would be that forgetting data is informed by accumulated message size constrains.

fvsch commented 5 years ago

Gotcha, so it can't be a "click to see older messages" like we have in the Inspector for nodes with many children.

On your mockup:

"Discarded N older messages"

"Disable temporarily"

fvsch commented 5 years ago

Maybe something like:

Discarded N older messages
[ ] Keep all messages (⚠️ can impact performance)

Note: it might be a better overall UX to not offer the "disable temporarily" or "keep all messages" if it's a footgun (might slow down devtools), or if we need to uncheck that user preference automatically based on opaque heuristics (might frustrate users).

digitarald commented 5 years ago

if it's a footgun (might slow down devtools)

Manually clearing the table is also possible when things get slow.

I did only spend a small amount on wording, so we can wordsmith a bit more …

Warning:

Button:

nchevobbe commented 5 years ago

For me, we should avoid any "negative" language, that might confuse the user ("discard" feels like something was done at content level)

We could only have a button that says "show older messages" (pretty common UI pattern before infinite scroll came in). That could only load the previous X messages (where X is the limit), so we would have "paging" kindof. We could have another button saying "Show all messages" with a warning on performance.

digitarald commented 5 years ago

We could only have a button that says "show older messages" (pretty common UI pattern before infinite scroll came in).

Good point. It only lacks the opt-in nature of keeping messages. Show all messages for this session would do this and be positive.

That could only load the previous X messages (where X is the limit), so we would have "paging" kindof.

The assumption (see https://github.com/firefox-devtools/ux/issues#issuecomment-514756448) is that older messages actually get discarded.

violasong commented 5 years ago

"Show Older Messages (240)" with paging seems like a great solution! It would make sense to use neutral gray styling instead of the yellow. The whole thing could be a centered button of the Photon micro style.

digitarald commented 5 years ago

"Show Older Messages (240)" with paging

I would love keeping older messages too, but memory is a concern here. We are not able to show the older messages that got discarded. Per my prior comment, the network panel throws away older messages; which means paging is not an option.

violasong commented 5 years ago

Oh, I see, the other type of 'message bar' pattern makes sense then.

If we do have an option to keep all messages instead, then I guess I'd ideally want to see that checkbox somewhere else, rather than in the warning that explains this after the fact. E.g. Console Persist Logs.

What if there's always a message that shows up at the beginning of the rows (so it would be the only thing in the panel at first), and it looked like this:

ℹ️ Messages are truncated after 1000 rows to conserve memory. [ ] Keep all messages

And then if the user actually gets past 1000, the message would change to be like this ℹ️ 224 messages have been truncated to conserve memory. [ ] Keep all future messages

digitarald commented 5 years ago

What if there's always a message that shows up at the beginning of the rows (so it would be the only thing in the panel at first), and it looked like this:

That seems very upfront. As I expect this to be a niche use case, users usually don't have to see this (Chrome doesn't show anything and only mentions the 100 message limit in their docs, which also seems too arbitrary).

fvsch commented 5 years ago

I like what Victoria suggested. Would love to warn users upfront so they don't hit this limitation and lose content, but as Harald said it's a niche use case so we probably shouldn't make all users deal with this cognitive cost. Keeping only the second message (with Harald's design and Victoria's wording) seems best.

"Keep all future messages" is great, it doesn't suggest that we're going to retrieve older messages.

nchevobbe commented 5 years ago

oh sorry, I thought we were keeping old messages (from what I understood reviewing the code). My paging proposal doesn't make sense if we don't :)

digitarald commented 5 years ago

I thought we were keeping old messages (from what I understood reviewing the code)

Right, this wasn't clearly specced out, and might change. I am only assuming that we want to somewhat prevent OOM with websocket messages (that might never be looked at by users); but we are also not truncating for network panel or console (which while they are primary panels, they also collect data while in background).

digitarald commented 5 years ago

The whole thing could be a centered button of the Photon micro style.

@violasong I am not sure where this puts the text, but it seems like we have general agreement on the discussion points. Could you summarize the discussion in a mockup?

violasong commented 5 years ago

Yep! I just made a fork of your figma :D

Here is it with Console-yellow. I think this might still look more like a warning than it should. The color does give it prominence, but not sure if it needs that.

image

Here it is with gray + info icon. This seems more correct to me.

image
digitarald commented 5 years ago

Here it is with gray + info icon. This seems more correct to me.

Agreed, gray is good enough as the row layout is already different from the rest which makes it stand out.

violasong commented 5 years ago

Great! Should I close this bug and move the design info to bugzilla?

tanhengyeow commented 5 years ago

I followed the conversation and this sounds great! I would like to clarify what happens after users click the Keep all future messages checkbox.

Does it mean messages stop getting truncated from then on?

And then when the checkbox is unchecked, do we immediately show only messages up to the limit?

For e.g.

  1. The limit is set at 100 and we have 110 messages now. The text would show "10 messages have been truncated..." with checkbox unchecked.
  2. The user checks the checkbox.
  3. Messages continue to be sent e.g. we have 130 messages now but the text still shows "10 messages have been truncated..." instead of "30 messages have been truncated..." as we would expect.
  4. The user unchecks the checkbox.
  5. At this point, users should see "30 messages have been truncated...."

Is that right? Also, what's the hex code we should use for the gray? Any SVGs we can reuse for the warning icon?

digitarald commented 5 years ago

Good point, checked state with the old label doesn't seem to make sense. The button was easier and fire-and-forget style in that regard, as it would just hide and only be active for the current session anyways.

@violasong any thoughts on if and how the msg should change when truncation is disabled? Maybe graying is out is all that is needed.

violasong commented 5 years ago

When the user checks the checkbox, it doesn't cause previously discarded messages to reappear, IIUC. So in step 3, with the checkbox on and 130 total messages sent, wouldn't "10 messages have been truncated" still be accurate? (Though it would be talking about the past, which is a little strange.) After unchecking, seems like 10 is still accurate until a new message arrives and makes it 11. We could make it more clear with two different states for the banner, if it seems worth the extra development effort.

Also, what's the hex code we should use for the gray? Any SVGs we can reuse for the warning icon?

There's a variable for this color called Gray 20. In dark mode we can use Gray 70. The icon should also exist already in the DevTools codebase. Note there's also a bottom border that's the same color as the main DevTools border color.

violasong commented 5 years ago

A more clear solution could have two buttons instead of a single checkbox. This messaging wouldn't preserve the number of messages truncated if the user is in "keep all" mode.

224 messages have been truncated to conserve memory. [Keep all future messages] Showing 1,240 messages. [Truncate messages past 1000]

digitarald commented 5 years ago

Taking a step back, some state is represented in the last mockup; while the user looks at the warning on high frequency messages, the counter will increase; and checking the box will stop that.

I am fine with the simple solution and not having too much state – mainly to bias on smaller scope, given this is only ever used by a small subset of users.

violasong commented 5 years ago

Yeah, I prefer the simple solution as well - the straightforwardness of the checkbox with its own built-in overview of the states seems nice.

tanhengyeow commented 5 years ago

Yep! I just made a fork of your figma :D

Here is it with Console-yellow. I think this might still look more like a warning than it should. The color does give it prominence, but not sure if it needs that.

image

Here it is with gray + info icon. This seems more correct to me.

image

To sum up, we will implement the UI according to this mockup. Once users check the checkbox, all future messages will be kept for that session. We will use Gray 20 in Light mode and Gray 70 in Dark mode. Also, we shall use an existing warning icon.

I suggest we disable the checkbox with the checkmark after the first click so that users would not be confused thinking that checking/unchecking the checkbox would toggle the state.

Anything I missed out?

fvsch commented 5 years ago

I suggest we disable the checkbox with the checkmark after the first click

We shouldn't do that with a checkbox. By convention, checkboxes are like switches: users should be able to turn things on and off again. If we want to make it a one-time action, a button would be more appropriate.

tanhengyeow commented 5 years ago

Sounds good! To make sure I'm getting the flow right, is this scenario what users should expect? Let's say the limit is set to 100.

  1. 110 total messages are sent.
  2. The text shows 10 messages have been truncated to conserve memory with the button Keep all future messages displayed. At this point, 100 messages are displayed to the user.
  3. The user clicks on the button and the text disappears.
  4. The user continues to send frames e.g. there are a total of 140 (+30 from step 2) messages now.
  5. The user should see 130 messages since truncation doesn't happen anymore.
violasong commented 5 years ago

I suggest we disable the checkbox with the checkmark after the first click so that users would not be confused thinking that checking/unchecking the checkbox would toggle the state.

My understanding was that the user should be able to uncheck the checkbox to toggle the state (go back to truncating after x messages). @digitarald?

tanhengyeow commented 5 years ago

Apologies, I thought the suggestion was to use a button instead but that was a response to the suggestion of disabling the checkbox.

Let's say the limit is set to 100. So the flow should be this instead:

  1. 110 total messages are sent.
  2. The text shows 10 messages have been truncated to conserve memory with the checkbox Keep all future messages displayed. At this point, 100 messages are displayed to the user.
  3. The user checks the checkbox (User still sees 100 messages, the current limit is 110 - 10 = 100).
  4. The user continues to send frames e.g. there are a total of 140 (+30 from step 2) messages now (User sees 10 messages have been truncated to conserve memory, latest 130 messages are visible to them).
  5. The user unchecks the checkbox (We should truncate now).
  6. The user continues to send frames e.g. there are a total of 200 (+60 from step 4) messages now.
  7. The user now sees 70 messages have been truncated to conserve memory. Latest 130 messages are visible to them. The new limit is 200 - 70 = 130.

If everyone's on the same page I shall go ahead and implement this 😄

digitarald commented 5 years ago

@tanhengyeow the slightly surprising part in 7 for me would be that the limit is updated to what is currently showing (in this case changes from 100 to 130). I think keeping it 100 is less surprising.

tanhengyeow commented 5 years ago

@digitarald With 4 in context (140 total messages), once 5 is executed, does it mean the user now sees 40 messages have been truncated to conserve memory, and the latest 100 messages are visible to them? This behavior would ensure that the limit remains at 100.

digitarald commented 5 years ago

@tanhengyeow yes, that would be my expectation. In other words, whenever the truncation is enabled again, the configured limit would be applied, reducing the rows to that fixed number.

tanhengyeow commented 5 years ago

@digitarald Great! The latest patch exhibits this intended behavior. 😄

digitarald commented 4 years ago

Leaving this here for reference, Slack's truncated attachment msg

image