Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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")
_ <-
Expand Down
Original file line number Diff line number Diff line change
@@ -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
}

}
27 changes: 27 additions & 0 deletions modules/ui/web/src/main/scala/oxygen/ui/web/service/Window.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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()
}

}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down Expand Up @@ -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"))
},
),
)

}
72 changes: 72 additions & 0 deletions report/OXY-155.md
Original file line number Diff line number Diff line change
@@ -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.
Loading