Skip to content
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

Fix scroll to bottom issue in #61. #685

Closed
wants to merge 1 commit into from

Conversation

ntoll
Copy link
Contributor

@ntoll ntoll commented Jan 7, 2020

Description

Fixes #61.

I've undone some changes that were applied to the original code which ensured that when new messages were added the pane would scroll to the bottom. I'm unsure why this code would have been added. AFAICT, when a new message is added to the pane, you should be able to see it!

Before merging, I'd like to explore what problems I may be re-introducing by removing this code (and associated test). The comment in the test appears to suggest that the code was added to avoid scrolling if the journalist was reading an older message. I feel these requirements are mutually exclusive, IYSWIM, so some sort of UX decision / discussion should probably happen (pinging @ninavizz).

Test Plan

This is removal of code and an associated unit test. Further UX discussion is required and I expect further revisions to the code to reflect the outcome of such discussions.

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

  • I have tested these changes in the appropriate Qubes environment
  • I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
  • These changes should not need testing in Qubes

@eloquence
Copy link
Member

I feel these requirements are mutually exclusive, IYSWIM, so some sort of UX decision / discussion should probably happen (pinging @ninavizz).

It's important to distinguish between:

  • messages that a source sends
  • replies the journalists sends.

By way of comparison, Slack will always scroll to the bottom if you send something, but it will keep your scroll position if another user sends something. In that case, it will show a "jump to new message" bar at the top.

That "jump to new message" bar is important -- it would be problematic to keep the scroll position without showing some indicator that new messages from a source have arrived.

Since we can safely assume that real-time interactions are pretty rare, for the beta, it's IMO fine to just always jump to the bottom of the conversation view in both cases for now. Nina, please jump in if you disagree.

@ntoll
Copy link
Contributor Author

ntoll commented Jan 8, 2020

@eloquence thanks for the feedback. I imagined you'd reply with something like you did (but I didn't want to just start changing UI behaviour without flagging things and getting some feedback).

AFAICT the behaviour can be encapsulated in this sentence: do not scroll to the bottom unless the new message is from the logged in journalist. The scroll to new message (a la Slack) is perhaps a new feature to be explored from a UX PoV before you let me get my grubby hands on it. ;-)

@ntoll
Copy link
Contributor Author

ntoll commented Jan 8, 2020

OK... I've been spending some more time looking into this thinking I could enhance the current state of this PR (which has the behaviour described by @eloquence -- i.e. scroll to bottom for all messages), so that only messages from the journalist cause the scroll to bottom.

It turns out that every time a new message is sent or received the conversation is cleared and then re-added to the widget (see the update_conversation method), rather than the new item being appended to the end of the currently rendered conversation. This feels odd / wrong to me and I've been experimenting with more efficient / less destructive ways to do this.

However, as it stands this PR is ready to merge, but I wanted to flag such odd behaviour found while investigating this issue during today.

@eloquence
Copy link
Member

so that only messages from the journalist cause the scroll to bottom.

To be clear, I think without a way to jump to new messages, that could be very confusing -- you might find yourself looking at an old message and not notice that a new one has arrived from the source. So for now I think the behavior implemented in this PR is in fact preferable, until such a time we have a UX design for highlighting new messages that could be implemented alongside further enhancements.

@ntoll
Copy link
Contributor Author

ntoll commented Jan 8, 2020

👍

@eloquence
Copy link
Member

Cross-referencing the related #635 in case it's still an issue.

Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This resolves #635

However it also introduces a regression, see: #144. When the user clicks "DOWNLOAD", they might also want to open, print, or export it. This should be fixed before merging.

