From e756d9bc809861cc2c1e7ccb12516de8ef69a9d8 Mon Sep 17 00:00:00 2001 From: luvi Date: Tue, 9 Nov 2021 08:43:13 +0000 Subject: [PATCH] [FIX] web: anchor scroll crash This commit fixes some issues happening when scrolling with an anchor in Odoo. When the anchor was contained inside of a notebook and no scrollable was found, a crash happens. This fix improves the behavior in notebooks by showing the correct pane before scrolling in form renderer. The scrollTo util has been improved to remove duplicate lines and handle multiple situations (0, one or more scrollables). Tests haven been written to test such behaviors. closes odoo/odoo#79935 X-original-commit: 4c2b807d1cb32e52a1b1fb5e6c80fb7e933940bc Signed-off-by: Aaron Bohy (aab) --- .../web/static/src/core/scroller_service.js | 12 +- addons/web/static/src/core/utils/scrolling.js | 34 ++--- .../src/legacy/js/views/form/form_renderer.js | 56 ++++++++ addons/web/static/src/legacy/utils.js | 4 + .../tests/core/scroller_service_tests.js | 133 ++++++++++++++++++ .../static/tests/legacy/views/form_tests.js | 122 +++++++++++++++- 6 files changed, 341 insertions(+), 20 deletions(-) diff --git a/addons/web/static/src/core/scroller_service.js b/addons/web/static/src/core/scroller_service.js index 7aecc82621e..bfdcfa67764 100644 --- a/addons/web/static/src/core/scroller_service.js +++ b/addons/web/static/src/core/scroller_service.js @@ -5,7 +5,7 @@ import { registry } from "./registry"; import { scrollTo } from "./utils/scrolling"; export const scrollerService = { - start() { + start(env) { /** * Listen to click event to allow having links with href towards an anchor. * Since odoo use hashtag to represent the current state of the view, we can't @@ -41,7 +41,15 @@ export const scrollerService = { } catch (e) { // Invalid selector: not an anchor anyway } - if (matchingEl) { + const triggerEv = new CustomEvent("anchor-link-clicked", { + detail: { + element: matchingEl, + id: href.value, + originalEv: ev, + }, + }); + env.bus.trigger("SCROLLER:ANCHOR_LINK_CLICKED", triggerEv); + if (matchingEl && !triggerEv.defaultPrevented) { ev.preventDefault(); scrollTo(matchingEl, { isAnchor: true }); } diff --git a/addons/web/static/src/core/utils/scrolling.js b/addons/web/static/src/core/utils/scrolling.js index 37226b84489..282cf7bc836 100644 --- a/addons/web/static/src/core/utils/scrolling.js +++ b/addons/web/static/src/core/utils/scrolling.js @@ -14,33 +14,35 @@ export function scrollTo(element, options = { scrollable: null, isAnchor: false return null; } - if (node.scrollHeight > node.clientHeight) { + if (node.scrollHeight > node.clientHeight && node.clientHeight > 0) { return node; } else { return _getScrollParent(node.parentNode); } } - const scrollable = options.scrollable ? options.scrollable : _getScrollParent(element); - // Scrollbar is present ? - if (scrollable.scrollHeight > scrollable.clientHeight) { + const scrollable = options.scrollable + ? options.scrollable + : _getScrollParent(element.parentNode); + if (scrollable) { const scrollBottom = scrollable.getBoundingClientRect().bottom; const scrollTop = scrollable.getBoundingClientRect().top; const elementBottom = element.getBoundingClientRect().bottom; const elementTop = element.getBoundingClientRect().top; - if (elementBottom > scrollBottom) { - // Scroll down - if (options.isAnchor) { - // For an anchor, the scroll place the element at the top - scrollable.scrollTop += elementTop - scrollBottom + scrollable.clientHeight; - } else { - // The scroll make the element visible in the scrollable - scrollable.scrollTop += - elementTop - scrollBottom + element.getBoundingClientRect().height; - } - } else if (elementTop < scrollTop) { - // Scroll up + if (elementBottom > scrollBottom && !options.isAnchor) { + // The scroll place the element at the bottom border of the scrollable + scrollable.scrollTop += + elementTop - scrollBottom + element.getBoundingClientRect().height; + } else if (elementTop < scrollTop || options.isAnchor) { + // The scroll place the element at the top of the scrollable scrollable.scrollTop -= scrollTop - elementTop; + if (options.isAnchor) { + // If the scrollable is within a scrollable, another scroll should be done + const parentScrollable = _getScrollParent(scrollable.parentNode); + if (parentScrollable) { + scrollTo(scrollable, { isAnchor: true, scrollable: parentScrollable }); + } + } } } } diff --git a/addons/web/static/src/legacy/js/views/form/form_renderer.js b/addons/web/static/src/legacy/js/views/form/form_renderer.js index 80950ec3fca..3edc28e95c7 100644 --- a/addons/web/static/src/legacy/js/views/form/form_renderer.js +++ b/addons/web/static/src/legacy/js/views/form/form_renderer.js @@ -10,6 +10,7 @@ var viewUtils = require('web.viewUtils'); var _t = core._t; var qweb = core.qweb; +const scrollUtils = require("@web/core/utils/scrolling"); // symbol used as key to set the node id on its widget const symbol = Symbol('form'); @@ -63,6 +64,20 @@ var FormRenderer = BasicRenderer.extend({ this._applyFormSizeClass(); return this._super.apply(this, arguments); }, + /** + * Called each time the form view is attached into the DOM + */ + on_attach_callback: function () { + this._super.apply(this, arguments); + core.bus.on("SCROLLER:ANCHOR_LINK_CLICKED", this, this._onAnchorLinkClicked); + }, + /** + * Called each time the form view is detached into the DOM + */ + on_detach_callback: function () { + this._super.apply(this, arguments); + core.bus.off("SCROLLER:ANCHOR_LINK_CLICKED", this, this._onAnchorLinkClicked); + }, //-------------------------------------------------------------------------- // Public @@ -1219,6 +1234,47 @@ var FormRenderer = BasicRenderer.extend({ var index = this.allFieldWidgets[this.state.id].indexOf(ev.data.target); this._activateNextFieldWidget(this.state, index); }, + /** + * Ensure that the pane containing an anchor `element` that has been + * targeted by a link will be visible in a notebook. + * @param {CustomEvent} ev + * @param {object} ev[detail] payload containing the element and the id to look for + * + */ + _onAnchorLinkClicked(ev) { + // Todo: we might need to search for the element elsewhere to know wich tab to activate + // Maybe a t-if rule is hidding the target from the DOM + const anchor = ev.detail.element || null; + if (!anchor) { + return; + } + function _getNotebookParent() { + const notebook = anchor.closest(".o_notebook"); + // If the notebook is containing the element, return the notebook + return notebook && notebook.contains(anchor) ? notebook : null; + } + function _setNotebookPage() { + // Simulate a click on the nav link corresponding to the target pane + const parentPane = anchor.closest(".tab-pane"); + if (notebook.contains(parentPane) && !parentPane.classList.contains(".active")) { + const navLink = [...notebook.querySelectorAll(".nav-link")].filter((e) => + e.href.includes(parentPane.id) + ); + navLink[0].click(); + } + } + + const notebook = _getNotebookParent(); + + // If the element is contained in a notebook, the page must be visible + if (notebook) { + _setNotebookPage(); + // Prevent the scroll to be handled by the scroller service itself + ev.preventDefault(); + ev.detail.originalEv.preventDefault(); + scrollUtils.scrollTo(ev.detail.element, { isAnchor: true }); + } + }, /** * @private * @param {MouseEvent} ev diff --git a/addons/web/static/src/legacy/utils.js b/addons/web/static/src/legacy/utils.js index af515c76bcb..c4541c6b2d7 100644 --- a/addons/web/static/src/legacy/utils.js +++ b/addons/web/static/src/legacy/utils.js @@ -176,6 +176,10 @@ export function mapLegacyEnvToWowlEnv(legacyEnv, wowlEnv) { legacyEnv.bus.trigger("web_client_ready"); }); + wowlEnv.bus.on("SCROLLER:ANCHOR_LINK_CLICKED", null, (payload) => { + legacyEnv.bus.trigger("SCROLLER:ANCHOR_LINK_CLICKED", payload); + }); + legacyEnv.bus.on("clear_cache", null, () => { wowlEnv.bus.trigger("CLEAR-CACHES"); }); diff --git a/addons/web/static/tests/core/scroller_service_tests.js b/addons/web/static/tests/core/scroller_service_tests.js index 3fc333323d4..901456112ab 100644 --- a/addons/web/static/tests/core/scroller_service_tests.js +++ b/addons/web/static/tests/core/scroller_service_tests.js @@ -187,6 +187,139 @@ QUnit.test("Rendering with multiple anchors and scrolls", async (assert) => { assert.ok(isVisible(scrollableParent.querySelector("#anchor3"))); }); +QUnit.test("clicking anchor when no scrollable", async (assert) => { + assert.expect(3); + const scrollableParent = document.createElement("div"); + scrollableParent.style.overflow = "auto"; + scrollableParent.style.height = "150px"; + scrollableParent.style.width = "400px"; + target.append(scrollableParent); + + class MyComponent extends Component {} + MyComponent.template = tags.xml/* xml */ ` +
+ scroll to ... +
+

