From 91bbdf41f44226031599be2fdb573f7175a82ef6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Theys?= Date: Mon, 16 Nov 2020 12:36:47 +0000 Subject: [PATCH] [FIX] mail: fix race conditions, especially with scroll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `_update` of some components must take place before the `_update` of other (usually sibling) components that might directly influence their layout. This is in particular the case for `_update` that are relying on height. Prevent force save of scroll position if there are pending hints to adjust it, and prevent pending hints from overriding new scroll value. Prevent flicker with message date. This should fix all known issues related to incorrectly saving or restoring scroll position. task-2344226 task-2358066 task-2369332 task-2372339 task-2373741 closes odoo/odoo#61791 X-original-commit: 2002c6a599a685f4a18fc670a9f7992c8d595c0f Related: odoo/enterprise#14759 Signed-off-by: Alexandre Kühn (aku) --- .../component_hooks/use_update/use_update.js | 64 +++++ .../src/components/chat_window/chat_window.js | 18 +- .../chat_window_manager_tests.js | 134 ++++++--- .../static/src/components/chatter/chatter.js | 13 +- .../chatter_container/chatter_container.js | 10 +- .../src/components/composer/composer.js | 10 +- .../composer_suggested_recipient.js | 10 +- .../composer_suggestion.js | 10 +- .../composer_text_input.js | 25 +- .../composer_text_input.xml | 2 +- .../static/src/components/discuss/discuss.js | 7 +- .../discuss/tests/discuss_domain_tests.js | 5 +- .../components/discuss/tests/discuss_tests.js | 272 +++++++++++++----- .../discuss_sidebar/discuss_sidebar.js | 14 +- .../emojis_popover/emojis_popover.js | 11 +- .../static/src/components/message/message.js | 27 +- .../static/src/components/message/message.xml | 2 +- .../components/message_list/message_list.js | 34 ++- .../src/components/thread_view/thread_view.js | 13 +- .../thread_view/thread_view_tests.js | 270 +++++++++-------- .../mail/static/src/models/message/message.js | 29 +- .../mail/static/src/models/thread/thread.js | 19 ++ .../src/models/thread_cache/thread_cache.js | 8 +- .../form_renderer/form_renderer_tests.js | 41 ++- .../mail/static/tests/helpers/mock_models.js | 1 + .../mail/static/tests/helpers/mock_server.js | 4 +- addons/mail/views/assets.xml | 1 + 27 files changed, 675 insertions(+), 379 deletions(-) create mode 100644 addons/mail/static/src/component_hooks/use_update/use_update.js diff --git a/addons/mail/static/src/component_hooks/use_update/use_update.js b/addons/mail/static/src/component_hooks/use_update/use_update.js new file mode 100644 index 00000000000..4d7893022cc --- /dev/null +++ b/addons/mail/static/src/component_hooks/use_update/use_update.js @@ -0,0 +1,64 @@ +odoo.define('mail/static/src/component_hooks/use_update/use_update.js', function (require) { +'use strict'; + +const { Component } = owl; +const { onMounted, onPatched } = owl.hooks; + +const executionQueue = []; + +function executeNextInQueue() { + if (executionQueue.length === 0) { + return; + } + const { component, func } = executionQueue.shift(); + if (!component.__owl__.isDestroyed) { + func(); + } + executeNextInQueue(); +} + +/** + * @param {Object} param0 + * @param {Component} param0.component + * @param {function} param0.func + * @param {integer} param0.priority + */ +async function addFunctionToQueue({ component, func, priority }) { + const index = executionQueue.findIndex(item => item.priority > priority); + const item = { component, func, priority }; + if (index === -1) { + executionQueue.push(item); + } else { + executionQueue.splice(index, 0, item); + } + // Timeout to allow all components to register their function before + // executing any of them, to respect all priorities. + await new Promise(resolve => setTimeout(resolve)); + executeNextInQueue(); +} + +/** + * This hook provides support for executing code after update (render or patch). + * + * @param {Object} param0 + * @param {function} param0.func the function to execute after the update. + * @param {integer} [param0.priority] determines the execution order of the function + * among the update function of other components. Lower priority is executed + * first. If no priority is given, the function is executed immediately. + */ +function useUpdate({ func, priority }) { + const component = Component.current; + onMounted(onUpdate); + onPatched(onUpdate); + function onUpdate() { + if (priority === undefined) { + func(); + return; + } + addFunctionToQueue({ component, func, priority }); + } +} + +return useUpdate; + +}); diff --git a/addons/mail/static/src/components/chat_window/chat_window.js b/addons/mail/static/src/components/chat_window/chat_window.js index 28e7f08a1fb..27d6bb68b68 100644 --- a/addons/mail/static/src/components/chat_window/chat_window.js +++ b/addons/mail/static/src/components/chat_window/chat_window.js @@ -7,6 +7,7 @@ const components = { ThreadView: require('mail/static/src/components/thread_view/thread_view.js'), }; const useStore = require('mail/static/src/component_hooks/use_store/use_store.js'); +const useUpdate = require('mail/static/src/component_hooks/use_update/use_update.js'); const { isEventHandled } = require('mail/static/src/utils/utils.js'); const { Component } = owl; @@ -29,6 +30,7 @@ class ChatWindow extends Component { thread: thread ? thread.__state : undefined, }; }); + useUpdate({ func: () => this._update() }); /** * Reference of the header of the chat window. * Useful to prevent click on header from wrongly focusing the window. @@ -55,11 +57,6 @@ class ChatWindow extends Component { mounted() { this.env.messagingBus.on('will_hide_home_menu', this, this._onWillHideHomeMenu.bind(this)); this.env.messagingBus.on('will_show_home_menu', this, this._onWillShowHomeMenu.bind(this)); - this._update(); - } - - patched() { - this._update(); } willUnmount() { @@ -132,7 +129,16 @@ class ChatWindow extends Component { * @private */ _saveThreadScrollTop() { - if (!this._threadRef.comp || !this.chatWindow.threadViewer) { + if ( + !this._threadRef.comp || + !this.chatWindow.threadViewer || + !this.chatWindow.threadViewer.threadView + ) { + return; + } + if (this.chatWindow.threadViewer.threadView.componentHintList.length > 0) { + // the current scroll position is likely incorrect due to the + // presence of hints to adjust it return; } this.chatWindow.threadViewer.saveThreadCacheScrollHeightAsInitial( diff --git a/addons/mail/static/src/components/chat_window_manager/chat_window_manager_tests.js b/addons/mail/static/src/components/chat_window_manager/chat_window_manager_tests.js index 1a8ded7815a..d8c62e31a87 100644 --- a/addons/mail/static/src/components/chat_window_manager/chat_window_manager_tests.js +++ b/addons/mail/static/src/components/chat_window_manager/chat_window_manager_tests.js @@ -797,11 +797,8 @@ QUnit.test('[technical] chat window: composer state conservation on toggle home QUnit.test('[technical] chat window: scroll conservation on toggle home menu', async function (assert) { // technical as show/hide home menu simulation are involved and home menu implementation // have side-effects on DOM that may make chat window components not work - assert.expect(3); + assert.expect(2); - - // channel that is expected to be found in the messaging menu - // with random unique id that is needed to link messages this.data['mail.channel'].records.push({ id: 20 }); for (let i = 0; i < 10; i++) { this.data['mail.message'].records.push({ @@ -811,9 +808,19 @@ QUnit.test('[technical] chat window: scroll conservation on toggle home menu', a } await this.start(); await afterNextRender(() => document.querySelector(`.o_MessagingMenu_toggler`).click()); - await afterNextRender(() => - document.querySelector(`.o_MessagingMenu_dropdownMenu .o_NotificationList_preview`).click() - ); + await this.afterEvent({ + eventName: 'o-component-message-list-scrolled', + func: () => document.querySelector('.o_NotificationList_preview').click(), + message: "should wait until channel 20 scrolled to its last message after opening it from the messaging menu", + predicate: ({ scrollTop, threadViewer }) => { + const messageList = document.querySelector('.o_ThreadView_messageList'); + return ( + threadViewer.thread.model === 'mail.channel' && + threadViewer.thread.id === 20 && + scrollTop === messageList.scrollHeight - messageList.clientHeight + ); + }, + }); // Set a scroll position to chat window await this.afterEvent({ eventName: 'o-component-message-list-scrolled', @@ -829,12 +836,6 @@ QUnit.test('[technical] chat window: scroll conservation on toggle home menu', a ); }, }); - assert.strictEqual( - document.querySelector(`.o_ThreadView_messageList`).scrollTop, - 142, - "chat window initial scrollTop should be 142px" - ); - await afterNextRender(() => this.hideHomeMenu()); assert.strictEqual( document.querySelector(`.o_ThreadView_messageList`).scrollTop, @@ -845,7 +846,7 @@ QUnit.test('[technical] chat window: scroll conservation on toggle home menu', a await this.afterEvent({ eventName: 'o-component-message-list-scrolled', func: () => this.showHomeMenu(), - message: "should wait until channel 20 restored its scroll to 142 after hiding the home menu", + message: "should wait until channel 20 restored its scroll to 142 after showing the home menu", predicate: ({ scrollTop, threadViewer }) => { return ( threadViewer.thread.model === 'mail.channel' && @@ -1575,9 +1576,19 @@ QUnit.test('chat window with a thread: keep scroll position in message list on f } await this.start(); await afterNextRender(() => document.querySelector(`.o_MessagingMenu_toggler`).click()); - await afterNextRender(() => - document.querySelector(`.o_NotificationList_preview`).click() - ); + await this.afterEvent({ + eventName: 'o-component-message-list-scrolled', + func: () => document.querySelector('.o_NotificationList_preview').click(), + message: "should wait until channel 20 scrolled to its last message after opening it from the messaging menu", + predicate: ({ scrollTop, threadViewer }) => { + const messageList = document.querySelector('.o_ThreadView_messageList'); + return ( + threadViewer.thread.model === 'mail.channel' && + threadViewer.thread.id === 20 && + scrollTop === messageList.scrollHeight - messageList.clientHeight + ); + }, + }); // Set a scroll position to chat window await this.afterEvent({ eventName: 'o-component-message-list-scrolled', @@ -1752,7 +1763,7 @@ QUnit.test('[technical] chat window: composer state conservation on toggle home QUnit.test('[technical] chat window with a thread: keep scroll position in message list on toggle home menu when folded', async function (assert) { // technical as show/hide home menu simulation are involved and home menu implementation // have side-effects on DOM that may make chat window components not work - assert.expect(3); + assert.expect(2); // channel that is expected to be found in the messaging menu // with random unique id, needed to link messages @@ -1765,22 +1776,48 @@ QUnit.test('[technical] chat window with a thread: keep scroll position in messa } await this.start(); await afterNextRender(() => document.querySelector(`.o_MessagingMenu_toggler`).click()); - await afterNextRender(() => - document.querySelector(`.o_MessagingMenu_dropdownMenu .o_NotificationList_preview`).click() - ); + await this.afterEvent({ + eventName: 'o-component-message-list-scrolled', + func: () => document.querySelector('.o_NotificationList_preview').click(), + message: "should wait until channel 20 scrolled to its last message after opening it from the messaging menu", + predicate: ({ scrollTop, threadViewer }) => { + const messageList = document.querySelector('.o_ThreadView_messageList'); + return ( + threadViewer.thread.model === 'mail.channel' && + threadViewer.thread.id === 20 && + scrollTop === messageList.scrollHeight - messageList.clientHeight + ); + }, + }); // Set a scroll position to chat window - document.querySelector(`.o_ThreadView_messageList`).scrollTop = 142; - assert.strictEqual( - document.querySelector(`.o_ThreadView_messageList`).scrollTop, - 142, - "should have scrolled to 142px" - ); - + await this.afterEvent({ + eventName: 'o-component-message-list-scrolled', + func: () => document.querySelector(`.o_ThreadView_messageList`).scrollTop = 142, + message: "should wait until channel 20 scrolled to 142 after setting this value manually", + predicate: ({ scrollTop, threadViewer }) => { + return ( + threadViewer.thread.model === 'mail.channel' && + threadViewer.thread.id === 20 && + scrollTop === 142 + ); + }, + }); // fold chat window await afterNextRender(() => document.querySelector('.o_ChatWindow_header').click()); await this.hideHomeMenu(); // unfold chat window - await afterNextRender(() => document.querySelector('.o_ChatWindow_header').click()); + await this.afterEvent({ + eventName: 'o-component-message-list-scrolled', + func: () => document.querySelector('.o_ChatWindow_header').click(), + message: "should wait until channel 20 restored its scroll to 142 after unfolding it", + predicate: ({ scrollTop, threadViewer }) => { + return ( + threadViewer.thread.model === 'mail.channel' && + threadViewer.thread.id === 20 && + scrollTop === 142 + ); + }, + }); assert.strictEqual( document.querySelector(`.o_ThreadView_messageList`).scrollTop, 142, @@ -1792,7 +1829,18 @@ QUnit.test('[technical] chat window with a thread: keep scroll position in messa // Show home menu await this.showHomeMenu(); // unfold chat window - await afterNextRender(() => document.querySelector('.o_ChatWindow_header').click()); + await this.afterEvent({ + eventName: 'o-component-message-list-scrolled', + func: () => document.querySelector('.o_ChatWindow_header').click(), + message: "should wait until channel 20 restored its scroll position to the last saved value (142)", + predicate: ({ scrollTop, threadViewer }) => { + return ( + threadViewer.thread.model === 'mail.channel' && + threadViewer.thread.id === 20 && + scrollTop === 142 + ); + }, + }); assert.strictEqual( document.querySelector(`.o_ThreadView_messageList`).scrollTop, 142, @@ -1927,8 +1975,8 @@ QUnit.test('new message separator is shown in a chat window of a chat on receivi }); this.data['mail.channel'].records = [ { - id: 10, channel_type: "chat", + id: 10, is_minimized: true, is_pinned: false, members: [this.data.currentPartnerId, 10], @@ -1953,8 +2001,8 @@ QUnit.test('new message separator is shown in a chat window of a chat on receivi context: { mockedUserId: 42, }, - uuid: 'channel-10-uuid', message_content: "hu", + uuid: 'channel-10-uuid', }, })); assert.containsOnce( @@ -1987,28 +2035,28 @@ QUnit.test('focusing a chat window of a chat should make new message separator d name: "Foreigner user", partner_id: 10, }); - this.data['mail.channel'].records = [ + this.data['mail.channel'].records.push( { - id: 10, channel_type: "chat", + id: 10, is_minimized: true, is_pinned: false, members: [this.data.currentPartnerId, 10], message_unread_counter: 0, uuid: 'channel-10-uuid', }, - ]; + ); await this.start(); // simulate receiving a message - await afterNextRender(async () => this.env.services.rpc({ + await afterNextRender(() => this.env.services.rpc({ route: '/mail/chat_post', params: { context: { mockedUserId: 42, }, - uuid: 'channel-10-uuid', message_content: "hu", + uuid: 'channel-10-uuid', }, })); assert.containsOnce( @@ -2017,7 +2065,17 @@ QUnit.test('focusing a chat window of a chat should make new message separator d "should display 'new messages' separator in the conversation, from reception of new messages" ); - await afterNextRender(() => document.querySelector('.o_ComposerTextInput_textarea').focus()); + await afterNextRender(() => this.afterEvent({ + eventName: 'o-thread-last-seen-by-current-partner-message-id-changed', + func: () => document.querySelector('.o_ComposerTextInput_textarea').focus(), + message: "should wait until last seen by current partner message id changed", + predicate: ({ thread }) => { + return ( + thread.id === 10 && + thread.model === 'mail.channel' + ); + }, + })); assert.containsNone( document.body, '.o_MessageList_separatorNewMessages', diff --git a/addons/mail/static/src/components/chatter/chatter.js b/addons/mail/static/src/components/chatter/chatter.js index d4314108b87..8c19c3ace07 100644 --- a/addons/mail/static/src/components/chatter/chatter.js +++ b/addons/mail/static/src/components/chatter/chatter.js @@ -9,6 +9,7 @@ const components = { ThreadView: require('mail/static/src/components/thread_view/thread_view.js'), }; const useStore = require('mail/static/src/component_hooks/use_store/use_store.js'); +const useUpdate = require('mail/static/src/component_hooks/use_update/use_update.js'); const { Component } = owl; const { useRef } = owl.hooks; @@ -37,6 +38,7 @@ class Chatter extends Component { attachments: 1, }, }); + useUpdate({ func: () => this._update() }); /** * Reference of the composer. Useful to focus it. */ @@ -47,14 +49,6 @@ class Chatter extends Component { this._threadRef = useRef('thread'); } - mounted() { - this._update(); - } - - patched() { - this._update(); - } - //-------------------------------------------------------------------------- // Public //-------------------------------------------------------------------------- @@ -84,6 +78,9 @@ class Chatter extends Component { * @private */ _update() { + if (!this.chatter) { + return; + } if (this.chatter.thread) { this._notifyRendered(); } diff --git a/addons/mail/static/src/components/chatter_container/chatter_container.js b/addons/mail/static/src/components/chatter_container/chatter_container.js index 39331939da8..19c847d01d9 100644 --- a/addons/mail/static/src/components/chatter_container/chatter_container.js +++ b/addons/mail/static/src/components/chatter_container/chatter_container.js @@ -5,6 +5,7 @@ const components = { Chatter: require('mail/static/src/components/chatter/chatter.js'), }; const useStore = require('mail/static/src/component_hooks/use_store/use_store.js'); +const useUpdate = require('mail/static/src/component_hooks/use_update/use_update.js'); const { clear } = require('mail/static/src/model/model_field_command.js'); const { Component } = owl; @@ -37,10 +38,7 @@ class ChatterContainer extends Component { } return { chatter: this.chatter }; }); - } - - mounted() { - this._update(); + useUpdate({ func: () => this._update() }); } /** @@ -53,10 +51,6 @@ class ChatterContainer extends Component { return super.willUpdateProps(...arguments); } - patched() { - this._update(); - } - /** * @override */ diff --git a/addons/mail/static/src/components/composer/composer.js b/addons/mail/static/src/components/composer/composer.js index 6c9965a57d1..4434697f8fb 100644 --- a/addons/mail/static/src/components/composer/composer.js +++ b/addons/mail/static/src/components/composer/composer.js @@ -11,6 +11,7 @@ const components = { ThreadTextualTypingStatus: require('mail/static/src/components/thread_textual_typing_status/thread_textual_typing_status.js'), }; const useDragVisibleDropZone = require('mail/static/src/component_hooks/use_drag_visible_dropzone/use_drag_visible_dropzone.js'); +const useUpdate = require('mail/static/src/component_hooks/use_update/use_update.js'); const useStore = require('mail/static/src/component_hooks/use_store/use_store.js'); const { isEventHandled, @@ -38,6 +39,7 @@ class Composer extends Component { : undefined, }; }); + useUpdate({ func: () => this._update() }); /** * Reference of the emoji popover. Useful to include emoji popover as * contained "inside" the composer. @@ -61,11 +63,6 @@ class Composer extends Component { mounted() { document.addEventListener('click', this._onClickCaptureGlobal, true); - this._update(); - } - - patched() { - this._update(); } willUnmount() { @@ -201,6 +198,9 @@ class Composer extends Component { * @private */ _update() { + if (!this.composer) { + return; + } if (this._subjectRef.el) { this._subjectRef.el.value = this.composer.subjectContent; } diff --git a/addons/mail/static/src/components/composer_suggested_recipient/composer_suggested_recipient.js b/addons/mail/static/src/components/composer_suggested_recipient/composer_suggested_recipient.js index cbc215a3b7e..f9b3944454b 100644 --- a/addons/mail/static/src/components/composer_suggested_recipient/composer_suggested_recipient.js +++ b/addons/mail/static/src/components/composer_suggested_recipient/composer_suggested_recipient.js @@ -2,6 +2,7 @@ odoo.define('mail/static/src/components/composer_suggested_recipient/composer_su 'use strict'; const useStore = require('mail/static/src/component_hooks/use_store/use_store.js'); +const useUpdate = require('mail/static/src/component_hooks/use_update/use_update.js'); const { FormViewDialog } = require('web.view_dialogs'); const { ComponentAdapter } = require('web.OwlCompatibility'); @@ -38,6 +39,7 @@ class ComposerSuggestedRecipient extends Component { suggestedRecipientInfo: suggestedRecipientInfo && suggestedRecipientInfo.__state, }; }); + useUpdate({ func: () => this._update() }); /** * Form view dialog class. Useful to reference it in the template. */ @@ -62,14 +64,6 @@ class ComposerSuggestedRecipient extends Component { this._onDialogSaved = this._onDialogSaved.bind(this); } - mounted() { - this._update(); - } - - patched() { - this._update(); - } - //-------------------------------------------------------------------------- // Public //-------------------------------------------------------------------------- diff --git a/addons/mail/static/src/components/composer_suggestion/composer_suggestion.js b/addons/mail/static/src/components/composer_suggestion/composer_suggestion.js index 80043eaf057..308dee76821 100644 --- a/addons/mail/static/src/components/composer_suggestion/composer_suggestion.js +++ b/addons/mail/static/src/components/composer_suggestion/composer_suggestion.js @@ -2,6 +2,7 @@ odoo.define('mail/static/src/components/composer_suggestion/composer_suggestion. 'use strict'; const useStore = require('mail/static/src/component_hooks/use_store/use_store.js'); +const useUpdate = require('mail/static/src/component_hooks/use_update/use_update.js'); const components = { PartnerImStatusIcon: require('mail/static/src/components/partner_im_status_icon/partner_im_status_icon.js'), @@ -24,14 +25,7 @@ class ComposerSuggestion extends Component { record: record ? record.__state : undefined, }; }); - } - - mounted() { - this._update(); - } - - patched() { - this._update(); + useUpdate({ func: () => this._update() }); } //-------------------------------------------------------------------------- diff --git a/addons/mail/static/src/components/composer_text_input/composer_text_input.js b/addons/mail/static/src/components/composer_text_input/composer_text_input.js index c2cc133417a..5afb99c4990 100644 --- a/addons/mail/static/src/components/composer_text_input/composer_text_input.js +++ b/addons/mail/static/src/components/composer_text_input/composer_text_input.js @@ -2,6 +2,7 @@ odoo.define('mail/static/src/components/composer_text_input/composer_text_input. 'use strict'; const useStore = require('mail/static/src/component_hooks/use_store/use_store.js'); +const useUpdate = require('mail/static/src/component_hooks/use_update/use_update.js'); const components = { ComposerSuggestionList: require('mail/static/src/components/composer_suggestion_list/composer_suggestion_list.js'), @@ -28,6 +29,11 @@ class ComposerTextInput extends Component { isDeviceMobile: this.env.messaging.device.isMobile, }; }); + /** + * Updates the composer text input content when composer is mounted + * as textarea content can't be changed from the DOM. + */ + useUpdate({ func: () => this._update() }); /** * Last content of textarea from input event. Useful to determine * whether the current partner is typing something. @@ -39,22 +45,6 @@ class ComposerTextInput extends Component { this._textareaRef = useRef('textarea'); } - /** - * Updates the composer text input content when composer is mounted - * as textarea content can't be changed from the DOM. - */ - mounted() { - this._update(); - } - - /** - * Updates the composer text input content when composer has changed - * as textarea content can't be changed from the DOM. - */ - patched() { - this._update(); - } - //-------------------------------------------------------------------------- // Public //-------------------------------------------------------------------------- @@ -153,6 +143,9 @@ class ComposerTextInput extends Component { * @private */ _update() { + if (!this.composer) { + return; + } this._textareaRef.el.value = this.composer.textInputContent; this._textareaRef.el.setSelectionRange( this.composer.textInputCursorStart, diff --git a/addons/mail/static/src/components/composer_text_input/composer_text_input.xml b/addons/mail/static/src/components/composer_text_input/composer_text_input.xml index dfa7bbd1182..493acbb1b4f 100644 --- a/addons/mail/static/src/components/composer_text_input/composer_text_input.xml +++ b/addons/mail/static/src/components/composer_text_input/composer_text_input.xml @@ -10,7 +10,7 @@ isBelow="props.hasMentionSuggestionsBelowPosition" /> -