diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-live-pane.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-live-pane.hbs index a9bd5db6a71..e10dbc9fa09 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-live-pane.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-live-pane.hbs @@ -40,17 +40,16 @@
- {{#if this.loadedOnce}} - {{#each @channel.messages key="id" as |message|}} {{/each}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-live-pane.js b/plugins/chat/assets/javascripts/discourse/components/chat-live-pane.js index f3d303c3f55..f907ac6f3a6 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-live-pane.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-live-pane.js @@ -53,14 +53,8 @@ export default class ChatLivePane extends Component { @tracked needsArrow = false; @tracked loadedOnce = false; - isAtBottom = true; - isTowardsBottom = false; - isTowardsTop = false; - isAtTop = false; - _loadedChannelId = null; _scrollerEl = null; - _previousScrollTop = 0; _lastSelectedMessage = null; _mentionWarningsSeen = {}; _unreachableGroupMentions = []; @@ -97,6 +91,8 @@ export default class ChatLivePane extends Component { @action updateChannel() { + this.loadedOnce = false; + // Technically we could keep messages to avoid re-fetching them, but // it's not worth the complexity for now this.args.channel?.clearMessages(); @@ -109,7 +105,7 @@ export default class ChatLivePane extends Component { } this.loadMessages(); - this._subscribeToUpdates(this.args.channel.id); + this._subscribeToUpdates(this.args.channel?.id); } @action @@ -158,8 +154,6 @@ export default class ChatLivePane extends Component { return; } - this.loadedOnce = false; - this._previousScrollTop = 0; this.loadingMorePast = true; const findArgs = { pageSize: PAGE_SIZE }; @@ -167,7 +161,8 @@ export default class ChatLivePane extends Component { if (this.requestedTargetMessageId) { findArgs["targetMessageId"] = this.requestedTargetMessageId; } else if (fetchingFromLastRead) { - findArgs["targetMessageId"] = this._getLastReadId(); + findArgs["targetMessageId"] = + this.args.channel.currentUserMembership.last_read_message_id; } return this.chatApi @@ -208,6 +203,7 @@ export default class ChatLivePane extends Component { this.requestedTargetMessageId = null; this.loadingMorePast = false; this.fillPaneAttempt(); + this.updateLastReadMessage(); }); } @@ -224,7 +220,7 @@ export default class ChatLivePane extends Component { !canLoadMore || this.loading || this[loadingMoreKey] || - !this.args.channel.messages.length + !this.args.channel.messages?.length > 0 ) { return Promise.resolve(); } @@ -254,7 +250,7 @@ export default class ChatLivePane extends Component { // prevents an edge case where user clicks bottom arrow // just after scrolling to top - if (loadingPast && this.isAtBottom) { + if (loadingPast && this.#isAtBottom()) { return; } @@ -268,7 +264,6 @@ export default class ChatLivePane extends Component { } this.args.channel.details = meta; - this.args.channel.addMessages(messages); // Edge case for IOS to avoid blank screens @@ -293,14 +288,14 @@ export default class ChatLivePane extends Component { }); } - @debounce(500, false) + @debounce(500) fillPaneAttempt() { if (this._selfDeleted) { return; } // safeguard - if (this.args.channel.messages.length > 200) { + if (this.args.channel.messages?.length > 200) { return; } @@ -313,9 +308,7 @@ export default class ChatLivePane extends Component { return; } - this.fetchMoreMessages({ - direction: PAST, - }); + this.fetchMoreMessages({ direction: PAST }); } @bind @@ -361,10 +354,6 @@ export default class ChatLivePane extends Component { return [messages, results.meta]; } - _getLastReadId() { - return this.args.channel.currentUserMembership.last_read_message_id; - } - @debounce(100) highlightOrFetchMessage(messageId) { const message = this.args.channel.findMessage(messageId); @@ -423,30 +412,52 @@ export default class ChatLivePane extends Component { } @action - didShowMessage(message) { + messageDidEnterViewport(message) { message.visible = true; - this.updateLastReadMessage(message); } @action - didHideMessage(message) { + messageDidLeaveViewport(message) { message.visible = false; } @debounce(READ_INTERVAL_MS) updateLastReadMessage() { - if (this._selfDeleted) { - return; - } + schedule("afterRender", () => { + if (this._selfDeleted) { + return; + } + + const lastReadId = + this.args.channel.currentUserMembership?.last_read_message_id; + let lastUnreadVisibleMessage = this.args.channel.visibleMessages.findLast( + (message) => !lastReadId || message.id > lastReadId + ); + + // all intersecting messages are read + if (!lastUnreadVisibleMessage) { + return; + } + + const element = this._scrollerEl.querySelector( + `[data-id='${lastUnreadVisibleMessage.id}']` + ); + + // if the last visible message is not fully visible, we don't want to mark it as read + // attempt to mark previous one as read + if (!this.#isBottomOfMessageVisible(element, this._scrollerEl)) { + lastUnreadVisibleMessage = lastUnreadVisibleMessage.previousMessage; + + if ( + !lastUnreadVisibleMessage && + lastReadId > lastUnreadVisibleMessage.id + ) { + return; + } + } - const lastReadId = - this.args.channel.currentUserMembership?.last_read_message_id; - const lastUnreadVisibleMessage = this.args.channel.visibleMessages.findLast( - (message) => !lastReadId || message.id > lastReadId - ); - if (lastUnreadVisibleMessage) { this.args.channel.updateLastReadMessage(lastUnreadVisibleMessage.id); - } + }); } @action @@ -454,7 +465,7 @@ export default class ChatLivePane extends Component { schedule("afterRender", () => { if (this.args.channel.canLoadMoreFuture) { this._fetchAndScrollToLatest(); - } else { + } else if (this.args.channel.messages?.length > 0) { this.scrollToMessage( this.args.channel.messages[this.args.channel.messages.length - 1].id ); @@ -463,51 +474,31 @@ export default class ChatLivePane extends Component { } @action - computeScrollState(event) { - if (this._selfDeleted) { - return; - } + computeArrow() { + this.needsArrow = Math.abs(this._scrollerEl.scrollTop) >= 100; + } + @action + computeScrollState() { cancel(this.onScrollEndedHandler); - this.isScrolling = true; - - const scrollPosition = Math.abs(event.target.scrollTop); - const total = event.target.scrollHeight - event.target.clientHeight; - const difference = total - scrollPosition; - this.isTowardsTop = difference >= 50 && difference <= 150; - this.isTowardsBottom = scrollPosition >= 50 && scrollPosition <= 150; - this.needsArrow = scrollPosition >= 50; - this.isAtBottom = scrollPosition < 50; - this.isAtTop = difference < 50; - - if (this._previousScrollTop - scrollPosition <= 0) { - if (this.isAtTop) { - this.fetchMoreMessages({ direction: PAST }); - } + if (this.#isAtTop()) { + this.fetchMoreMessages({ direction: PAST }); + this.onScrollEnded(); + } else if (this.#isAtBottom()) { + this.updateLastReadMessage(); + this.hasNewMessages = false; + this.fetchMoreMessages({ direction: FUTURE }); + this.onScrollEnded(); } else { - if (this.isAtBottom) { - this.fetchMoreMessages({ direction: FUTURE }); - } + this.isScrolling = true; + this.onScrollEndedHandler = discourseLater(this, this.onScrollEnded, 150); } - - this._previousScrollTop = scrollPosition; - this.onScrollEndedHandler = discourseLater(this, this.onScrollEnded, 150); } @bind onScrollEnded() { this.isScrolling = false; - - if (this.isAtBottom) { - this.hasNewMessages = false; - } - } - - _isBetween(target, a, b) { - const min = Math.min.apply(Math, [a, b]); - const max = Math.max.apply(Math, [a, b]); - return target > min && target < max; } removeMessage(msgData) { @@ -594,7 +585,7 @@ export default class ChatLivePane extends Component { if (this.args.channel.canLoadMoreFuture) { // If we can load more messages, we just notice the user of new messages this.hasNewMessages = true; - } else if (this.isAtBottom || this.isTowardsBottom) { + } else if (this.#isTowardsBottom()) { // If we are at the bottom, we append the message and scroll to it const message = ChatMessage.create(this.args.channel, data.chat_message); this.args.channel.addMessages([message]); @@ -1141,6 +1132,10 @@ export default class ChatLivePane extends Component { } _subscribeToUpdates(channelId) { + if (!channelId) { + return; + } + this._unsubscribeToUpdates(channelId); this.messageBus.subscribe( `/chat/${channelId}`, @@ -1277,4 +1272,26 @@ export default class ChatLivePane extends Component { }); }); } + + #isAtBottom() { + return Math.abs(this._scrollerEl.scrollTop) <= 2; + } + + #isTowardsBottom() { + return Math.abs(this._scrollerEl.scrollTop) <= 50; + } + + #isAtTop() { + return ( + Math.abs(this._scrollerEl.scrollTop) >= + this._scrollerEl.scrollHeight - this._scrollerEl.offsetHeight - 2 + ); + } + + #isBottomOfMessageVisible(element, container) { + const rect = element.getBoundingClientRect(); + const containerRect = container.getBoundingClientRect(); + // - 1.0 to account for rounding errors, especially on firefox + return rect.bottom - 1.0 <= containerRect.bottom; + } } diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-message.hbs index 78933c25feb..da06ec0699d 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-message.hbs @@ -56,8 +56,10 @@ }} data-id={{@message.id}} {{chat/track-message - (fn @didShowMessage @message) - (fn @didHideMessage @message) + (hash + didEnterViewport=(fn @messageDidEnterViewport @message) + didLeaveViewport=(fn @messageDidLeaveViewport @message) + ) }} > {{#if this.show}} diff --git a/plugins/chat/assets/javascripts/discourse/modifiers/chat/track-message.js b/plugins/chat/assets/javascripts/discourse/modifiers/chat/track-message.js index 469188eaa32..519113ed822 100644 --- a/plugins/chat/assets/javascripts/discourse/modifiers/chat/track-message.js +++ b/plugins/chat/assets/javascripts/discourse/modifiers/chat/track-message.js @@ -3,23 +3,23 @@ import { registerDestructor } from "@ember/destroyable"; import { bind } from "discourse-common/utils/decorators"; export default class ChatTrackMessage extends Modifier { - visibleCallback = null; - notVisibleCallback = null; + didEnterViewport = null; + didLeaveViewport = null; constructor(owner, args) { super(owner, args); registerDestructor(this, (instance) => instance.cleanup()); } - modify(element, [visibleCallback, notVisibleCallback]) { - this.visibleCallback = visibleCallback; - this.notVisibleCallback = notVisibleCallback; + modify(element, [callbacks = {}]) { + this.didEnterViewport = callbacks.didEnterViewport; + this.didLeaveViewport = callbacks.didLeaveViewport; this.intersectionObserver = new IntersectionObserver( this._intersectionObserverCallback, { root: document, - threshold: 0.9, + threshold: 0, } ); @@ -34,9 +34,9 @@ export default class ChatTrackMessage extends Modifier { _intersectionObserverCallback(entries) { entries.forEach((entry) => { if (entry.isIntersecting) { - this.visibleCallback?.(); + this.didEnterViewport?.(); } else { - this.notVisibleCallback?.(); + this.didLeaveViewport?.(); } }); } diff --git a/plugins/chat/assets/stylesheets/common/chat-message-left-gutter.scss b/plugins/chat/assets/stylesheets/common/chat-message-left-gutter.scss index 955082fe5fb..bad68f19ce7 100644 --- a/plugins/chat/assets/stylesheets/common/chat-message-left-gutter.scss +++ b/plugins/chat/assets/stylesheets/common/chat-message-left-gutter.scss @@ -6,16 +6,15 @@ width: var(--message-left-width); } +.chat-message-container.is-hovered .chat-message-left-gutter { + .chat-time { + color: var(--secondary-mediumy); + } +} + .chat-message-left-gutter__date { color: var(--primary-high); font-size: var(--font-down-1); - - &:hover, - &:focus { - .chat-time { - color: var(--primary); - } - } } .chat-message-left-gutter__flag { diff --git a/plugins/chat/spec/system/update_last_read.rb b/plugins/chat/spec/system/update_last_read.rb new file mode 100644 index 00000000000..4941712ad28 --- /dev/null +++ b/plugins/chat/spec/system/update_last_read.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +RSpec.describe "Update last read", type: :system, js: true do + fab!(:current_user) { Fabricate(:user) } + fab!(:channel_1) { Fabricate(:chat_channel) } + fab!(:first_unread) { Fabricate(:chat_message, chat_channel: channel_1) } + + let(:chat_page) { PageObjects::Pages::Chat.new } + let(:channel_page) { PageObjects::Pages::ChatChannel.new } + let(:membership) { Chat::ChatChannelMembershipManager.new(channel_1).find_for_user(current_user) } + + before do + chat_system_bootstrap + channel_1.add(current_user) + membership.update!(last_read_message_id: first_unread.id) + 25.times { |i| Fabricate(:chat_message, chat_channel: channel_1) } + sign_in(current_user) + end + + context "when the full message is not visible" do + it "doesn’t mark it as read" do + before_last_message = Fabricate(:chat_message, chat_channel: channel_1) + last_message = Fabricate(:chat_message, chat_channel: channel_1) + chat_page.visit_channel(channel_1) + + page.execute_script("document.querySelector('.chat-messages-scroll').scrollTo(0, -5)") + + try_until_success(timeout: 5) do + expect(membership.reload.last_read_message_id).to eq(before_last_message.id) + end + end + end + + context "when the full message is visible" do + it "marks it as read" do + last_message = Fabricate(:chat_message, chat_channel: channel_1) + chat_page.visit_channel(channel_1) + + page.execute_script("document.querySelector('.chat-messages-scroll').scrollTo(0, 0)") + + try_until_success(timeout: 5) do + expect(membership.reload.last_read_message_id).to eq(last_message.id) + end + end + end + + context "when user had not previous last read" do + before { membership.update!(last_read_message_id: nil) } + + it "marks last message as read" do + last_message = Fabricate(:chat_message, chat_channel: channel_1) + chat_page.visit_channel(channel_1) + + try_until_success(timeout: 5) do + expect(membership.reload.last_read_message_id).to eq(last_message.id) + end + end + end + + context "when scrolling from not visible to bottom" do + it "marks last message as read" do + before_last_message = Fabricate(:chat_message, chat_channel: channel_1) + last_message = Fabricate(:chat_message, chat_channel: channel_1) + chat_page.visit_channel(channel_1) + + page.execute_script("document.querySelector('.chat-messages-scroll').scrollTo(0, -15)") + + try_until_success(timeout: 5) do + expect(membership.reload.last_read_message_id).to eq(before_last_message.id) + end + + page.execute_script("document.querySelector('.chat-messages-scroll').scrollTo(0, -1)") + + try_until_success(timeout: 5) do + expect(membership.reload.last_read_message_id).to eq(last_message.id) + end + end + end +end diff --git a/plugins/chat/test/javascripts/components/chat-message-test.js b/plugins/chat/test/javascripts/components/chat-message-test.js index d75f7fd5284..e220825339f 100644 --- a/plugins/chat/test/javascripts/components/chat-message-test.js +++ b/plugins/chat/test/javascripts/components/chat-message-test.js @@ -55,8 +55,8 @@ module("Discourse Chat | Component | chat-message", function (hooks) { onSelectMessage: () => {}, bulkSelectMessages: () => {}, onHoverMessage: () => {}, - didShowMessage: () => {}, - didHideMessage: () => {}, + messageDidEnterViewport: () => {}, + messageDidLeaveViewport: () => {}, forceRendering: () => {}, }; } @@ -74,9 +74,8 @@ module("Discourse Chat | Component | chat-message", function (hooks) { @onSelectMessage={{this.onSelectMessage}} @bulkSelectMessages={{this.bulkSelectMessages}} @onHoverMessage={{this.onHoverMessage}} - @didShowMessage={{this.didShowMessage}} - @didHideMessage={{this.didHideMessage}} - @didHideMessage={{this.didHideMessage}} + @messageDidEnterViewport={{this.messageDidEnterViewport}} + @messageDidLeaveViewport={{this.messageDidLeaveViewport}} @forceRendering={{this.forceRendering}} /> `;