From 59813f0cff10f3ea8f8ce4e0b8723729de45ecd7 Mon Sep 17 00:00:00 2001 From: "Louis (loco)" Date: Mon, 15 Jan 2024 11:28:30 +0000 Subject: [PATCH] [FIX] website: destroy widgets before instantiating the wysiwyg Steps to reproduce the problem: - Add a "Countdown" snippet on the website. - Save. - Enter in edit mode again. -> Problem: the `destroy()` of the `CountdownWidget` public widget comes from a `widgets_start_request` that is triggered when the `WysiwygAdapterComponent` has been mounted and patched (see [useEffect documentation]). The problem is that, as the widgets are only destroyed at this moment, it is possible that a widget could replace an element that has been modified during the instantiation of the `WysiwygAdapterComponent`. For example: - At the `onWillStart()` hook of the `WysiwygAdapterComponent`, the `o_editable` is added on some elements. - A widget could then replace one of these elements. - The widget is destroyed after the rendering of the `WysiwygAdapterComponent`. -> The `o_editable` class is not on the element anymore. The goal of this commit is to, as before [1], destroy the existing widgets before instantiating the `wysiwyg`. This commit also adds a test to ensure that the order of the calls to the widgets and wysiwyg lifecycle methods is coherent. Related to runbot-28700 [1]: https://github.com/odoo/odoo/commit/a3e34512bf229d5d55f9e9d9eb0a9f7211a5a826 [useEffect documentation]: https://github.com/odoo/owl/blob/master/doc/reference/hooks.md#useeffect closes odoo/odoo#151411 X-original-commit: 723c0dfeb7d39ca2522e65cf021664aacfc7099c Signed-off-by: Quentin Smetz (qsm) --- addons/website/__manifest__.py | 4 +- .../wysiwyg_adapter/wysiwyg_adapter.js | 10 +++ .../tour_utils/focus_blur_snippets_options.js | 2 +- .../tour_utils/widget_lifecycle_dep_widget.js | 42 +++++++++++ .../widget_lifecycle_patch_wysiwyg.js | 62 +++++++++++++++++ .../static/tests/tours/widget_lifecycle.js | 69 +++++++++++++++++++ addons/website/tests/test_ui.py | 8 +++ 7 files changed, 195 insertions(+), 2 deletions(-) create mode 100644 addons/website/static/tests/tour_utils/widget_lifecycle_dep_widget.js create mode 100644 addons/website/static/tests/tour_utils/widget_lifecycle_patch_wysiwyg.js create mode 100644 addons/website/static/tests/tours/widget_lifecycle.js diff --git a/addons/website/__manifest__.py b/addons/website/__manifest__.py index a89e30a5df8..1aacd6215a2 100644 --- a/addons/website/__manifest__.py +++ b/addons/website/__manifest__.py @@ -174,7 +174,9 @@ ('prepend', 'website/static/src/scss/secondary_variables.scss'), ], 'web.assets_tests': [ - 'website/static/tests/tour_utils/**/*', + 'website/static/tests/tour_utils/focus_blur_snippets_options.js', + 'website/static/tests/tour_utils/website_preview_test.js', + 'website/static/tests/tour_utils/widget_lifecycle_dep_widget.js', 'website/static/tests/tours/**/*', ], 'web.assets_backend': [ diff --git a/addons/website/static/src/components/wysiwyg_adapter/wysiwyg_adapter.js b/addons/website/static/src/components/wysiwyg_adapter/wysiwyg_adapter.js index c27942cc1c4..66eb656ca2f 100644 --- a/addons/website/static/src/components/wysiwyg_adapter/wysiwyg_adapter.js +++ b/addons/website/static/src/components/wysiwyg_adapter/wysiwyg_adapter.js @@ -92,6 +92,15 @@ export class WysiwygAdapterComponent extends Wysiwyg { useHotkey('control+k', () => {}); onWillStart(() => { + // Destroy the widgets before instantiating the wysiwyg. + // grep: RESTART_WIDGETS_EDIT_MODE + // TODO ideally this should be done as close as the restart as + // as possible to avoid long flickering when entering edit mode. At + // moment some RPC are awaited before the restart so it is not + // ideal. But this has to be done before adding o_editable classes + // in the DOM. To review once everything is OWLified. + this._websiteRootEvent("widgets_stop_request"); + const pageOptionEls = this.websiteService.pageDocument.querySelectorAll('.o_page_option_data'); for (const pageOptionEl of pageOptionEls) { const optionName = pageOptionEl.name; @@ -199,6 +208,7 @@ export class WysiwygAdapterComponent extends Wysiwyg { } // The jquery instance inside the iframe needs to be aware of the wysiwyg. this.websiteService.contentWindow.$('#wrapwrap').data('wysiwyg', this); + // grep: RESTART_WIDGETS_EDIT_MODE await new Promise((resolve, reject) => this._websiteRootEvent('widgets_start_request', { editableMode: true, onSuccess: resolve, diff --git a/addons/website/static/tests/tour_utils/focus_blur_snippets_options.js b/addons/website/static/tests/tour_utils/focus_blur_snippets_options.js index 002ce91e460..578182e4121 100644 --- a/addons/website/static/tests/tour_utils/focus_blur_snippets_options.js +++ b/addons/website/static/tests/tour_utils/focus_blur_snippets_options.js @@ -1,7 +1,7 @@ /** @odoo-module **/ odoo.loader.bus.addEventListener("module-started", (e) => { - if (e.detail.moduleName === "@web_editor/js/editor/snippets.options"){ + if (e.detail.moduleName === "@web_editor/js/editor/snippets.options") { const options = e.detail.module[Symbol.for("default")]; const FocusBlur = options.Class.extend({ onFocus() { diff --git a/addons/website/static/tests/tour_utils/widget_lifecycle_dep_widget.js b/addons/website/static/tests/tour_utils/widget_lifecycle_dep_widget.js new file mode 100644 index 00000000000..f318b8ec1a2 --- /dev/null +++ b/addons/website/static/tests/tour_utils/widget_lifecycle_dep_widget.js @@ -0,0 +1,42 @@ +/** @odoo-module **/ + +odoo.loader.bus.addEventListener("module-started", (e) => { + if (e.detail.moduleName !== "@web/legacy/js/public/public_widget") { + return; + } + + const publicWidget = e.detail.module[Symbol.for("default")]; + + const localStorageKey = 'widgetAndWysiwygLifecycle'; + if (!window.localStorage.getItem(localStorageKey)) { + window.localStorage.setItem(localStorageKey, '[]'); + } + + function addLifecycleStep(step) { + const oldValue = window.localStorage.getItem(localStorageKey); + const newValue = JSON.stringify(JSON.parse(oldValue).concat(step)); + window.localStorage.setItem(localStorageKey, newValue); + } + + publicWidget.registry.CountdownPatch = publicWidget.Widget.extend({ + selector: ".s_countdown", + disabledInEditableMode: false, + + /** + * @override + */ + async start() { + addLifecycleStep('widgetStart'); + await this._super(...arguments); + this.el.classList.add("public_widget_started"); + }, + /** + * @override + */ + destroy() { + this.el.classList.remove("public_widget_started"); + addLifecycleStep('widgetStop'); + this._super(...arguments); + }, + }); +}); diff --git a/addons/website/static/tests/tour_utils/widget_lifecycle_patch_wysiwyg.js b/addons/website/static/tests/tour_utils/widget_lifecycle_patch_wysiwyg.js new file mode 100644 index 00000000000..1bb7afc1413 --- /dev/null +++ b/addons/website/static/tests/tour_utils/widget_lifecycle_patch_wysiwyg.js @@ -0,0 +1,62 @@ +/** @odoo-module **/ + +import { onMounted, onWillRender } from "@odoo/owl"; +import { patch } from "@web/core/utils/patch"; + +odoo.loader.bus.addEventListener("module-started", (e) => { + if (e.detail.moduleName !== "@website/components/wysiwyg_adapter/wysiwyg_adapter") { + return; + } + + const { WysiwygAdapterComponent } = e.detail.module; + + // Duplicated from "@website/../tests/tour_utils/widget_lifecycle_dep_widget" + // Cannot be imported for some reason, probably because of this being lazy + // loaded? + function addLifecycleStep(step) { + const localStorageKey = 'widgetAndWysiwygLifecycle'; + const oldValue = window.localStorage.getItem(localStorageKey); + const newValue = JSON.stringify(JSON.parse(oldValue).concat(step)); + window.localStorage.setItem(localStorageKey, newValue); + } + + patch(WysiwygAdapterComponent.prototype, { + /** + * @override + */ + setup() { + super.setup(...arguments); + + // The Wysiwyg class is very messy at the moment: it touches the DOM in + // onWillStart hook, mixes OWL & legacy widget, etc. Here we want to + // test "when the Wysiwyg is started"... for now we will settle on + // testing "the first time it touches the DOM", relying on it to be when + // he reads what "editable elements" are for the first time, thanks to + // the `editableElements` method. + // TODO to be reviewed (probably as soon as we touch the editable DOM + // only when considered initialized). + onWillRender(() => { + this.__consideredInitialized = true; + }); + onMounted(() => { + addLifecycleStep('wysiwygStarted'); + }); + }, + /** + * @override + */ + editableElements() { + if (!this.__consideredInitialized) { + addLifecycleStep('wysiwygStart'); + } + return super.editableElements(...arguments); + }, + /** + * @override + */ + destroy() { + addLifecycleStep('wysiwygStop'); + super.destroy(...arguments); + }, + }); +}); diff --git a/addons/website/static/tests/tours/widget_lifecycle.js b/addons/website/static/tests/tours/widget_lifecycle.js new file mode 100644 index 00000000000..e7207078c0a --- /dev/null +++ b/addons/website/static/tests/tours/widget_lifecycle.js @@ -0,0 +1,69 @@ +/** @odoo-module **/ + +import wTourUtils from '@website/js/tours/tour_utils'; + +// Note: cannot import @website/../tests/tour_utils/widget_lifecycle_dep_widget +// here because that module requires web.public.widget which is not available +// in the backend, where this tour definition is loaded. Easier to duplicate +// that key for now rather than create a whole file to handle this localStorage +// key only. +const localStorageKey = 'widgetAndWysiwygLifecycle'; + +wTourUtils.registerWebsitePreviewTour("widget_lifecycle", { + test: true, + url: "/", + edition: true, +}, () => [ + wTourUtils.dragNDrop({ + id: "s_countdown", + name: "Countdown", + }), + { + content: "Wait for the widget to be started and empty the widgetAndWysiwygLifecycle list", + trigger: "iframe .s_countdown.public_widget_started", + run: () => { + // Start recording the calls to the "start" and "destroy" method of + // the widget and the wysiwyg. + window.localStorage.setItem(localStorageKey, '[]'); + }, + }, + ...wTourUtils.clickOnSave(), + { + content: "Wait for the widget to be started", + trigger: "iframe .s_countdown.public_widget_started", + run: () => {}, // It's a check + }, + ...wTourUtils.clickOnEditAndWaitEditMode(), + { + content: "Wait for the widget to be started and check the order of the lifecycle method call of the widget and the wysiwyg", + trigger: "iframe .s_countdown.public_widget_started", + run: () => { + const result = JSON.parse(window.localStorage.widgetAndWysiwygLifecycle); + const expected = ["widgetStop", "wysiwygStop", "widgetStart", + "widgetStop", "wysiwygStart", "wysiwygStarted", "widgetStart", + ]; + const alternative = ["widgetStop", "widgetStart", "wysiwygStop", + "widgetStop", "wysiwygStart", "wysiwygStarted", "widgetStart", + ]; + const resultIsEqualTo = (arr) => { + return arr.length === result.length + && arr.every((item, i) => item === result[i]); + }; + if (!(resultIsEqualTo(expected) || resultIsEqualTo(alternative))) { + // The "destroy" method of the wysiwyg is called two times when + // leaving the edit mode: the first one comes explicitly from + // the "leaveEditMode" of the "wysiwyg_adapter". The second + // comes from the OWL mechanism as the wysiwyg is not present in + // the DOM when the page is reloaded. Because it is not + // guaranteed that this last call happens before the start of + // the widget at the page reload, two sequences are acceptable + // as a result. + console.error(` + Expected: ${expected.toString()} + Or: ${alternative.toString()} + Result: ${result.toString()} + `); + } + }, + }, +]); diff --git a/addons/website/tests/test_ui.py b/addons/website/tests/test_ui.py index 0ed5d2ea741..08ef385bb60 100644 --- a/addons/website/tests/test_ui.py +++ b/addons/website/tests/test_ui.py @@ -504,3 +504,11 @@ class TestUi(odoo.tests.HttpCase): website.menu_id.child_id[1:].unlink() self.start_tour('/', 'website_no_dirty_page', login='admin') + + def test_widget_lifecycle(self): + self.env['ir.asset'].create({ + 'name': 'wysiwyg_patch_start_and_destroy', + 'bundle': 'website.assets_wysiwyg', + 'path': 'website/static/tests/tour_utils/widget_lifecycle_patch_wysiwyg.js', + }) + self.start_tour(self.env['website'].get_client_action_url('/'), 'widget_lifecycle', login='admin')