There is no scrollable with only the height of this element

+
+
+

There should be no scrollable if this element has 0 height

+

+ Aliquam convallis sollicitudin purus. Praesent aliquam, enim at fermentum mollis, + ligula massa adipiscing nisl, ac euismod nibh nisl eu lectus. Fusce vulputate sem + at sapien. Vivamus leo. Aliquam euismod libero eu enim. Nulla nec felis sed leo + placerat imperdiet. Aenean suscipit nulla in justo. Suspendisse cursus rutrum + augue. Nulla tincidunt tincidunt mi. Curabitur iaculis, lorem vel rhoncus faucibus, + felis magna fermentum augue, et ultricies lacus lorem varius purus. Curabitur eu amet. +

+
should try to scroll here only if scrollable!
+
+
+ `; + comp = await mount(MyComponent, { env, target: scrollableParent }); + assert.strictEqual(scrollableParent.scrollTop, 0); + await click(scrollableParent, ".btn.btn-primary"); + assert.ok(scrollableParent.scrollTop === 0, "no scroll happened"); + scrollableParent.querySelector(".inactive-container").style.maxHeight = "unset"; + await click(scrollableParent, ".btn.btn-primary"); + assert.ok(scrollableParent.scrollTop !== 0, "a scroll happened"); +}); + +QUnit.test("clicking anchor when multi levels scrollables", async (assert) => { + assert.expect(4); + const scrollableParent = document.createElement("div"); + scrollableParent.style.overflow = "auto"; + scrollableParent.style.height = "150px"; + scrollableParent.style.width = "400px"; + target.append(scrollableParent); + + class MyComponent extends Component {} + MyComponent.template = tags.xml/* xml */ ` +
+ go to level 2 anchor +
+

