From 1096ed7b5a88d041d45e021f80ad56128b850be3 Mon Sep 17 00:00:00 2001 From: xO-Tx Date: Mon, 9 Jan 2023 18:44:17 +0000 Subject: [PATCH] [FIX] web_editor: prevent async code on link tools update Starting from [1], the `_updateColorpicker` method was added on the "link tools" widget to update colorpickers with the selected link colors (and it will create the colorpicker for each CSS property on widget `start`). Following this update, the code on [2] and [3] added some fixes to handle the async parts of `_updateColorpicker`. [3] was done very recently, more than one year later after the introduction of [1]. It turned a synchronous method into an asynchronous one (rightfully considering the async calls to `_updateColorpicker` that were inside). As a fix, this commit will try to re-made it a synchronous method, and other parts related to `_updateColorpicker` too. The goal of this commit is to separate the link tools colorpicker "creation" code (`_addColorPicker` now) from colorpicker's "update" code. These changes are supposed to fix a race condition issue that appeared on 16.0, but since the code is the same on 15.0 and subject to the same issues, we target 15.0 with this fix. Hopefully, this prepares for an easier refactoring in master. [1]: https://github.com/odoo/odoo/commit/26d37812f0217ae913c8ddf761bcb26b5c44bff5 [2]: https://github.com/odoo/odoo/pull/78301/commits/6f1a6b76642db2e4f81cf3474ad0ec745a4bb3c9 [3]: https://github.com/odoo/odoo/commit/d15fdca644ef9ed1dfb011d3790f403e0157f55b runbot-5753 closes odoo/odoo#113510 X-original-commit: 2d4a3efe0d7c9618c388575b4bb652a26913fe68 Signed-off-by: Quentin Smetz (qsm) Co-authored-by: qsm-odoo --- .../static/src/js/editor/snippets.editor.js | 32 +++- .../static/src/js/wysiwyg/widgets/link.js | 18 ++- .../src/js/wysiwyg/widgets/link_tools.js | 139 ++++++++++-------- 3 files changed, 125 insertions(+), 64 deletions(-) diff --git a/addons/web_editor/static/src/js/editor/snippets.editor.js b/addons/web_editor/static/src/js/editor/snippets.editor.js index dacf1c7e0c2..669f1e35b1c 100644 --- a/addons/web_editor/static/src/js/editor/snippets.editor.js +++ b/addons/web_editor/static/src/js/editor/snippets.editor.js @@ -2054,6 +2054,15 @@ var SnippetsMenu = Widget.extend({ * - Remove the 'contentEditable' attributes */ cleanForSave: async function () { + // Wait for snippet post-drop code here, since sometimes we save very + // quickly after a snippet drop during automated testing, which breaks + // some options code (executed while destroying the editor). + // TODO we should find a better way, by better locking the drag and drop + // code inside the edition mutex... which unfortunately cannot be done + // given the state of the code, as internal operations of that drag and + // drop code need to use the mutex themselves. + await this.postSnippetDropPromise; + // First disable the snippet selection, calling options onBlur, closing // widgets, etc. Then wait for full resolution of the mutex as widgets // may have triggered some final edition requests that need to be @@ -2157,6 +2166,10 @@ var SnippetsMenu = Widget.extend({ * @returns {Promise} */ callPostSnippetDrop: async function ($target) { + this.postSnippetDropPromise = new Promise(resolve => { + this._postSnippetDropResolver = resolve; + }); + // First call the onBuilt of all options of each item in the snippet // (and so build their editor instance first). await this._callForEachChildSnippet($target, function (editor, $snippet) { @@ -2180,6 +2193,8 @@ var SnippetsMenu = Widget.extend({ // Lastly, ensure that the snippets or its related parts are added to // the invisible DOM list if needed. await this._updateInvisibleDOM(); + + this._postSnippetDropResolver(); }, /** * Public implementation of _execWithLoadingEffect. @@ -2532,13 +2547,20 @@ var SnippetsMenu = Widget.extend({ } return editorToEnable; - }).then(editor => { + }).then(async editor => { // If a link was clicked, the linktools should be focused after // the right panel is shown to the user. - if (this._currentTab === this.tabs.OPTIONS - && this.options.wysiwyg.linkTools - && !this.options.wysiwyg.linkTools.noFocusUrl) { - this.options.wysiwyg.linkTools.focusUrl(); + // TODO: this should be reviewed to be done another way: we + // should avoid focusing something here while it is being + // rendered elsewhere. + const linkTools = this.options.wysiwyg.linkTools; + if (linkTools && this._currentTab === this.tabs.OPTIONS + && !linkTools.noFocusUrl) { + // Wait for `linkTools` potential in-progress rendering + // before focusing the URL input on `snippetsMenu` (this + // prevents race condition for automated testing). + await linkTools.renderingPromise; + linkTools.focusUrl(); } return editor; }); diff --git a/addons/web_editor/static/src/js/wysiwyg/widgets/link.js b/addons/web_editor/static/src/js/wysiwyg/widgets/link.js index 87381b57e09..cea6a5143db 100644 --- a/addons/web_editor/static/src/js/wysiwyg/widgets/link.js +++ b/addons/web_editor/static/src/js/wysiwyg/widgets/link.js @@ -125,6 +125,8 @@ const Link = Widget.extend({ if (/(?:s_website_form_send|o_submit)/.test(this.data.className)) { this.isButton = true; } + + this.renderingPromise = new Promise(resolve => this._renderingResolver = resolve); }, /** * @override @@ -152,7 +154,7 @@ const Link = Widget.extend({ const _super = this._super.bind(this); - await this._updateOptionsUI(); + this._updateOptionsUI(); if (this.data.url) { var match = /mailto:(.+)/.exec(this.data.url); @@ -167,6 +169,20 @@ const Link = Widget.extend({ return _super(...arguments); }, + /** + * @private + */ + async _widgetRenderAndInsert() { + const res = await this._super(...arguments); + + // TODO find a better solution than this during the upcoming refactoring + // of the link tools / link dialog. + if (this._renderingResolver) { + this._renderingResolver(); + } + + return res; + }, /** * @override */ diff --git a/addons/web_editor/static/src/js/wysiwyg/widgets/link_tools.js b/addons/web_editor/static/src/js/wysiwyg/widgets/link_tools.js index 0c421861a27..7a3c28d602c 100644 --- a/addons/web_editor/static/src/js/wysiwyg/widgets/link_tools.js +++ b/addons/web_editor/static/src/js/wysiwyg/widgets/link_tools.js @@ -31,7 +31,8 @@ const LinkTools = Link.extend({ */ init: function (parent, options, editable, data, $button, link) { this._link = link; - this._observer = new MutationObserver(() =>{ + this._observer = new MutationObserver(async () => { + await this.renderingPromise; this._updateLabelInput(); }); this._observer.observe(this._link, {subtree: true, childList: true, characterData: true}); @@ -39,7 +40,19 @@ const LinkTools = Link.extend({ // Keep track of each selected custom color and colorpicker. this.customColors = {}; this.colorpickers = {}; - this.colorpickersPromises = {}; + this.COLORPICKER_CSS_PROPERTIES = ['color', 'background-color', 'border-color']; + this.PREFIXES = { + 'color': 'text-', + 'background-color': 'bg-', + }; + }, + /** + * @override + */ + willStart: async function () { + const _super = this._super.bind(this); + await Promise.all(this.COLORPICKER_CSS_PROPERTIES.map(cssProperty => this._addColorPicker(cssProperty))); + return _super(...arguments); }, /** * @override @@ -48,12 +61,24 @@ const LinkTools = Link.extend({ this._addHintClasses(); const ret = await this._super(...arguments); const link = this.$link[0]; + const colorpickerLocations = { + 'color': '.link-custom-color-text .dropdown-menu', + 'background-color': '.link-custom-color-fill .dropdown-menu', + 'border-color': '.link-custom-color-border .o_we_so_color_palette .dropdown-menu', + }; + for (const cssProperty of this.COLORPICKER_CSS_PROPERTIES) { + // Colorpickers were created into fragments before any UI or event + // handler of this main widget was built. This just moves those + // colorpickers at their rightful position, synchronously. + const locationEl = this.el.querySelector(colorpickerLocations[cssProperty]); + this.colorpickers[cssProperty].$el.appendTo(locationEl); + } const customStyleProps = ['color', 'background-color', 'background-image', 'border-width', 'border-style', 'border-color']; if (customStyleProps.some(s => link.style[s])) { // Force custom style if style exists on the link. const customOption = this.el.querySelector('[name="link_style_color"] we-button[data-value="custom"]'); this._setSelectOption($(customOption), true); - await this._updateOptionsUI(); + this._updateOptionsUI(); } return ret; }, @@ -226,7 +251,7 @@ const LinkTools = Link.extend({ /** * @override */ - _updateOptionsUI: async function () { + _updateOptionsUI: function () { const el = this.el.querySelector('[name="link_style_color"] we-button.active'); if (el) { this.colorCombinationClass = el.dataset.value; @@ -235,9 +260,12 @@ const LinkTools = Link.extend({ // Show custom colors only for Custom style. this.$('.link-custom-color').toggleClass('d-none', el.dataset.value !== 'custom'); - await this._updateColorpicker('color'); - await this._updateColorpicker('background-color'); - await this._updateColorpicker('border-color'); + // Note: the _updateColorpicker method is supposedly async but can + // be used synchronously given the fact that _addColorPicker was + // called during this widget initialization. + this._updateColorpicker('color'); + this._updateColorpicker('background-color'); + this._updateColorpicker('border-color'); const borderWidth = this.linkEl.style['border-width']; const numberAndUnit = getNumericAndUnit(borderWidth); @@ -261,58 +289,8 @@ const LinkTools = Link.extend({ * @param {string} cssProperty */ _updateColorpicker: async function (cssProperty) { - const prefix = { - 'color': 'text-', - 'background-color': 'bg-', - }[cssProperty]; - + const prefix = this.PREFIXES[cssProperty]; let colorpicker = this.colorpickers[cssProperty]; - await this.colorpickersPromises[cssProperty]; - if (!colorpicker) { - colorpicker = new ColorPaletteWidget(this, { - excluded: ['transparent_grayscale'], - $editable: $(this.options.wysiwyg.odooEditor.editable), - withGradients: cssProperty === 'background-color', - }); - this.colorpickers[cssProperty] = colorpicker; - const target = this.el.querySelector({ - 'color': '.link-custom-color-text .dropdown-menu', - 'background-color': '.link-custom-color-fill .dropdown-menu', - 'border-color': '.link-custom-color-border .o_we_so_color_palette .dropdown-menu', - }[cssProperty]); - this.colorpickersPromises[cssProperty] = colorpicker.appendTo($(target)); - await this.colorpickersPromises[cssProperty]; - colorpicker.on('custom_color_picked color_picked color_hover color_leave', this, (ev) => { - // Reset color styles in link content to make sure new color is not hidden. - // Only done when applied to avoid losing state during preview. - if (['custom_color_picked', 'color_picked'].includes(ev.name)) { - const selection = window.getSelection(); - const range = document.createRange(); - range.selectNodeContents(this.linkEl); - selection.removeAllRanges(); - selection.addRange(range); - this.options.wysiwyg.odooEditor.execCommand('applyColor', '', 'color'); - this.options.wysiwyg.odooEditor.execCommand('applyColor', '', 'backgroundColor'); - } - - let color = ev.data.color; - const colorNames = colorpicker.getColorNames(); - const colorClasses = prefix ? computeColorClasses(colorNames, prefix) : []; - const colorClass = `${prefix}${color}`; - if (colorClasses.includes(colorClass)) { - color = colorClass; - } else if (colorNames.includes(color)) { - // Store as color value. - color = getCSSVariableValue(color); - } - this.customColors[cssProperty] = color; - this.applyLinkToDom(this._getData()); - if (['custom_color_picked', 'color_picked'].includes(ev.name)) { - this.options.wysiwyg.odooEditor.historyStep(); - this._updateOptionsUI(); - } - }); - } // Update selected color. const colorNames = colorpicker.getColorNames(); @@ -349,6 +327,51 @@ const LinkTools = Link.extend({ $colorPreview.css('background-image', isColorGradient(color) ? color : ''); } }, + /** + * @private + * @param {string} cssProperty + */ + async _addColorPicker(cssProperty) { + const prefix = this.PREFIXES[cssProperty]; + const colorpicker = new ColorPaletteWidget(this, { + excluded: ['transparent_grayscale'], + $editable: $(this.options.wysiwyg.odooEditor.editable), + withGradients: cssProperty === 'background-color', + }); + this.colorpickers[cssProperty] = colorpicker; + await colorpicker.appendTo(document.createDocumentFragment()); + colorpicker.on('custom_color_picked color_picked color_hover color_leave', this, (ev) => { + // Reset color styles in link content to make sure new color is not hidden. + // Only done when applied to avoid losing state during preview. + if (['custom_color_picked', 'color_picked'].includes(ev.name)) { + const selection = window.getSelection(); + const range = document.createRange(); + range.selectNodeContents(this.linkEl); + selection.removeAllRanges(); + selection.addRange(range); + this.options.wysiwyg.odooEditor.execCommand('applyColor', '', 'color'); + this.options.wysiwyg.odooEditor.execCommand('applyColor', '', 'backgroundColor'); + } + + let color = ev.data.color; + const colorNames = colorpicker.getColorNames(); + const colorClasses = prefix ? computeColorClasses(colorNames, prefix) : []; + const colorClass = `${prefix}${color}`; + if (colorClasses.includes(colorClass)) { + color = colorClass; + } else if (colorNames.includes(color)) { + // Store as color value. + color = getCSSVariableValue(color); + } + this.customColors[cssProperty] = color; + this.applyLinkToDom(this._getData()); + if (['custom_color_picked', 'color_picked'].includes(ev.name)) { + this.options.wysiwyg.odooEditor.historyStep(); + this._updateOptionsUI(); + } + }); + return colorpicker; + }, /** * Add hint to the classes of the link and button. */