It is also a regression to scroll to the bottom when the user has the conversation loaded and is positioned somewhere in the middle of a thread (I will try to find a relevant ticket with background info). It could be a long conversation or a long single message from a source that they are in the middle of (which is why it's the conversation that is being actively viewed in the first place). Moving the user mid-view/read is something we've discussed in design meetings so others may want to chime in.

@eloquence
Copy link
Member

eloquence commented Jan 10, 2020

It is also a regression to scroll to the bottom when the user has the conversation loaded and is positioned somewhere in the middle of a thread

If you mean "scroll to the bottom if a message arrives during that time", as noted above, that's IMO preferable to potentially missing the new message entirely. The ideal long term solution here would be to have a Slack-like "Jump to new message" appear without scrolling the conversation view (while always scrolling it for journalist-initiated replies), but given that real-time conversations are the exception in SecureDrop, I think this is an acceptable solution for the beta. Happy to discuss further, of course.

If you're describing a different "scroll to the bottom" scenario, could you elaborate?

@sssoleileraaa
Copy link
Contributor

I recorded the a video to clarify the two regressions I mentioned above (a) clicking on the "Download" button now auto-scrolls you to the bottom of the conversation and you will have to scroll back up to open it, and (b) when a new message comes in (this will happen during a background sync) for the conversation that you are in the middle of reading the client auto-scroll you to the bottom of the conversation and you will have to scroll back up to open it.

The design decision we made at the end of last year was to only "stick" to the bottom of the conversation if the user is already at the bottom of the conversation or if the user loads a new conversation by clicking on a source.

Note: I click on the refresh icon instead of waiting for a background sync in this recording for demo purposes

auto-scrolling-on-download-and-sync

@eloquence
Copy link
Member

eloquence commented Jan 10, 2020

(a) clicking on the "Download" button now auto-scrolls you to the bottom of the conversation and you will have to scroll back up to open it,

That's clearly broken.

(b) when a new message comes in (this will happen during a background sync) for the conversation that you are in the middle of reading the client auto-scroll you to the bottom of the conversation and you will have to scroll back up to open it.

IMO that's the right behavior for now, until we have a new message indicator. It's arguably worse if the new message comes in and the user never sees it because they were scrolled up.

In any event, if we change this further, then we should still make sure that the user's own replies always trigger a scroll.

@sssoleileraaa
Copy link
Contributor

sssoleileraaa commented Jan 10, 2020

IMO that's the right behavior for now, until we have a new message indicator. It's arguably worse if the new message comes in and the user never sees it because they were scrolled up.

#187 is a high-priority issue to show users when new messages come in and indicating it per source in the source list. I think changing the code to auto-scroll to the bottom when a new message comes in while the user is viewing the conversation in a different location is unnecessary given that this is not the design we're working towards.

I believe the scope of this PR is to (a) resolve #635 which is, as you pointed out, making sure that when a user sends a reply we don't scroll to the top of the conversation, and (b) make sure we continue to autoscroll to the bottom when new messages come in only if the user is already at the bottom of the conversation view-- we already have code for this that deeplow added.

@eloquence
Copy link
Member

eloquence commented Jan 10, 2020

I think changing the code to auto-scroll to the bottom when a new message comes in is unnecessary given that this is not the design we're working towards.

That's fair; my main point is that I don't think its a huge deal either way at this point and there are downsides to this approach in the absence of newness indicators, so we should go with what's quickest.

(b) make sure we continue to autoscroll to the bottom when new messages come in only if the user is already at the bottom of the conversation view-- we already have code for this that deeplow added.

We should IMO autoscroll if they're not at the bottom, but sending a reply. That's at least how Slack does it, and pretty common messaging app behavior.

@ntoll
Copy link
Contributor Author

ntoll commented Jan 13, 2020

Morning folks.

My summary of the story so far...

From my PoV everything @creviera says makes sense, except there were bugs such that the scrolling didn't work in the way described (i.e. the scrolling was jumping around -- the problem I was assigned to fix). Yet my work (just scroll to the bottom) has introduced a regression. Also, @eloquence has made the point that something must be done quickly so this aspect of the client is broken for release.

So what to do..? ;-)

To synthesis all the suggestions we ger:

  • Don't scroll when a new message is added because of the problems @creviera mentions.
  • Except in one single exceptional circumstance, the user of the client is posting a new message (i.e. they can't be doing anything else with conversation since they're writing to it), and in that case, scroll to the bottom so they immediately see their new message is added to the conversation -- a sort of UI confirmation that their contribution has worked (which is @eloquence's suggestion).

For the sake of "something must be done", I'll fix up this PR to reflect the behaviour in the two points mentioned above. At least this addresses both aspects of the UX ("don't scroll, I'm doing something" and "I expect to see my new message"). This will mean the UI is working without bugs and consistently. I expect this behaviour will be re-visited as we work out what more we may need to do given actual user feedback and the potential for readjustment and refinement (for instance a "scroll to bottom" button).

My personal take is that, for now, "it'll do" and we can refine in future.

I hope this makes sense.

@ntoll
Copy link
Contributor Author

ntoll commented Jan 13, 2020

OK... this PR should be dropped because the changes made in #688 make the solution possible.

The technical explanation follows:

  • When a new message is added to the conversation, as things stand in this branch, we clear all the messages and re-add completely new speech widgets for the current state of the conversation.
  • However, if the new message is a reply composed by the current user, the speech widget is appended to the end of the current conversation so the update appears immediately to the user who created the message. (Only when the server responds with the updated conversation as a response to us sending the user's message does the activity described above take place).
  • During this process the height of the conversation widget is first set to the new height of the conversation + new message's height. Then the height is set to 0 (when the conversation is cleared), and then (a third time) when we re-render the complete conversation afresh. Each time the height of the conversation is set the update_conversation_position method is called as a result of the firing of the scroll bar's rangeChanged signal. This explains the flickering.
  • However, when setting a flag to show that the user has sent a reply (and then resetting the flag once the update_conversation_position does its job, the triple event problem kicks in again: The position is always set to the bottom on the first event and the flag is reset. However, when the conversation is cleared the bottom now becomes position 0, 0 (since there's no content). Finally, when the messages are all recreated, then, since the flag is no longer set, the scrolling down to the bottom doesn't take place. This is why the work on Inline conversation updates. Fixes #473. #688 makes it possible for the correct scrolling behaviour to take place because the signal that calls update_conversation_position only happens once when the user replies.

Phew..! Apologies for dumping "War and Peace" length explanations on tickets, but I also don't want you to have to go through the unpicking of code I've just had to go through. Brain share... it's a thing.! ;-)

ntoll added a commit to ntoll/securedrop-client that referenced this pull request Jan 13, 2020
@sssoleileraaa
Copy link
Contributor

Thanks for the explanation @ntoll We can add the appropriate "resolves" "fixes" strings to #688 then and close this PR

ntoll added a commit to ntoll/securedrop-client that referenced this pull request Jan 29, 2020
ntoll added a commit to ntoll/securedrop-client that referenced this pull request Jan 29, 2020
Fix freedomofpress#61 to supercede freedomofpress#685.
Make on_reply_sent work properly without duplicate entries.
sssoleileraaa pushed a commit that referenced this pull request Jan 29, 2020
Fix #61 to supercede #685.
Make on_reply_sent work properly without duplicate entries.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

conversation should appear scrolled all the way to the bottom
3 participants