This is some content

+

+ Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed non risus. + Suspendisse lectus tortor, dignissim sit amet, adipiscing nec, ultricies sed, + dolor. Cras elementum ultrices diam. Maecenas +

+
+
+

This is level 1 of scrollable

+

+ Aliquam convallis sollicitudin purus. Praesent aliquam, enim at fermentum mollis, + ligula massa adipiscing nisl, ac euismod nibh nisl eu lectus. Fusce vulputate sem + at sapien. Vivamus leo. Aliquam euismod libero eu enim. Nulla nec felis sed leo + placerat imperdiet. Aenean suscipit nulla in justo. Suspendisse cursus rutrum + augue. Nulla tincidunt tincidunt mi. Curabitur iaculis, lorem vel rhoncus faucibus, + felis magna fermentum augue, et ultricies lacus lorem varius purus. Curabitur eu amet. +

+
+

This is level 2 of scrollable

+

+ Aliquam convallis sollicitudin purus. Praesent aliquam, enim at fermentum mollis, + ligula massa adipiscing nisl, ac euismod nibh nisl eu lectus. Fusce vulputate sem + at sapien. Vivamus leo. Aliquam euismod libero eu enim. Nulla nec felis sed leo + placerat imperdiet. Aenean suscipit nulla in justo. Suspendisse cursus rutrum + augue. Nulla tincidunt tincidunt mi. Curabitur iaculis, lorem vel rhoncus faucibus, + felis magna fermentum augue, et ultricies lacus lorem varius purus. Curabitur eu amet. +

