diff --git a/modules/ui/web/src/main/scala/oxygen/ui/web/internal/PageManager.scala b/modules/ui/web/src/main/scala/oxygen/ui/web/internal/PageManager.scala index da441900..49607ac5 100644 --- a/modules/ui/web/src/main/scala/oxygen/ui/web/internal/PageManager.scala +++ b/modules/ui/web/src/main/scala/oxygen/ui/web/internal/PageManager.scala @@ -120,8 +120,9 @@ object PageManager { _ <- pageInstance.render(navType) - // W7-T03: after first paint, scroll to #fragment (retry for async content) - _ <- oxygen.ui.web.service.HashScroll.toFragment(fragment).unit + // W7-T03 / OXY-155: after first paint, scroll to #fragment (retry for async content), + // or reset to the top of the page when there is no fragment (page switch). + _ <- oxygen.ui.web.service.PageScroll.onNavigate(fragment) _ <- ZIO.logTrace("PageManager.postLoad.start") _ <- diff --git a/modules/ui/web/src/main/scala/oxygen/ui/web/service/PageScroll.scala b/modules/ui/web/src/main/scala/oxygen/ui/web/service/PageScroll.scala new file mode 100644 index 00000000..67244594 --- /dev/null +++ b/modules/ui/web/src/main/scala/oxygen/ui/web/service/PageScroll.scala @@ -0,0 +1,34 @@ +package oxygen.ui.web.service + +import zio.* + +/** + * Scroll behavior for a page switch (OXY-155). + * + * When navigating to a new page: scroll to the `#anchor` when the URL has one, otherwise reset to + * the top of the page. Without this, a client-side page switch inherits the previous page's scroll + * position (the HolyGrail center pane is reused), so the new page "lands in the middle". + */ +object PageScroll { + + /** Where a navigation should scroll to. */ + enum Target { + case Top + case Fragment(id: String) + } + + /** Pure decision: blank/empty fragment ⇒ [[Target.Top]]; otherwise scroll to the cleaned id. */ + def targetFor(fragment: Option[String]): Target = + fragment.map(_.trim.stripPrefix("#").trim).filter(_.nonEmpty) match { + case Some(id) => Target.Fragment(id) + case None => Target.Top + } + + /** Apply the scroll behavior for a page switch, given the navigating URL's fragment. */ + def onNavigate(fragment: Option[String]): UIO[Unit] = + targetFor(fragment) match { + case Target.Top => Window.scroll.toTop() + case Target.Fragment(id) => HashScroll.toFragment(Some(id)).unit + } + +} diff --git a/modules/ui/web/src/main/scala/oxygen/ui/web/service/Window.scala b/modules/ui/web/src/main/scala/oxygen/ui/web/service/Window.scala index e6fe950a..433e6d8d 100644 --- a/modules/ui/web/src/main/scala/oxygen/ui/web/service/Window.scala +++ b/modules/ui/web/src/main/scala/oxygen/ui/web/service/Window.scala @@ -89,6 +89,21 @@ object Window { ), ) + /** Page-content scroll containers (HolyGrail center pane, reused across page switches). */ + private val scrollContainerSelector: String = + ".oxy-holy-grail-center, .oxy-holy-grail--center-only" + + private def resetScrollContainers(): Unit = { + val nodes = D.querySelectorAll(scrollContainerSelector) + var i = 0 + while i < nodes.length do { + val el = nodes(i) + el.scrollTop = 0 + el.scrollLeft = 0 + i += 1 + } + } + private def scrollElementIntoView(el: org.scalajs.dom.Element, smooth: Boolean): Unit = el.asInstanceOf[js.Dynamic].scrollIntoView( js.Dynamic.literal( @@ -118,6 +133,18 @@ object Window { def toY(y: Double, smooth: Boolean = true): UIO[Unit] = ZIO.succeed { scrollWindowTo(y, smooth) } + /** + * Reset scroll to the very top of the page. Resets both the document/window scroll and the + * reused page-content scroll containers (HolyGrail center pane). Used on page switch so a new + * page appears at the top instead of inheriting the previous page's scroll position. + * Instant by default: a fresh page should render at the top, not animate up from mid-page. + */ + def toTop(smooth: Boolean = false): UIO[Unit] = + ZIO.succeed { + scrollWindowTo(0, smooth) + resetScrollContainers() + } + } } diff --git a/modules/ui/web/src/test/scala/oxygen/ui/web/style/OxygenColorSystemSpec.scala b/modules/ui/web/src/test/scala/oxygen/ui/web/style/OxygenColorSystemSpec.scala index fcd10d5c..074e67d7 100644 --- a/modules/ui/web/src/test/scala/oxygen/ui/web/style/OxygenColorSystemSpec.scala +++ b/modules/ui/web/src/test/scala/oxygen/ui/web/style/OxygenColorSystemSpec.scala @@ -7,6 +7,7 @@ import oxygen.ui.web.component.{ColorPicker, DatePicker, DateTimePicker, Icon, I import oxygen.ui.web.create.{AnchorId, CSSColor, MediaCSS, Motion} import oxygen.ui.web.service.{IndexedDB, Intersect} import oxygen.ui.web.service.HashScroll +import oxygen.ui.web.service.PageScroll import oxygen.ui.web.style.Breakpoints import oxygen.ui.web.style.OxygenColorSystem.* import zio.http.{Path, QueryParams} @@ -383,6 +384,19 @@ object OxygenColorSystemSpec extends OxygenSpecDefault { assertTrue(w.forall(_.ratio > 0)) }, ), + suite("PageScroll (OXY-155)")( + test("no fragment resets to top of page") { + assertTrue(PageScroll.targetFor(None) == PageScroll.Target.Top) && + assertTrue(PageScroll.targetFor(Some("")) == PageScroll.Target.Top) && + assertTrue(PageScroll.targetFor(Some(" ")) == PageScroll.Target.Top) && + assertTrue(PageScroll.targetFor(Some("#")) == PageScroll.Target.Top) + }, + test("fragment scrolls to the cleaned anchor id") { + assertTrue(PageScroll.targetFor(Some("section-1")) == PageScroll.Target.Fragment("section-1")) && + assertTrue(PageScroll.targetFor(Some("#section-1")) == PageScroll.Target.Fragment("section-1")) && + assertTrue(PageScroll.targetFor(Some(" #section-1 ")) == PageScroll.Target.Fragment("section-1")) + }, + ), ) } diff --git a/report/OXY-155.md b/report/OXY-155.md new file mode 100644 index 00000000..17ae0d09 --- /dev/null +++ b/report/OXY-155.md @@ -0,0 +1,72 @@ +# OXY-155 — Page switch lands in the middle of the page + +Bug (Normal). Module: `oxygen-ui` (`modules/ui/web`). +Jira description + comments are EMPTY — root cause derived from code investigation. + +## Symptom +Client-side navigation (router push/replace) to a new page shows the new page +scrolled to the middle/bottom instead of the top, unless the URL has a `#anchor`. + +## Root cause +- `PageManager.loadPage` (`internal/PageManager.scala`) renders the new page then, + after first paint, calls `HashScroll.toFragment(fragment)`. +- When the navigating URL has NO fragment, `toFragment(None)` is a no-op → scroll + position is left wherever the previous page had it. +- The actual scroll container is usually `.oxy-holy-grail-center` (HolyGrail sets the + shell to `height:100vh; overflow:hidden`, and the center pane is `overflow-y:auto` + via `OxygenStyleSheet.Scrollable`). That center `div` is reused across page switches, + so its `scrollTop` persists → new page lands mid-page. +- Pages not using HolyGrail scroll the document/window instead; that also isn't reset. + +## Fix +- New pure/effect helper `service/PageScroll.scala`: + - `targetFor(fragment): Target` → `Top` when fragment empty/blank, else `Fragment(id)` + (strips `#`, trims). Pure + unit-testable. + - `onNavigate(fragment)` → `Top` ⇒ `Window.scroll.toTop()`; `Fragment(id)` ⇒ existing + `HashScroll.toFragment` (retry for async content). +- New `Window.scroll.toTop(smooth=false)`: + - resets the document/window scroll to (0,0), AND + - resets `scrollTop/scrollLeft` on `.oxy-holy-grail-center` / `.oxy-holy-grail--center-only` + scroll containers (the reused page-content scrollers). + - Instant (not smooth): a fresh page should appear at top, no animation from mid-page. +- `PageManager.loadPage` now calls `PageScroll.onNavigate(fragment)` instead of + `HashScroll.toFragment(fragment)`. + +## Decisions / assumptions +- Reset targets ONLY the center content scrollers, NOT all `.scrollable` elements — + sidebars (`SideBar` uses `O.Scrollable`) are shell chrome; keeping their scroll is desirable. +- Fix applied for every `loadPage` (Push / Replace / browserLoad-None). `loadPage` is only + invoked for real page navigation (a new PageInstance replacing the previous); in-place + re-render goes through `reRenderCurrentPage`/`render`, which does NOT reset scroll. Correct. +- Anchor/hash navigation behavior unchanged (still scrolls to the fragment). +- Custom non-HolyGrail scroll containers other than the two center classes are not explicitly + reset, but the window reset covers document-level scrolling (common case). Edge case noted. + +## Tests +- JS test suite (`OxygenColorSystemSpec`) runs on Node without a DOM, so DOM scrolling can't be + exercised. Added pure-logic tests for `PageScroll.targetFor` (Top vs Fragment, `#`/blank handling). + +## Files changed +- `modules/ui/web/.../service/PageScroll.scala` (new) — Target ADT + `targetFor` + `onNavigate`. +- `modules/ui/web/.../service/Window.scala` — added `scroll.toTop` + center-container reset helper. +- `modules/ui/web/.../internal/PageManager.scala` — `loadPage` calls `PageScroll.onNavigate`. +- `modules/ui/web/.../style/OxygenColorSystemSpec.scala` — `PageScroll` test suite. + +## Build/verify notes +- `sbt oxygen-ui-web/test` → 47 tests pass (incl. new PageScroll suite). Clean compile (-Werror). +- Env gotcha: sbt-git's JGit backend throws `NoWorkTreeException` in a linked git worktree. Used a + TEMPORARY `zzz-worktree-git-fix.sbt` (overriding `git.gitUncommittedChanges/CurrentTags/HeadCommit`) + only to load the build; it was DELETED and is NOT part of the changeset. Not a code issue. + +## Final summary +Root cause identified and fixed: page switches now reset scroll to top (window + reused HolyGrail +center scroll container) unless the URL carries a `#anchor`, in which case the existing anchor-scroll +behavior is preserved. Compiles and all tests pass. + +### CONFIDENCE: 8/10 +- High confidence on the root cause (only-scroll-on-fragment) and that the fix addresses it for the + common HolyGrail + document-scroll cases. +- Not lowered further because: I could not run the actual app/browser to visually confirm (no DOM in + the JS test env), and pages using a bespoke non-HolyGrail scroll container (not `.oxy-holy-grail-center` + / `.oxy-holy-grail--center-only`) are only covered by the window reset, not an element reset. + Both are edge cases; the standard navigation path is well covered.