From d304be57beb86c1f67dd6227dc8acbb7dc915fb8 Mon Sep 17 00:00:00 2001 From: luvi Date: Wed, 27 Oct 2021 06:03:03 +0000 Subject: [PATCH] [FIX] web: scrolling to anchor working badly This commit fixes a bug when you clicked on a link related to an anchor. e.g. an tag with an href="#anchor". The scroll to the element with id="anchor" didn't happen because the position was not found correctly. The fix uses getBoundingCLientRect to get more precise positions of the element with the anchor and the scrollable area. A test is introduced to check scrolling for multiple anchors in a template with a more complex structure (Note: the test fails without the fix). The scroll behaves differently depending if the target is an anchor or an element. An other test has been added for the command palette about this behavior. closes odoo/odoo#79310 X-original-commit: b0da44d62f85d6530f8e568025f7fea25c5454c5 Signed-off-by: FrancoisGe --- .../src/core/commands/command_palette.js | 2 +- .../core/dropdown/dropdown_navigation_hook.js | 2 +- .../web/static/src/core/scroller_service.js | 2 +- addons/web/static/src/core/utils/scrolling.js | 27 ++- .../core/commands/command_palette_tests.js | 158 ++++++++++++++++++ .../tests/core/scroller_service_tests.js | 153 +++++++++++++++++ 6 files changed, 333 insertions(+), 11 deletions(-) diff --git a/addons/web/static/src/core/commands/command_palette.js b/addons/web/static/src/core/commands/command_palette.js index 8f46d5292c4..30ecec7a56f 100644 --- a/addons/web/static/src/core/commands/command_palette.js +++ b/addons/web/static/src/core/commands/command_palette.js @@ -212,7 +212,7 @@ export class CommandPalette extends Component { const listbox = this.el.querySelector(".o_command_palette_listbox"); const command = listbox.querySelector(`#o_command_${nextIndex}`); - scrollTo(command, listbox); + scrollTo(command, { scrollable: listbox }); } onCommandClicked(index) { diff --git a/addons/web/static/src/core/dropdown/dropdown_navigation_hook.js b/addons/web/static/src/core/dropdown/dropdown_navigation_hook.js index 37c4bb7a09a..b490a7dda88 100644 --- a/addons/web/static/src/core/dropdown/dropdown_navigation_hook.js +++ b/addons/web/static/src/core/dropdown/dropdown_navigation_hook.js @@ -184,7 +184,7 @@ export function useDropdownNavigation() { menuElement = menuElements[nextIndex]; } menuElement.makeOnlyActive(); - scrollTo(menuElement.el, menuElement.el.parentElement); + scrollTo(menuElement.el, { scrollable: menuElement.el.parentElement }); } }; diff --git a/addons/web/static/src/core/scroller_service.js b/addons/web/static/src/core/scroller_service.js index aafd57dc6cf..7aecc82621e 100644 --- a/addons/web/static/src/core/scroller_service.js +++ b/addons/web/static/src/core/scroller_service.js @@ -43,7 +43,7 @@ export const scrollerService = { } if (matchingEl) { ev.preventDefault(); - scrollTo(matchingEl); + 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 40cf6c92636..37226b84489 100644 --- a/addons/web/static/src/core/utils/scrolling.js +++ b/addons/web/static/src/core/utils/scrolling.js @@ -4,9 +4,11 @@ * Ensures that `element` will be visible in its `scrollable`. * * @param {HTMLElement} element - * @param {HTMLElement} [scrollable] + * @param {Object} options + * @param {HTMLElement} options[scrollable] a scrollable area + * @param {Boolean} options[isAnchor] states if the scroll is to an anchor */ -export function scrollTo(element, scrollable = null) { +export function scrollTo(element, options = { scrollable: null, isAnchor: false }) { function _getScrollParent(node) { if (node == null) { return null; @@ -18,18 +20,27 @@ export function scrollTo(element, scrollable = null) { return _getScrollParent(node.parentNode); } } - scrollable = scrollable ? scrollable : _getScrollParent(element); + const scrollable = options.scrollable ? options.scrollable : _getScrollParent(element); // Scrollbar is present ? if (scrollable.scrollHeight > scrollable.clientHeight) { - const scrollBottom = scrollable.clientHeight + scrollable.scrollTop; - const elementBottom = element.offsetTop + element.offsetHeight; + 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 - scrollable.scrollTop = elementBottom - scrollable.clientHeight; - } else if (element.offsetTop < scrollable.scrollTop) { + 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 - scrollable.scrollTop = element.offsetTop; + scrollable.scrollTop -= scrollTop - elementTop; } } } diff --git a/addons/web/static/tests/core/commands/command_palette_tests.js b/addons/web/static/tests/core/commands/command_palette_tests.js index c0dc77fed56..6f467c4d720 100644 --- a/addons/web/static/tests/core/commands/command_palette_tests.js +++ b/addons/web/static/tests/core/commands/command_palette_tests.js @@ -732,6 +732,164 @@ QUnit.test("press enter on command", async (assert) => { assert.verifySteps(["C2"]); }); +QUnit.test("keyboard navigation scroll", async (assert) => { + testComponent = await mount(TestComponent, { env, target }); + const commands = [ + { name: "Command1" }, + { name: "Command2" }, + { name: "Command3" }, + { name: "Command4" }, + ]; + const providers = [ + { + provide: () => commands, + }, + ]; + const config = { + providers, + }; + env.services.dialog.add(CommandPaletteDialog, { + config, + }); + + const isVisible = (el) => { + // Returns the visibility of the element in the scrollable element + return ( + el.getBoundingClientRect().bottom <= + target.querySelector(".o_command_palette_listbox").getBoundingClientRect().bottom && + el.getBoundingClientRect().top >= + target.querySelector(".o_command_palette_listbox").getBoundingClientRect().top + ); + }; + + const border = (el) => { + // Returns the state of the element in relation to the borders + const element = el.getBoundingClientRect(); + const scrollable = target + .querySelector(".o_command_palette_listbox") + .getBoundingClientRect(); + return { + top: element.top === scrollable.top, + bottom: element.bottom === scrollable.bottom, + }; + }; + + await nextTick(); + // The listbox height is set to be lower than the list of commands + // to assure the command palette is scrollable. The palette is only able to + // display three rows of commands so we are sure we always have one row + // element out of bounds + target.querySelectorAll(".o_command").forEach((e) => (e.style.height = "50px")); + target.querySelector(".o_command_palette_listbox").style.maxHeight = "150px"; + target.querySelector(".o_command_category").style.padding = "0"; + assert.containsOnce(target, ".o_command_palette"); + assert.containsN(target, ".o_command", 4); + + let focusedCommand = target.querySelector(".o_command.focused"); + assert.ok( + isVisible(target.querySelector("#o_command_0")) && + isVisible(target.querySelector("#o_command_1")) && + isVisible(target.querySelector("#o_command_2")) && + !isVisible(target.querySelector("#o_command_3")), + "commands 1-2-3 are visible" + ); + assert.ok( + border(focusedCommand).top && !border(focusedCommand).bottom, + "the focus is at the top border" + ); + + triggerHotkey("arrowdown"); + await nextTick(); + focusedCommand = target.querySelector(".o_command.focused"); + assert.ok( + isVisible(target.querySelector("#o_command_0")) && + isVisible(target.querySelector("#o_command_1")) && + isVisible(target.querySelector("#o_command_2")) && + !isVisible(target.querySelector("#o_command_3")), + "commands 1-2-3 are visible" + ); + assert.ok(isVisible(target.querySelector("#o_command_1")), "the second element is visible"); + assert.ok( + !border(focusedCommand).top && !border(focusedCommand).bottom, + "the focus does not reach a border" + ); + + triggerHotkey("arrowdown"); + await nextTick(); + focusedCommand = target.querySelector(".o_command.focused"); + assert.ok( + isVisible(target.querySelector("#o_command_0")) && + isVisible(target.querySelector("#o_command_1")) && + isVisible(target.querySelector("#o_command_2")) && + !isVisible(target.querySelector("#o_command_3")), + "commands 1-2-3 are visible" + ); + assert.ok( + !border(focusedCommand).top && border(focusedCommand).bottom, + "the focus has reached the bottom border" + ); + + triggerHotkey("arrowdown"); + await nextTick(); + focusedCommand = target.querySelector(".o_command.focused"); + assert.ok( + !isVisible(target.querySelector("#o_command_0")) && + isVisible(target.querySelector("#o_command_1")) && + isVisible(target.querySelector("#o_command_2")) && + isVisible(target.querySelector("#o_command_3")), + "commands 2-3-4 are visible" + ); + assert.ok( + !border(focusedCommand).top && border(focusedCommand).bottom, + "the focus is still at the bottom border" + ); + + triggerHotkey("arrowup"); + await nextTick(); + focusedCommand = target.querySelector(".o_command.focused"); + assert.ok( + !isVisible(target.querySelector("#o_command_0")) && + isVisible(target.querySelector("#o_command_1")) && + isVisible(target.querySelector("#o_command_2")) && + isVisible(target.querySelector("#o_command_3")), + "commands 2-3-4 are visible" + ); + assert.ok( + !border(focusedCommand).top && !border(focusedCommand).bottom, + "the focus does not reach a border" + ); + + triggerHotkey("arrowup"); + await nextTick(); + focusedCommand = target.querySelector(".o_command.focused"); + assert.ok( + !isVisible(target.querySelector("#o_command_0")) && + isVisible(target.querySelector("#o_command_1")) && + isVisible(target.querySelector("#o_command_2")) && + isVisible(target.querySelector("#o_command_3")), + "commands 2-3-4 are visible" + ); + assert.ok( + border(focusedCommand).top && !border(focusedCommand).bottom, + "the focus has reached the top border" + ); + + triggerHotkey("arrowup"); + await nextTick(); + focusedCommand = target.querySelector(".o_command.focused"); + assert.ok( + isVisible(target.querySelector("#o_command_0")) && + isVisible(target.querySelector("#o_command_1")) && + isVisible(target.querySelector("#o_command_2")) && + !isVisible(target.querySelector("#o_command_3")), + "commands 1-2-3 are visible" + ); + assert.ok( + border(focusedCommand).top && !border(focusedCommand).bottom, + "the focus is still at the top border" + ); +}); + QUnit.test("multi level command", async (assert) => { testComponent = await mount(TestComponent, { env, target }); const emptyMessageByNamespace = { diff --git a/addons/web/static/tests/core/scroller_service_tests.js b/addons/web/static/tests/core/scroller_service_tests.js index 6eb122b015c..3fc333323d4 100644 --- a/addons/web/static/tests/core/scroller_service_tests.js +++ b/addons/web/static/tests/core/scroller_service_tests.js @@ -2,6 +2,7 @@ import { registry } from "@web/core/registry"; import { scrollerService } from "@web/core/scroller_service"; +import { scrollTo } from "@web/core/utils/scrolling"; import { registerCleanup } from "../helpers/cleanup"; import { makeTestEnv } from "../helpers/mock_env"; import { click, getFixture, nextTick } from "../helpers/utils"; @@ -111,3 +112,155 @@ QUnit.test("Simple rendering with a scroll", async (assert) => { await click(scrollableParent, ".btn.btn-primary"); assert.ok(scrollableParent.scrollTop !== 0); }); + +QUnit.test("Rendering with multiple anchors and scrolls", async (assert) => { + assert.expect(4); + const scrollableParent = document.createElement("div"); + scrollableParent.style.overflow = "scroll"; + scrollableParent.style.height = "150px"; + scrollableParent.style.width = "400px"; + target.append(scrollableParent); + + class MyComponent extends Component {} + MyComponent.template = tags.xml/* xml */ ` +
+