+
this element is contained in a scrollable metaverse!
+ go to level 1 anchor +

+ Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed non risus. + Suspendisse lectus tortor, dignissim sit amet, adipiscing nec, ultricies sed, + dolor. Cras elementum ultrices diam. Maecenas ligula massa, varius a, semper + congue, euismod non, mi. Proin porttitor, orci nec nonummy molestie, enim est + eleifend mi, non fermentum diam nisl sit amet erat. Duis semper. Duis arcu + massa, scelerisque vitae, consequat in, pretium a, enim. Pellentesque congue. Ut + in risus volutpat libero pharetra tempor. Cras vestibulum bibendum augue. Praesent + egestas leo in pede. Praesent blandit odio eu enim. +

+
+
+
this is an anchor at level 1!
+

+ Aliquam convallis sollicitudin purus. Praesent aliquam, enim at fermentum mollis, + ligula massa adipiscing nisl, ac euismod nibh nisl eu lectus. Fusce vulputate sem + at sapien. Vivamus leo. Aliquam euismod libero eu enim. Nulla nec felis sed leo + at sapien. Vivamus leo. Aliquam euismod libero eu enim. Nulla nec felis sed leo + placerat imperdiet. Aenean suscipit nulla in justo. Suspendisse cursus rutrum + augue. Nulla tincidunt tincidunt mi. Curabitur iaculis, lorem vel rhoncus faucibus, + felis magna fermentum augue, et ultricies lacus lorem varius purus. Curabitur eu amet. +

+
+ `; + comp = await mount(MyComponent, { env, target: scrollableParent }); + + const border = (el) => { + // Returns the state of the element in relation to the borders + const element = el.getBoundingClientRect(); + const scrollable = scrollableParent.getBoundingClientRect(); + return { + top: parseInt(element.top - scrollable.top) < 10, + bottom: parseInt(scrollable.bottom - element.bottom) < 10, + }; + }; + + assert.strictEqual(scrollableParent.scrollTop, 0); + await click(scrollableParent, ".btn1"); + assert.ok( + border(scrollableParent.querySelector("#scroll1")).top, + "the element must be near the top border" + ); + assert.ok( + border(scrollableParent.querySelector("#scroll1")).top, + "the scrollable inside level 1 must be near the top border" + ); + await click(scrollableParent, ".btn2"); + assert.ok( + border(scrollableParent.querySelector("#scroll2")).top, + "the element must be near the top border" + ); +}); + QUnit.test("Simple scroll to HTML elements", async (assert) => { assert.expect(6); const scrollableParent = document.createElement("div"); diff --git a/addons/web/static/tests/legacy/views/form_tests.js b/addons/web/static/tests/legacy/views/form_tests.js index ce66ab3139f..dccb474fc9a 100644 --- a/addons/web/static/tests/legacy/views/form_tests.js +++ b/addons/web/static/tests/legacy/views/form_tests.js @@ -23,12 +23,19 @@ var _t = core._t; var createView = testUtils.createView; const { registerCleanup } = require("@web/../tests/helpers/cleanup"); -const { legacyExtraNextTick, patchWithCleanup } = require("@web/../tests/helpers/utils"); +const { getFixture, legacyExtraNextTick, patchWithCleanup } = require("@web/../tests/helpers/utils"); const { createWebClient, doAction } = require('@web/../tests/webclient/helpers'); +const { makeTestEnv } = require("@web/../tests/helpers/mock_env"); +const makeTestEnvironment = require("web.test_env"); +const { mapLegacyEnvToWowlEnv } = require("@web/legacy/utils"); +const { registry } = require("@web/core/registry"); +const { scrollerService } = require("@web/core/scroller_service"); let serverData; QUnit.module('Views', { - beforeEach: function () { + beforeEach: async function () { + registry.category("services").add("scroller", scrollerService); + this.data = { partner: { fields: { @@ -1009,6 +1016,117 @@ QUnit.module('Views', { form.destroy(); }); + QUnit.test("notebook page is changing when an anchor is clicked from another page", async (assert) => { + assert.expect(6); + + // This should be removed as soon as the view is moved to owl + const wowlEnv = await makeTestEnv(); + const legacyEnv = makeTestEnvironment({ bus: core.bus }); + mapLegacyEnvToWowlEnv(legacyEnv, wowlEnv); + + const scrollableParent = document.createElement("div"); + scrollableParent.style.overflow = "auto"; + const target = getFixture(); + target.append(scrollableParent); + + var form = await createView({ + View: FormView, + model: "partner", + data: { + partner: { + fields: {}, + records: [ + { + id: 1, + }, + ], + }, + }, + arch: `
+ + + +
No scrollbar!
+ TO ANCHOR 2 + + +

