From 7931d1a14a3e4e65463cc101536061fa259b615d Mon Sep 17 00:00:00 2001 From: qsm-odoo Date: Tue, 16 Jan 2024 14:41:36 +0100 Subject: [PATCH] [FIX] website, web_editor: review "small" font-size behavior Before this commit, since [1], the "small" class behavior of Bootstrap was changed to use a fixed value instead of being dependent on the context (using `em` units). This was a bad choice, given the fact that the "small" class could be used in the past with its previous behavior in custo but also in default Odoo layouts, where it could be use in more legit cases that the one Odoo currently offers: instead of applying the class to a whole paragraph, applying it to part of a title. In that case, since [1], we can have a big title ... with a very small text next to it, which may be strange. Of course, the proper way to achieve a big title with a smaller text next to it would be to not use "small" but another hx font-size, but the legit bootstrap behavior which is to use their "small" class is thus broken. This commit keeps the current possibilities (big title with very small text next to it) but does it by using new custom Odoo classes instead of changing the Bootstrap "small" one. That way, legit custo in previous versions (or trying to use default Bootstrap in this version) will be supported. Note that this commit will impact existing 17.0 users though: if they actually configured a big title with small inner text... those will become big title with slightly smaller inner text (the default Bootstrap behavior). Worse: a big title whose size was reduced using the font-size selector and choosing "small" will now become a big title. We think this is worth the risk (see PR description for more visual details). However, notice that "legit" use case of the "small" font-size in 17.0 will be kept untouched: e.g. adding a small text on its own or next to a paragraph: the "smaller" Bootstrap behavior is now computed based on the ratio of the configured base and small font-sizes. Note that some additional "bugs" were found investigating this: - Form descriptions use the "small" class for no reason and should probably not be possible to customize ("font style"-wise) anyway. - The use of the "small" *tag* should probably be reviewed in all Odoo layouts and/or its interaction with the editor be fixed. [1]: https://github.com/odoo/odoo/commit/194f73a9bbad8c3c3fb5c378e7bdfa704aaacdc0 closes odoo/odoo#149590 Signed-off-by: Romain Derie (rde) --- .../js/editor/odoo-editor/src/OdooEditor.js | 12 ++++++++--- .../js/editor/odoo-editor/src/utils/utils.js | 12 +++++++++-- .../static/src/scss/bootstrap_overridden.scss | 10 ++++++++++ .../static/src/scss/secondary_variables.scss | 6 ++++++ .../static/src/scss/web_editor.common.scss | 20 ++++++++++++++++++- addons/web_editor/static/src/xml/editor.xml | 4 ++-- .../static/src/scss/bootstrap_overridden.scss | 2 -- .../static/src/scss/primary_variables.scss | 12 ++++++----- .../static/src/scss/secondary_variables.scss | 2 ++ .../static/src/xml/website_form_editor.xml | 10 ++++++++++ .../tests/tours/website_text_font_size.js | 6 +++++- 11 files changed, 80 insertions(+), 16 deletions(-) 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 c5f9c1f69c1..04a62824843 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 @@ -3172,7 +3172,10 @@ export class OdooEditor extends EventTarget { const block = closestBlock(sel.anchorNode); let activeLabel = undefined; for (const [style, cssSelector, isList] of [ - ['paragraph', 'p:not(.small, .lead)', false], + // TODO we might want to review this list to not mention o_xxx + // classes but be a setting instead? Probably after current + // refactorings being made in master. + ['paragraph', 'p:not(.small, .lead, .o_small)', false], ['pre', 'pre', false], ['heading1', 'h1:not(.display-1, .display-2, .display-3, .display-4)', false], ['heading2', 'h2', false], @@ -3185,7 +3188,10 @@ export class OdooEditor extends EventTarget { ['display-3', 'h1.display-3', false], ['display-4', 'h1.display-4', false], ['blockquote', 'blockquote', false], - ['small', '.small', false], + // Note: this button will apply the "o_small" class but as an + // approximation, we display "Small" if this actually use the + // Bootstrap "small" class. + ['small', '.small, .o_small', false], ['light', '.lead', false], ['unordered', 'UL', true], ['ordered', 'OL', true], @@ -3247,7 +3253,7 @@ export class OdooEditor extends EventTarget { const range = getDeepRange(this.editable, { sel, correctTripleClick: true }); const spansBlocks = [...range.commonAncestorContainer.childNodes].some(isBlock); linkButton?.classList.toggle('d-none', spansBlocks || isInMedia); - + // Hide link button group if it has no visible button. const linkBtnGroup = this.toolbar.querySelector('#link.btn-group'); linkBtnGroup?.classList.toggle('d-none', !linkBtnGroup.querySelector('.btn:not(.d-none)')); diff --git a/addons/web_editor/static/src/js/editor/odoo-editor/src/utils/utils.js b/addons/web_editor/static/src/js/editor/odoo-editor/src/utils/utils.js index 36e4b0b84b1..519e10cea28 100644 --- a/addons/web_editor/static/src/js/editor/odoo-editor/src/utils/utils.js +++ b/addons/web_editor/static/src/js/editor/odoo-editor/src/utils/utils.js @@ -63,14 +63,22 @@ export const PROTECTED_BLOCK_TAG = ['TR','TD','TABLE','TBODY','UL','OL','LI']; /** * Array of all the classes used by the editor to change the font size. + * + * Note: the Bootstrap "small" class is an exception, the editor does not allow + * to set it but it did in the past and we want to remove it when applying an + * override of the font-size. */ export const FONT_SIZE_CLASSES = ["display-1-fs", "display-2-fs", "display-3-fs", "display-4-fs", "h1-fs", - "h2-fs", "h3-fs", "h4-fs", "h5-fs", "h6-fs", "base-fs", "small"]; + "h2-fs", "h3-fs", "h4-fs", "h5-fs", "h6-fs", "base-fs", "o_small-fs", "small"]; /** * Array of all the classes used by the editor to change the text style. + * + * Note: the Bootstrap "small" class was actually part of "text style" + * configuration in the past... but also of the "font size" configuration (see + * FONT_SIZE_CLASSES). It should be mentioned here too. */ -export const TEXT_STYLE_CLASSES = ["display-1", "display-2", "display-3", "display-4", "lead"]; +export const TEXT_STYLE_CLASSES = ["display-1", "display-2", "display-3", "display-4", "lead", "o_small", "small"]; //------------------------------------------------------------------------------ // Position and sizes diff --git a/addons/web_editor/static/src/scss/bootstrap_overridden.scss b/addons/web_editor/static/src/scss/bootstrap_overridden.scss index f80dd76c1e2..dda72169439 100644 --- a/addons/web_editor/static/src/scss/bootstrap_overridden.scss +++ b/addons/web_editor/static/src/scss/bootstrap_overridden.scss @@ -84,3 +84,13 @@ $gray-900: map-get($grays, '900') !default; $black: map-get($grays, 'black') !default; $o-color-system-initialized: true; + +// This was added by compatibility but it actually became a nice behavior: the +// bootstrap default "small" behavior will use the ratio of the configured base +// font size (if configured, e.g. with website settings) and the Odoo own's +// "small" font size. Grep: SMALLER_FONT_SIZE_RATIO. +$small-font-size: if( + variable-exists('font-size-base'), + ($o-small-font-size / $font-size-base) * 1em, + null +) !default; diff --git a/addons/web_editor/static/src/scss/secondary_variables.scss b/addons/web_editor/static/src/scss/secondary_variables.scss index e90c3452133..17e83bb9399 100644 --- a/addons/web_editor/static/src/scss/secondary_variables.scss +++ b/addons/web_editor/static/src/scss/secondary_variables.scss @@ -140,3 +140,9 @@ $o-we-auto-contrast-exclusions: () !default; $colors: str-replace($colors, ' ', '%20'); @return $colors; } + +//------------------------------------------------------------------------------ +// Fonts +//------------------------------------------------------------------------------ + +$o-small-font-size: 0.875rem !default; diff --git a/addons/web_editor/static/src/scss/web_editor.common.scss b/addons/web_editor/static/src/scss/web_editor.common.scss index db4fe2f841a..b203d9e59ec 100644 --- a/addons/web_editor/static/src/scss/web_editor.common.scss +++ b/addons/web_editor/static/src/scss/web_editor.common.scss @@ -90,7 +90,6 @@ @include print-variable('h5-font-size', $h5-font-size); @include print-variable('h6-font-size', $h6-font-size); @include print-variable('font-size-base', $font-size-base); - @include print-variable('small-font-size', $small-font-size); } html, body { @@ -250,6 +249,19 @@ img.ms-auto, img.mx-auto { width: auto; } +%o-small-font-size { + @include font-size($o-small-font-size); +} +// Dedicated class to be able to keep the default "small" behavior of bootstrap: +// being "smaller" that the context where it is used (em units). Here we want to +// define a specific fixed font-size for a smaller font-size than the base font +// size. Note that this class is designed to work as the display-x classes: an +// extra "styling" class to go on an element. For the "font-size class" +// equivalent, see o_small-fs below. +.o_small { + @extend %o-small-font-size; +} + @for $index from 1 through 4 { .display-#{$index}-fs { @include font-size(map-get($display-font-sizes, $index)); @@ -276,6 +288,12 @@ img.ms-auto, img.mx-auto { .base-fs { @include font-size($font-size-base); } +// Equivalent "font-size" only for the Odoo own "o_small" class. Note that the +// "o_small" class currently also changes the font-size only but this is to stay +// consistent with the other classes which act that way (as display-x). +.o_small-fs { + @extend %o-small-font-size; +} div.media_iframe_video { margin: 0 auto; diff --git a/addons/web_editor/static/src/xml/editor.xml b/addons/web_editor/static/src/xml/editor.xml index 6f7f60b1eff..de439d81826 100644 --- a/addons/web_editor/static/src/xml/editor.xml +++ b/addons/web_editor/static/src/xml/editor.xml @@ -50,7 +50,7 @@ Light
  • - Small + Small
  • Code @@ -112,7 +112,7 @@
  • ? Heading 5
  • ? Heading 6
  • ? Normal
    • -
  • ? Small
    • +
  • ? Small
    • diff --git a/addons/website/static/src/scss/bootstrap_overridden.scss b/addons/website/static/src/scss/bootstrap_overridden.scss index 5deccfd09e3..fe262fca0bc 100644 --- a/addons/website/static/src/scss/bootstrap_overridden.scss +++ b/addons/website/static/src/scss/bootstrap_overridden.scss @@ -130,8 +130,6 @@ $display-font-sizes: ( 6: o-website-value('display-6-font-size') or 2.5rem ) !default; -$small-font-size: o-website-value('small-font-size') !default; - // H2~H6 font families are custom variables. $headings-font-family: $o-theme-headings-font !default; $h2-font-family: $o-theme-h2-font !default; diff --git a/addons/website/static/src/scss/primary_variables.scss b/addons/website/static/src/scss/primary_variables.scss index a268ace0984..a2dbf49b34f 100644 --- a/addons/website/static/src/scss/primary_variables.scss +++ b/addons/website/static/src/scss/primary_variables.scss @@ -1973,11 +1973,13 @@ $o-base-website-values-palette: ( 'display-3-font-size': null, // Default to BS 'display-4-font-size': null, // Default to BS - // Forced to use `rem` by default instead of Bootstrap default using `em` as - // the editor would not be working with `em` values for now. This will have - // the drawback of changing the default small behavior of bootstrap which - // can normally be put inside titles etc... but that's not the way the - // editor will allow to use it for now. This is a compromise. + // Note that this custom value is not directly linked to the + // "small-font-size" variable of bootstrap. Indeed we want the default + // "small" behavior of bootstrap to stay the same: being "smaller" that the + // context where it is used (em units). Here we want to define a specific + // fixed font-size for a smaller font-size than the base font size. However, + // the default value for the "smaller" behavior of Bootstrap is based on + // this value anyway. See SMALLER_FONT_SIZE_RATIO. 'small-font-size': 0.875rem, 'google-fonts': null, diff --git a/addons/website/static/src/scss/secondary_variables.scss b/addons/website/static/src/scss/secondary_variables.scss index 09ea82423a8..20ac820e1ee 100644 --- a/addons/website/static/src/scss/secondary_variables.scss +++ b/addons/website/static/src/scss/secondary_variables.scss @@ -283,3 +283,5 @@ $o-theme-display-3-font: o-get-font-info('display-3') or $o-theme-headings-font $o-theme-display-4-font: o-get-font-info('display-4') or $o-theme-headings-font !default; $o-theme-navbar-font: o-get-font-info('navbar') or $o-theme-font !default; $o-theme-buttons-font: o-get-font-info('buttons') or $o-theme-font !default; + +$o-small-font-size: o-website-value('small-font-size') !default; diff --git a/addons/website/static/src/xml/website_form_editor.xml b/addons/website/static/src/xml/website_form_editor.xml index 8d0e34bcdd9..d293719f4ba 100644 --- a/addons/website/static/src/xml/website_form_editor.xml +++ b/addons/website/static/src/xml/website_form_editor.xml @@ -68,6 +68,16 @@ +
    diff --git a/addons/website/static/tests/tours/website_text_font_size.js b/addons/website/static/tests/tours/website_text_font_size.js index 7f94deb1ca2..4f876eaff65 100644 --- a/addons/website/static/tests/tours/website_text_font_size.js +++ b/addons/website/static/tests/tours/website_text_font_size.js @@ -15,7 +15,7 @@ classNameInfo.set("h4-fs", {scssVariableName: "h4-font-size", start: 24, end: 34 classNameInfo.set("h5-fs", {scssVariableName: "h5-font-size", start: 20, end: 30}); classNameInfo.set("h6-fs", {scssVariableName: "h6-font-size", start: 16, end: 26}); classNameInfo.set("base-fs", {scssVariableName: "font-size-base", start: 16, end: 26}); -classNameInfo.set("small", {scssVariableName: "small-font-size", start: 14, end: 24}); +classNameInfo.set("o_small-fs", {scssVariableName: "small-font-size", start: 14, end: 24}); function checkComputedFontSize(fontSizeClass, stage) { return { @@ -99,6 +99,10 @@ function getAllFontSizesTestSteps() { // That option is hidden by default because same value as base-fs continue; } + if (fontSizeClass === 'small') { + // There is nothing related to that class in the UI to test anymore. + continue; + } steps.push(...getFontSizeTestSteps(fontSizeClass)); } return steps;