From 7880c1599bd2ec1b7d836a623e986a849fb27ad4 Mon Sep 17 00:00:00 2001 From: Stanislas Gueniffey Date: Sun, 9 Apr 2023 11:47:47 +0000 Subject: [PATCH] [FIX] web_editor, *: add system for editable medias in non editable envs *: website Some specific media items are meant to be editable even though located in non-editable environments. The typical case in 15.0 is the website "Team" snippet: it is made of multiple bootstrap rows, each containing a column with an image and a column with texts. The columns with the image are only meant to hold that image, it was therefore marked as non editable to prevent users adding text in there by mistake... but the image is still meant to be editable. See [1], later fixed by [2]. The problem now is that the system that [2] had to use is quite messy: the column is marked non editable thanks to the `o_not_editable` class but the inner images were to be declared editable via some custom JS method overrides (`_getContentEditableAreas`). The debate about if we should keep a class-based system and/or a method-override system will be left to master. This commit although comes with an uniformisation about this, introducing a new class to declare "an editable media despite its non editable environment". This is not a counter-part of the class `o_not_editable`: we do not want the media to be marked with the `contenteditable` attribute. Indeed, this is actually required since [2] was not enough to solve the problem. See those steps: - Add a company snippet in your page - Change one of the images by an icon - (Save / Reenter edit mode) - Try to change the icon again / edit its options => You can't. Indeed [2] relied on the media being an image to re-enable edition on it... but even if it did not, it would not have been enough. Indeed, icons are forced to being `contenteditable="false"` by the new editor library (since 15.0 then), last update on that at [3]. We thus needed a different way to differentiate editable media, hence the introduction of the new `o_editable_media` class. Note: this system is not perfect, but so is not the whole system about determining what is editable or not at the moment. In this case, it may exist cases of a media marked with the class to be editable but end up in an environment which is not editable by force (xpath somewhere etc) ... and ends up being editable anyway. Most cases work though and at worse it will be about a non editable image being editable but not possible to save in very rare cases. As advertised, the whole system about what is editable or not should be improved. [1]: https://github.com/odoo/odoo/commit/30db617bc8ff7727f40d7ef58c6578e84a13f284 [2]: https://github.com/odoo/odoo/commit/61270ee8bffb6e85f8ff0d19c7a3889fdce2f486 [3]: https://github.com/odoo/odoo/commit/7646429e894f28f398b7b212e893822c06c7b03d task-3226172 closes odoo/odoo#131139 X-original-commit: 436265e815684046a37a8b80e4cd520c2bd637f0 Signed-off-by: Quentin Smetz (qsm) Co-authored-by: qsm-odoo --- .../web_editor/static/src/js/common/utils.js | 27 +++++++++++++++++++ .../js/editor/odoo-editor/src/OdooEditor.js | 8 ++++-- .../static/src/js/editor/snippets.editor.js | 18 +++++++++---- .../static/src/js/wysiwyg/wysiwyg.js | 16 ++++++++++- .../wysiwyg_adapter/wysiwyg_adapter.js | 14 ++++++---- .../website/views/snippets/s_company_team.xml | 15 +++-------- 6 files changed, 74 insertions(+), 24 deletions(-) diff --git a/addons/web_editor/static/src/js/common/utils.js b/addons/web_editor/static/src/js/common/utils.js index 53707e352ac..c96c832fcc5 100644 --- a/addons/web_editor/static/src/js/common/utils.js +++ b/addons/web_editor/static/src/js/common/utils.js @@ -435,6 +435,32 @@ function _addBackgroundImageAttributes(...newAttributes) { function _isBackgroundImageAttribute(attribute) { return BACKGROUND_IMAGE_ATTRIBUTES.has(attribute); } +/** + * Checks if an element supposedly marked with the o_editable_media class should + * in fact be editable (checks if its environment looks like a non editable + * environment whose media should be editable). + * + * TODO: the name of this function is voluntarily bad to reflect the fact that + * this system should be improved. The combination of o_not_editable, + * o_editable, getContentEditableAreas, getReadOnlyAreas and other concepts + * related to what should be editable or not should be reviewed. + * + * @returns {boolean} + */ +function _shouldEditableMediaBeEditable(mediaEl) { + // Some sections of the DOM are contenteditable="false" (for + // example with the help of the o_not_editable class) but have + // inner media that should be editable (the fact the container + // is not is to prevent adding text in between those medias). + // This case is complex and the solution to support it is not + // perfect: we mark those media with a class and check that the + // first non editable ancestor is in fact in an editable parent. + const parentEl = mediaEl.parentElement; + const nonEditableAncestorRootEl = parentEl && parentEl.closest('[contenteditable="false"]'); + return nonEditableAncestorRootEl + && nonEditableAncestorRootEl.parentElement + && nonEditableAncestorRootEl.parentElement.isContentEditable; +} export default { COLOR_PALETTE_COMPATIBILITY_COLOR_NAMES: COLOR_PALETTE_COMPATIBILITY_COLOR_NAMES, @@ -460,4 +486,5 @@ export default { setEditableWindow: _setEditableWindow, addBackgroundImageAttributes: _addBackgroundImageAttributes, isBackgroundImageAttribute: _isBackgroundImageAttribute, + shouldEditableMediaBeEditable: _shouldEditableMediaBeEditable, }; diff --git a/addons/web_editor/static/src/js/editor/odoo-editor/src/OdooEditor.js b/addons/web_editor/static/src/js/editor/odoo-editor/src/OdooEditor.js index f688230294f..6661daab2a3 100644 --- a/addons/web_editor/static/src/js/editor/odoo-editor/src/OdooEditor.js +++ b/addons/web_editor/static/src/js/editor/odoo-editor/src/OdooEditor.js @@ -2388,10 +2388,14 @@ export class OdooEditor extends EventTarget { this.observerUnactive('_activateContenteditable'); this.editable.setAttribute('contenteditable', this.options.isRootEditable); - const editableAreas = this.options.getContentEditableAreas(this).filter(node => !isArtificialVoidElement(node)); + const editableAreas = this.options.getContentEditableAreas(this); for (const node of editableAreas) { if (!node.isContentEditable) { - node.setAttribute('contenteditable', true); + if (isArtificialVoidElement(node) || node.nodeName === 'IMG') { + node.classList.add('o_editable_media'); + } else { + node.setAttribute('contenteditable', true); + } } } for (const node of this.options.getReadOnlyAreas()) { 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 8d6a3857218..6a4710a61b5 100644 --- a/addons/web_editor/static/src/js/editor/snippets.editor.js +++ b/addons/web_editor/static/src/js/editor/snippets.editor.js @@ -2769,10 +2769,12 @@ var SnippetsMenu = Widget.extend({ _computeSelectorFunctions: function (selector, exclude, target, noCheck, isChildren, excludeParent) { var self = this; - // TODO the `:not([contenteditable="true"])` part is designed to make - // images with such attribute editable even when they are in an - // environment where editing is not normally possible. This should be - // reviewed if we are to handle more hierarchy of editable nodes being + // The `:not(.o_editable_media)` part is handled outside of the selector + // (see filterFunc). + // Note: the `:not([contenteditable="true"])` part was there for that + // same purpose before the implementation of the o_editable_media class. + // It still make sense for potential editable areas though. Although it + // should be reviewed if we are to handle more hierarchy of nodes being // editable despite their non editable environment. // Without the `:not(.s_social_media)`, it is no longer possible to edit // icons in the social media snippet. This should be fixed in a more @@ -2780,7 +2782,13 @@ var SnippetsMenu = Widget.extend({ exclude += `${exclude && ', '}.o_snippet_not_selectable, .o_not_editable:not(.s_social_media) :not([contenteditable="true"])`; let filterFunc = function () { - return !$(this).is(exclude); + if (!$(this).is(exclude)) { + return true; + } + if (this.classList.contains('o_editable_media')) { + return weUtils.shouldEditableMediaBeEditable(this); + } + return false; }; if (target) { const oldFilter = filterFunc; diff --git a/addons/web_editor/static/src/js/wysiwyg/wysiwyg.js b/addons/web_editor/static/src/js/wysiwyg/wysiwyg.js index 317e2cf98d3..348f002d339 100644 --- a/addons/web_editor/static/src/js/wysiwyg/wysiwyg.js +++ b/addons/web_editor/static/src/js/wysiwyg/wysiwyg.js @@ -487,7 +487,21 @@ export class Wysiwyg extends Component { this.showTooltip = true; this.$editable.on('dblclick', mediaSelector, ev => { const targetEl = ev.currentTarget; - if (targetEl.isContentEditable || (targetEl.parentElement && targetEl.parentElement.isContentEditable)) { + let isEditable = + // TODO that first check is probably useless/wrong: checking if + // the media itself has editable content should not be relevant. + // In fact the content of all media should be marked as non + // editable anyway. + targetEl.isContentEditable || + // For a media to be editable, the base case is to be in a + // container whose content is editable. + (targetEl.parentElement && targetEl.parentElement.isContentEditable); + + if (!isEditable && targetEl.classList.contains('o_editable_media')) { + isEditable = weUtils.shouldEditableMediaBeEditable(targetEl); + } + + if (isEditable) { this.showTooltip = false; if (!isProtected(this.odooEditor.document.getSelection().anchorNode)) { 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 b69782631fe..1378ebc0f5d 100644 --- a/addons/website/static/src/components/wysiwyg_adapter/wysiwyg_adapter.js +++ b/addons/website/static/src/components/wysiwyg_adapter/wysiwyg_adapter.js @@ -7,6 +7,7 @@ import { useHotkey } from '@web/core/hotkeys/hotkey_hook'; import { Wysiwyg } from "@web_editor/js/wysiwyg/wysiwyg"; import { requireWysiwygLegacyModule } from "@web_editor/js/frontend/loader"; import weUtils from '@web_editor/js/common/utils'; +import { isMediaElement } from '@web_editor/js/editor/odoo-editor/src/utils/utils'; import { EditMenuDialog, MenuDialog } from "../dialog/edit_menu"; import { WebsiteDialog } from '../dialog/dialog'; @@ -474,12 +475,15 @@ export class WysiwygAdapterComponent extends Wysiwyg { return !$(el).closest('.o_not_editable').length; }); - // TODO review in master. This stable fix restores the possibility to + // TODO migrate in master. This stable fix restores the possibility to // edit the company team snippet images on subsequent editions. Indeed - // this badly relies on the contenteditable="true" attribute being on - // those images but it is rightfully lost after the first save. - // grep: COMPANY_TEAM_CONTENTEDITABLE - let $extraEditableZones = $editableSavableZones.find('.s_company_team .o_not_editable img'); + // this badly relied on the contenteditable="true" attribute being on + // those images but it is rightfully lost after the first save. Later, + // the o_editable_media class system was implemented and the class was + // added in the snippet template but this did not solve existing + // snippets in user databases. + let $extraEditableZones = $editableSavableZones.find('.s_company_team .o_not_editable *') + .filter((i, el) => isMediaElement(el) || el.tagName === 'IMG'); // To make sure the selection remains bounded to the active tab, // each tab is made non editable while keeping its nested diff --git a/addons/website/views/snippets/s_company_team.xml b/addons/website/views/snippets/s_company_team.xml index 2d4ff7c8fb0..924f62dd3a6 100644 --- a/addons/website/views/snippets/s_company_team.xml +++ b/addons/website/views/snippets/s_company_team.xml @@ -1,13 +1,6 @@ -