+ Aliquam convallis sollicitudin purus. Praesent aliquam, enim at fermentum mollis, + ligula massa adipiscing nisl, ac euismod nibh nisl eu lectus. Fusce vulputate sem + at sapien. Vivamus leo. Aliquam euismod libero eu enim. Nulla nec felis sed leo + placerat imperdiet. Aenean suscipit nulla in justo. Suspendisse cursus rutrum + augue. +

+

+ Aliquam convallis sollicitudin purus. Praesent aliquam, enim at fermentum mollis, + ligula massa adipiscing nisl, ac euismod nibh nisl eu lectus. Fusce vulputate sem + at sapien. Vivamus leo. Aliquam euismod libero eu enim. Nulla nec felis sed leo + placerat imperdiet. Aenean suscipit nulla in justo. Suspendisse cursus rutrum + augue. +

+

There is a scroll bar

+ TO ANCHOR 1 +

+ Aliquam convallis sollicitudin purus. Praesent aliquam, enim at fermentum mollis, + ligula massa adipiscing nisl, ac euismod nibh nisl eu lectus. Fusce vulputate sem + at sapien. Vivamus leo. Aliquam euismod libero eu enim. Nulla nec felis sed leo + placerat imperdiet. Aenean suscipit nulla in justo. Suspendisse cursus rutrum + augue. +

+ + + +
`, + res_id: 1, + }); + scrollableParent.append(form.el); + + // We set the height of the parent to the height of the second pane + // We are then sure there will be no scrollable on this pane but a + // only for the first pane + scrollableParent.style.maxHeight = + scrollableParent.querySelector(".o_action").getBoundingClientRect().height + "px"; + + // The element must be contained in the scrollable parent (top and bottom) + const isVisible = (el) => { + return ( + el.getBoundingClientRect().bottom <= scrollableParent.getBoundingClientRect().bottom && + el.getBoundingClientRect().top >= scrollableParent.getBoundingClientRect().top + ); + }; + + assert.ok( + scrollableParent + .querySelector(".tab-pane.active") + .contains(scrollableParent.querySelector("#anchor1")), + "the first pane is visible" + ); + assert.ok( + !isVisible(scrollableParent.querySelector("#anchor2")), + "the second anchor is not visible" + ); + scrollableParent.querySelector(".link2").click(); + assert.ok( + scrollableParent + .querySelector(".tab-pane.active") + .contains(scrollableParent.querySelector("#anchor2")), + "the second pane is visible" + ); + assert.ok( + isVisible(scrollableParent.querySelector("#anchor2")), + "the second anchor is visible" + ); + scrollableParent.querySelector(".link1").click(); + assert.ok( + scrollableParent + .querySelector(".tab-pane.active") + .contains(scrollableParent.querySelector("#anchor1")), + "the first pane is visible" + ); + assert.ok(isVisible(scrollableParent.querySelector("#anchor1")), "the first anchor is visible"); + form.destroy(); + }); + QUnit.test('invisible attrs on group are re-evaluated on field change', async function (assert) { assert.expect(2);