ANCHOR 3

+ sroll to ... +

+ 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. +

+

+ Ut velit mauris, egestas sed, gravida nec, ornare ut, mi. Aenean ut orci vel massa + suscipit pulvinar. Nulla sollicitudin. Fusce varius, ligula non tempus aliquam, nunc + turpis ullamcorper nibh, in tempus sapien eros vitae ligula. Pellentesque rhoncus + nunc et augue. Integer id felis. Curabitur aliquet pellentesque diam. Integer quis + metus vitae elit lobortis egestas. Lorem ipsum dolor sit amet, consectetuer adipiscing + elit. Morbi vel erat non mauris convallis vehicula. Nulla et sapien. Integer tortor + tellus, aliquam faucibus, convallis id, congue eu, quam. +

+ + + + + + +
+

ANCHOR 2

+ TO ANCHOR 3 +

+ The table forces you to get the precise position of the element. +

+
+

+ 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. +

+
sroll here!
+ TO ANCHOR 2 +
+ `; + comp = await mount(MyComponent, { env, target: scrollableParent }); + assert.strictEqual(scrollableParent.scrollTop, 0); + await click(scrollableParent, ".link1"); + + // 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(isVisible(scrollableParent.querySelector("#anchor1"))); + await click(scrollableParent, ".link2"); + assert.ok(isVisible(scrollableParent.querySelector("#anchor2"))); + await click(scrollableParent, ".link3"); + assert.ok(isVisible(scrollableParent.querySelector("#anchor3"))); +}); + +QUnit.test("Simple scroll to HTML elements", async (assert) => { + assert.expect(6); + const scrollableParent = document.createElement("div"); + scrollableParent.style.overflow = "scroll"; + scrollableParent.style.height = "150px"; + scrollableParent.style.width = "400px"; + target.append(scrollableParent); + + class MyComponent extends Component {} + MyComponent.template = tags.xml/* xml */ ` +
+

