[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) <qsm@odoo.com> Co-authored-by: qsm-odoo <qsm@odoo.com>
This commit is contained in:
Stanislas Gueniffey
committed by
qsm-odoo
qsm-odoo
co-authored by
qsm-odoo
qsm-odoo
parent
9f183ea0e9
commit
7880c1599b
@@ -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,
|
||||
};
|
||||
|
||||
@@ -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()) {
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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)) {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -1,13 +1,6 @@
|
||||
<?xml version="1.0" encoding="utf-8"?>
|
||||
<odoo>
|
||||
|
||||
<!--
|
||||
TODO review in master: this snippet's images use contenteditable="true" to be
|
||||
editable, their parent being o_not_editable/contenteditable="false" to prevent
|
||||
adding text by mistake. After the first save, contenteditable="true" are lost,
|
||||
so this is a bad practice / requires a better system to do this in the future.
|
||||
Meanwhile, this is fixed in JS (see COMPANY_TEAM_CONTENTEDITABLE).
|
||||
-->
|
||||
<template id="s_company_team" name="Team">
|
||||
<section class="s_company_team pt48 pb48">
|
||||
<div class="container">
|
||||
@@ -15,7 +8,7 @@ Meanwhile, this is fixed in JS (see COMPANY_TEAM_CONTENTEDITABLE).
|
||||
<div class="col-lg-6 pt24 pb24">
|
||||
<div class="row s_col_no_resize s_col_no_bgcolor">
|
||||
<div class="col-lg-4 pb16 o_not_editable" contenteditable="false">
|
||||
<img alt="" src="/web/image/website.s_company_team_image_1" class="img-fluid rounded-circle mx-auto" contenteditable="true"/>
|
||||
<img alt="" src="/web/image/website.s_company_team_image_1" class="img-fluid rounded-circle mx-auto o_editable_media"/>
|
||||
</div>
|
||||
<div class="col-lg-8">
|
||||
<h4>Tony Fred, CEO</h4>
|
||||
@@ -30,7 +23,7 @@ Meanwhile, this is fixed in JS (see COMPANY_TEAM_CONTENTEDITABLE).
|
||||
<div class="col-lg-6 pt24 pb24">
|
||||
<div class="row s_col_no_resize s_col_no_bgcolor">
|
||||
<div class="col-lg-4 pb16 o_not_editable" contenteditable="false">
|
||||
<img alt="" src="/web/image/website.s_company_team_image_2" class="img-fluid rounded-circle mx-auto" contenteditable="true"/>
|
||||
<img alt="" src="/web/image/website.s_company_team_image_2" class="img-fluid rounded-circle mx-auto o_editable_media"/>
|
||||
</div>
|
||||
<div class="col-lg-8">
|
||||
<h4>Mich Stark, COO</h4>
|
||||
@@ -41,7 +34,7 @@ Meanwhile, this is fixed in JS (see COMPANY_TEAM_CONTENTEDITABLE).
|
||||
<div class="col-lg-6 pt24 pb24">
|
||||
<div class="row s_col_no_resize s_col_no_bgcolor">
|
||||
<div class="col-lg-4 pb16 o_not_editable" contenteditable="false">
|
||||
<img alt="" src="/web/image/website.s_company_team_image_3" class="img-fluid rounded-circle mx-auto" contenteditable="true"/>
|
||||
<img alt="" src="/web/image/website.s_company_team_image_3" class="img-fluid rounded-circle mx-auto o_editable_media"/>
|
||||
</div>
|
||||
<div class="col-lg-8">
|
||||
<h4>Aline Turner, CTO</h4>
|
||||
@@ -52,7 +45,7 @@ Meanwhile, this is fixed in JS (see COMPANY_TEAM_CONTENTEDITABLE).
|
||||
<div class="col-lg-6 pt24 pb24">
|
||||
<div class="row s_col_no_resize s_col_no_bgcolor">
|
||||
<div class="col-lg-4 pb16 o_not_editable" contenteditable="false">
|
||||
<img alt="" src="/web/image/website.s_company_team_image_4" class="img-fluid rounded-circle mx-auto" contenteditable="true"/>
|
||||
<img alt="" src="/web/image/website.s_company_team_image_4" class="img-fluid rounded-circle mx-auto o_editable_media"/>
|
||||
</div>
|
||||
<div class="col-lg-8">
|
||||
<h4>Iris Joe, CFO</h4>
|
||||
|
||||
Reference in New Issue
Block a user