From 829f5a5e707177f5096398ea6244a015385e8901 Mon Sep 17 00:00:00 2001 From: Konstantin Gogov Date: Mon, 28 Oct 2024 13:08:17 +0200 Subject: [PATCH 1/3] fix(ui5-dynamic-page): improve scrolling smoothness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug Description When scrolling, the header content would snap into place, causing an unintended shift in the page content scroll position. This resulted in the main content scrolling further down than expected, disrupting the user’s view. Root Cause Snapping the header set the height of the headerArea content to 0, causing a layout shift. This shift created additional space, which moved the main content of the dynamic page upwards unexpectedly. Fix Details - Modified `snapTitleByScroll` to handle snapping conditions more precisely, using `requestAnimationFrame` to wait for rendering to complete before adjusting scroll positions. - Adjusted logic to maintain a stable scroll position when the header snaps or unsnaps, preventing unnecessary layout shifts. Outcome The header now snaps smoothly, maintaining the main content’s scroll position and providing a seamless user experience. Fixes: #10011 --- packages/fiori/src/DynamicPage.ts | 22 +++++++++++++++++----- packages/fiori/src/themes/DynamicPage.css | 7 ------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/packages/fiori/src/DynamicPage.ts b/packages/fiori/src/DynamicPage.ts index 44b71c969420..cf4448468948 100644 --- a/packages/fiori/src/DynamicPage.ts +++ b/packages/fiori/src/DynamicPage.ts @@ -246,7 +246,7 @@ class DynamicPage extends UI5Element { } get headerInContent(): boolean { - return !this.showHeaderInStickArea && !this.headerInTitle; + return !this.showHeaderInStickArea && !this.headerInTitle && !this.hasSnappedTitleOnMobile; } get _headerLabel() { @@ -308,6 +308,7 @@ class DynamicPage extends UI5Element { } const scrollTop = this.scrollContainer!.scrollTop; + const headerHeight = this.dynamicPageHeader.getBoundingClientRect().height; const lastHeaderSnapped = this._headerSnapped; if (this.skipSnapOnScroll) { @@ -315,18 +316,29 @@ class DynamicPage extends UI5Element { return; } - if (scrollTop > this.dynamicPageHeader.getBoundingClientRect().height) { + // Snap if above threshold and not already snapped + if (!this._headerSnapped && scrollTop > headerHeight + SCROLL_THRESHOLD) { this.showHeaderInStickArea = false; this._headerSnapped = true; - } else { + + // Wait for render to adjust scrollTop if necessary + requestAnimationFrame(() => { + if (this.scrollContainer!.scrollTop === 0) { + this.scrollContainer!.scrollTop = SCROLL_THRESHOLD; + } + }); + return; // Early return to avoid further checks + } + + // Unsnap if below threshold + if (scrollTop < headerHeight - SCROLL_THRESHOLD || (!scrollTop && !headerHeight && this._headerSnapped)) { this._headerSnapped = false; } + // Fire event if snapped state changed if (lastHeaderSnapped !== this._headerSnapped) { this.fireDecoratorEvent("title-toggle"); } - - this.dynamicPageTitle.snapped = this._headerSnapped; } async onExpandClick() { diff --git a/packages/fiori/src/themes/DynamicPage.css b/packages/fiori/src/themes/DynamicPage.css index c4415e41418b..93db2137cae4 100644 --- a/packages/fiori/src/themes/DynamicPage.css +++ b/packages/fiori/src/themes/DynamicPage.css @@ -126,11 +126,4 @@ :host([media-range="XL"]) ::slotted([slot="headerArea"]) { padding: var(--_ui5_dynamic_page_header_padding_XL); -} - -/* snappedTitleOnMobile */ -:host([_header-snapped]) ::slotted([slot="headerArea"]) { - height: 0; - padding: 0; - visibility: hidden; } \ No newline at end of file From 00e0e6cf15005609d2f0682ad8910cd3e98b6ecf Mon Sep 17 00:00:00 2001 From: Konstantin Gogov Date: Thu, 31 Oct 2024 10:40:52 +0200 Subject: [PATCH 2/3] fix(ui5-dynamic-page): added unit test and improved the snapping logic readablity --- packages/fiori/src/DynamicPage.ts | 27 ++++++------- .../test/specs/DynamicPage.mobile.spec.js | 38 +++++++++++++++++++ 2 files changed, 52 insertions(+), 13 deletions(-) diff --git a/packages/fiori/src/DynamicPage.ts b/packages/fiori/src/DynamicPage.ts index cf4448468948..7b7f7bcb445e 100644 --- a/packages/fiori/src/DynamicPage.ts +++ b/packages/fiori/src/DynamicPage.ts @@ -307,35 +307,36 @@ class DynamicPage extends UI5Element { return; } - const scrollTop = this.scrollContainer!.scrollTop; - const headerHeight = this.dynamicPageHeader.getBoundingClientRect().height; - const lastHeaderSnapped = this._headerSnapped; - if (this.skipSnapOnScroll) { this.skipSnapOnScroll = false; return; } - // Snap if above threshold and not already snapped - if (!this._headerSnapped && scrollTop > headerHeight + SCROLL_THRESHOLD) { + const scrollTop = this.scrollContainer!.scrollTop; + const headerHeight = this.dynamicPageHeader.getBoundingClientRect().height; + const lastHeaderSnapped = this._headerSnapped; + + const shouldSnap = !this._headerSnapped && scrollTop > headerHeight + SCROLL_THRESHOLD; + const shouldUnsnap = this._headerSnapped + && (scrollTop < headerHeight - SCROLL_THRESHOLD + || (!scrollTop && !headerHeight)); + + if (shouldSnap) { this.showHeaderInStickArea = false; this._headerSnapped = true; - // Wait for render to adjust scrollTop if necessary + //* snappTitleOnMobile + // If the header is snapped and the scroll is at the top, scroll down a bit + // to make avoid ending in endless loop of snapping and unsnapping requestAnimationFrame(() => { if (this.scrollContainer!.scrollTop === 0) { this.scrollContainer!.scrollTop = SCROLL_THRESHOLD; } }); - return; // Early return to avoid further checks - } - - // Unsnap if below threshold - if (scrollTop < headerHeight - SCROLL_THRESHOLD || (!scrollTop && !headerHeight && this._headerSnapped)) { + } else if (shouldUnsnap) { this._headerSnapped = false; } - // Fire event if snapped state changed if (lastHeaderSnapped !== this._headerSnapped) { this.fireDecoratorEvent("title-toggle"); } diff --git a/packages/fiori/test/specs/DynamicPage.mobile.spec.js b/packages/fiori/test/specs/DynamicPage.mobile.spec.js index 6407fb7a5dab..5a532e5fcfd5 100644 --- a/packages/fiori/test/specs/DynamicPage.mobile.spec.js +++ b/packages/fiori/test/specs/DynamicPage.mobile.spec.js @@ -23,6 +23,44 @@ describe("DynamicPage Mobile Behaviour", () => { ); }); + it("the header content should not be rendered when snappedTitleOnMobile content is present", async () => { + const dynamicPage = await browser.$("#page"); + const headerAreaSlot = await dynamicPage.shadow$("slot[name=headerArea]"); + const scrollContainer = await dynamicPage.shadow$(".ui5-dynamic-page-scroll-container"); + + const initialScrollTop = await scrollContainer.getProperty("scrollTop"); + + // Scroll the content to snap the header + await browser.execute((container) => { + container.scrollTop = 340; + }, scrollContainer); + + // Wait until the scroll position has changed + await browser.waitUntil( + async () => (await scrollContainer.getProperty("scrollTop")) > initialScrollTop, + { + timeout: 2000, + timeoutMsg: "The scroll handler must be called.", + } + ); + + // Assert that the header is snapped + const isHeaderSnapped = await dynamicPage.getProperty("headerSnapped"); + assert.strictEqual( + isHeaderSnapped, + true, + "Header should be snapped when the content is scrolled." + ); + + // Assert that the header content is not rendered + const isHeaderContentRendered = await headerAreaSlot.isExisting(); + assert.strictEqual( + isHeaderContentRendered, + false, + "Header content should not be rendered when snappedTitleOnMobile slot has content." + ); + }); + it("should not display snapped title on mobile when snappedTitleOnMobile slot is empty", async () => { const dynamicPage = await browser.$("#page"); const title = await browser.$("#page ui5-dynamic-page-title"); From 150cb6a614b5fdb6acfd5689a9fa083c0697abc7 Mon Sep 17 00:00:00 2001 From: Konstantin Gogov Date: Mon, 28 Oct 2024 13:08:17 +0200 Subject: [PATCH 3/3] fix(ui5-dynamic-page): improve scrolling smoothness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug Description When scrolling, the header content would snap into place, causing an unintended shift in the page content scroll position. This resulted in the main content scrolling further down than expected, disrupting the user’s view. Root Cause Snapping the header set the height of the headerArea content to 0, causing a layout shift. This shift created additional space, which moved the main content of the dynamic page upwards unexpectedly. Fix Details - Modified `snapTitleByScroll` to handle snapping conditions more precisely, using `requestAnimationFrame` to wait for rendering to complete before adjusting scroll positions. - Adjusted logic to maintain a stable scroll position when the header snaps or unsnaps, preventing unnecessary layout shifts. Outcome The header now snaps smoothly, maintaining the main content’s scroll position and providing a seamless user experience. Fixes: #10011 --- packages/fiori/src/DynamicPage.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/fiori/src/DynamicPage.ts b/packages/fiori/src/DynamicPage.ts index 7b7f7bcb445e..98f76a52d5f5 100644 --- a/packages/fiori/src/DynamicPage.ts +++ b/packages/fiori/src/DynamicPage.ts @@ -325,9 +325,9 @@ class DynamicPage extends UI5Element { this.showHeaderInStickArea = false; this._headerSnapped = true; - //* snappTitleOnMobile + //* snappedTitleOnMobile // If the header is snapped and the scroll is at the top, scroll down a bit - // to make avoid ending in endless loop of snapping and unsnapping + // to avoid ending in an endless loop of snapping and unsnapping requestAnimationFrame(() => { if (this.scrollContainer!.scrollTop === 0) { this.scrollContainer!.scrollTop = SCROLL_THRESHOLD; @@ -337,6 +337,7 @@ class DynamicPage extends UI5Element { this._headerSnapped = false; } + // Fire event if snapped state changed if (lastHeaderSnapped !== this._headerSnapped) { this.fireDecoratorEvent("title-toggle"); }