+ Aliquam convallis sollicitudin purus. +

+
A div is an HTML element
+

+ 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. +

+

+ Ut velit mauris, egestas sed, gravida nec, ornare ut, mi. Aenean ut orci vel massa + suscipit pulvinar. Nulla sollicitudin. Fusce varius, ligula non tempus aliquam, nunc + turpis ullamcorper nibh, in tempus sapien eros vitae ligula. Pellentesque rhoncus + nunc et augue. Integer id felis. Curabitur aliquet pellentesque diam. Integer quis + metus vitae elit lobortis egestas. Lorem ipsum dolor sit amet, consectetuer adipiscing + elit. Morbi vel erat non mauris convallis vehicula. Nulla et sapien. Integer tortor + tellus, aliquam faucibus, convallis id, congue eu, quam. +

+
A div is an HTML element
+

+ 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. +

+
+ `; + comp = await mount(MyComponent, { env, target: scrollableParent }); + assert.strictEqual(scrollableParent.scrollTop, 0); + + // 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 + ); + }; + + 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) === 0, + bottom: parseInt(scrollable.bottom - element.bottom) === 0, + }; + }; + + // When using scrollTo to an element, this should just scroll + // until the element is visible in the scrollable parent + const div_1 = scrollableParent.querySelector("#o-div-1"); + const div_2 = scrollableParent.querySelector("#o-div-2"); + assert.ok(isVisible(div_1) && !isVisible(div_2), "only the first div is visible"); + assert.ok(!border(div_1).top, "the element is not at the top border"); + scrollTo(div_2); + assert.ok(!isVisible(div_1) && isVisible(div_2), "only the second div is visible"); + assert.ok(border(div_2).bottom, "the element must be at the bottom border"); + scrollTo(div_1); + assert.ok(border(div_1).top, "the element must be at the top border"); +});