From 788f1a8b8d186cd69bb369a4573d02d99aafea10 Mon Sep 17 00:00:00 2001 From: Bruno Boi Date: Tue, 18 May 2021 12:40:04 +0000 Subject: [PATCH] [IMP] web: avoid useless navbar adaptations After this commit, adaptations for the more menu behaviour are not computed when there is no sections menu at all. Also, before this commit, the more menu dropdown was always attached in the dom, even if not visible. Now it is only attached when needed. closes odoo-dev/odoo#814 Related: odoo-dev/enterprise#102 Signed-off-by: Aaron Bohy (aab) --- .../web/static/src/webclient/navbar/navbar.js | 27 ++-- .../static/src/webclient/navbar/navbar.xml | 8 +- .../static/tests/webclient/navbar_tests.js | 122 ++++++++++++++++-- 3 files changed, 135 insertions(+), 22 deletions(-) diff --git a/addons/web/static/src/webclient/navbar/navbar.js b/addons/web/static/src/webclient/navbar/navbar.js index 206f19bb985..7f9a9e287cb 100644 --- a/addons/web/static/src/webclient/navbar/navbar.js +++ b/addons/web/static/src/webclient/navbar/navbar.js @@ -8,7 +8,7 @@ import { registry } from "../../core/registry"; import { debounce } from "../../core/utils/timing"; const { Component, hooks } = owl; -const { useExternalListener } = hooks; +const { useExternalListener, useRef } = hooks; const systrayRegistry = registry.category("systray"); export class MenuDropdown extends Dropdown { @@ -43,6 +43,7 @@ export class NavBar extends Component { this.currentAppSectionsExtra = []; this.actionService = useService("action"); this.menuService = useService("menu"); + this.appSubMenus = useRef("appSubMenus"); const debouncedAdapt = debounce(this.adapt.bind(this), 250); useExternalListener(window, "resize", debouncedAdapt); } @@ -97,28 +98,34 @@ export class NavBar extends Component { } // ------- Initialize ------- - // Check actual "more" dropdown state - const moreDropdown = this.el.querySelector(".o_menu_sections_more"); + // Get the sectionsMenu + const sectionsMenu = this.appSubMenus.el; + if (!sectionsMenu) { + // No need to continue adaptations if there is no sections menu. + return; + } + + // Save initial state to further check if new render has to be done. const initialAppSectionsExtra = this.currentAppSectionsExtra; const firstInitialAppSectionExtra = [...initialAppSectionsExtra].shift(); const initialAppId = firstInitialAppSectionExtra && firstInitialAppSectionExtra.appID; // Restore (needed to get offset widths) const sections = [ - ...this.el.querySelectorAll(".o_menu_sections > *:not(.o_menu_sections_more)"), + ...sectionsMenu.querySelectorAll(":scope > *:not(.o_menu_sections_more)"), ]; - sections.forEach((s) => s.classList.remove("d-none")); + for (const section of sections) { + section.classList.remove("d-none"); + } this.currentAppSectionsExtra = []; - moreDropdown.classList.add("d-none"); // ------- Check overflowing sections ------- - const sectionsMenu = this.el.querySelector(".o_menu_sections"); const sectionsAvailableWidth = sectionsMenu.offsetWidth; const sectionsTotalWidth = sections.reduce((sum, s) => sum + s.offsetWidth, 0); if (sectionsAvailableWidth < sectionsTotalWidth) { - // Sections are overflowing, show "more" menu - moreDropdown.classList.remove("d-none"); - let width = moreDropdown.offsetWidth; + // Sections are overflowing + // Initial width is harcoded to the width the more menu dropdown will take + let width = 46; for (const section of sections) { if (sectionsAvailableWidth < width + section.offsetWidth) { // Last sections are overflowing diff --git a/addons/web/static/src/webclient/navbar/navbar.xml b/addons/web/static/src/webclient/navbar/navbar.xml index bc4254e63c1..cbf8a0cb295 100644 --- a/addons/web/static/src/webclient/navbar/navbar.xml +++ b/addons/web/static/src/webclient/navbar/navbar.xml @@ -15,10 +15,10 @@ class="o_menu_brand" payload="currentApp" t-esc="currentApp.name" - t-ref="menuBrand"/> + t-ref="menuBrand" /> - + @@ -76,7 +76,7 @@ - + @@ -104,7 +104,7 @@ - + diff --git a/addons/web/static/tests/webclient/navbar_tests.js b/addons/web/static/tests/webclient/navbar_tests.js index 5e7f099cf10..3bdd5fd7b09 100644 --- a/addons/web/static/tests/webclient/navbar_tests.js +++ b/addons/web/static/tests/webclient/navbar_tests.js @@ -188,9 +188,13 @@ QUnit.test("can adapt with 'more' menu sections behavior", async (assert) => { }; baseConfig.serverData.menus = newMenus; const env = await makeTestEnv(baseConfig); + + // Force the parent width, to make this test independent of screen size + const target = getFixture(); + target.style.width = "1080px"; + // Set menu and mount env.services.menu.setCurrentMenu(1); - const target = getFixture(); const navbar = await mount(MyNavbar, { env, target }); assert.containsN( navbar.el, @@ -198,10 +202,10 @@ QUnit.test("can adapt with 'more' menu sections behavior", async (assert) => { 3, "should have 3 menu sections displayed (that are not the 'more' menu)" ); - assert.containsOnce( + assert.containsNone( navbar.el, - ".o_menu_sections_more.d-none", - "the 'more' menu should be hidden" + ".o_menu_sections_more", + "the 'more' menu should not exist" ); // Force minimal width and dispatch window resize event navbar.el.style.width = "0%"; @@ -234,10 +238,10 @@ QUnit.test("can adapt with 'more' menu sections behavior", async (assert) => { 3, "should have 3 menu sections displayed (that are not the 'more' menu)" ); - assert.containsOnce( + assert.containsNone( navbar.el, - ".o_menu_sections_more.d-none", - "the 'more' menu should be hidden" + ".o_menu_sections_more", + "the 'more' menu should not exist" ); // Check the navbar adaptation calls assert.verifySteps([ @@ -248,6 +252,105 @@ QUnit.test("can adapt with 'more' menu sections behavior", async (assert) => { navbar.destroy(); }); +QUnit.test("'more' menu sections adaptations do not trigger render in some cases", async (assert) => { + let adaptRunning = false; + let adaptCount = 0; + let adaptRenderCount = 0; + class MyNavbar extends NavBar { + async adapt() { + adaptRunning = true; + adaptCount++; + await super.adapt(); + adaptRunning = false; + } + async render() { + if (adaptRunning) { + adaptRenderCount++; + } + await super.render(...arguments); + } + } + + const newMenus = { + root: { id: "root", children: [1], name: "root", appID: "root" }, + 1: { id: 1, children: [11, 12, 13], name: "App1", appID: 1 }, + 11: { id: 11, children: [], name: "Section 1", appID: 1 }, + 12: { id: 12, children: [], name: "Section 2", appID: 1 }, + 13: { id: 13, children: [], name: "Section 3", appID: 1 }, + }; + baseConfig.serverData.menus = newMenus; + + // Force the parent width, to make this test independent of screen size + const target = getFixture(); + target.style.width = "600px"; + + const env = await makeTestEnv(baseConfig); + const navbar = await mount(MyNavbar, { env, target }); + assert.strictEqual(navbar.currentAppSections.length, 0, "0 app sub menus"); + assert.strictEqual(navbar.el.offsetWidth, 600); + assert.strictEqual(adaptCount, 1); + assert.strictEqual( + adaptRenderCount, 0, + "during adapt, render not triggered as the navbar has no app sub menus" + ); + + // Force minimal width and dispatch window resize event + navbar.el.style.width = "0%"; + window.dispatchEvent(new Event("resize")); + await nextTick(); + assert.strictEqual(navbar.el.offsetWidth, 0); + assert.strictEqual(adaptCount, 2); + assert.strictEqual( + adaptRenderCount, 0, + "during adapt, render not triggered as the navbar has no app sub menus" + ); + + // Set menu + env.services.menu.setCurrentMenu(1); + await nextTick(); + assert.strictEqual(navbar.currentAppSections.length, 3, "3 app sub menus"); + assert.strictEqual( + navbar.currentAppSectionsExtra.length, 3, + "all app sub menus are inside the more menu" + ); + assert.strictEqual(adaptCount, 3); + assert.strictEqual( + adaptRenderCount, 1, + "during adapt, render triggered as the navbar does not have enough space for app sub menus" + ); + + // Force 40% width and dispatch window resize event + navbar.el.style.width = "40%"; + window.dispatchEvent(new Event("resize")); + await nextTick(); + assert.strictEqual( + navbar.currentAppSectionsExtra.length, 3, + "all app sub menus are STILL inside the more menu" + ); + assert.strictEqual(adaptCount, 4); + assert.strictEqual( + adaptRenderCount, 1, + "during adapt, render not triggered as the more menu dropdown is STILL the same" + ); + + // Reset to full width and dispatch window resize event + navbar.el.style.width = "100%"; + window.dispatchEvent(new Event("resize")); + await nextTick(); + assert.strictEqual(navbar.currentAppSections.length, 3, "still 3 app sub menus"); + assert.strictEqual( + navbar.currentAppSectionsExtra.length, 0, + "all app sub menus are NO MORE inside the more menu" + ); + assert.strictEqual(adaptCount, 5); + assert.strictEqual( + adaptRenderCount, 2, + "during adapt, render triggered as the more menu dropdown is NO MORE the same" + ); + + navbar.destroy(); +}); + QUnit.test("'more' menu sections properly updated on app change", async (assert) => { const newMenus = { root: { id: "root", children: [1, 2], name: "root", appID: "root" }, @@ -271,9 +374,12 @@ QUnit.test("'more' menu sections properly updated on app change", async (assert) baseConfig.serverData.menus = newMenus; const env = await makeTestEnv(baseConfig); + // Force the parent width, to make this test independent of screen size + const target = getFixture(); + target.style.width = "1080px"; + // Set App1 menu and mount env.services.menu.setCurrentMenu(1); - const target = getFixture(); const navbar = await mount(NavBar, { env, target }); // Force minimal width and dispatch window resize event