From e66dd7d7a9c8f98244bd37f10be8001872c8c415 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Wed, 9 Sep 2026 16:46:26 +0000 Subject: [PATCH] fix(hierarchy): hold the tree's place when a deep sub-event is clicked Curators, re-testing #137: "no jumping in the search and the analysis table and none in the upper section of event hierarchy, but pathways near the bottom of the hierarchy (e.g. subevents) still jump." They were right, and the revealing was not the cause. Rebuilding the tree starts by emptying it -- a workaround for an Angular Material bug where nested children otherwise do not render -- and emptying it destroys every row, so the container collapses and the browser resets its scroll to the top. Clicking a sub-event that was already on screen measured 274px, then 0, then 6: a jump to the top and a scroll back. Expansion state was already carried across that rebuild. The scroll position now is too, restored again on the next frame because the height is not final until the restored branches have rendered and the browser clamps a scrollTop set against a container that is still short. Clicking a row you can already see now moves the tree 0px, from 274. A row you cannot see is still brought into view -- 274 to 130 for one above the fold -- because that is the half worth keeping, and both are tested. I tried removing the workaround instead, since the bug it cites is from 2018 and it carries a "check performance issue" note. It is still needed: without it the tree renders 29 rows instead of 41 and no branch opens. Left in place, with what it costs written down next to it. Verified to fail without the fix: the already-visible case goes red, the reveal case stays green. Fixes #137 Co-Authored-By: Claude Opus 5 --- e2e/hierarchy-scroll.spec.ts | 131 ++++++++++++++++++ .../event-hierarchy.component.html | 2 +- .../event-hierarchy.component.ts | 21 +++ 3 files changed, 153 insertions(+), 1 deletion(-) create mode 100644 e2e/hierarchy-scroll.spec.ts diff --git a/e2e/hierarchy-scroll.spec.ts b/e2e/hierarchy-scroll.spec.ts new file mode 100644 index 00000000..efe64406 --- /dev/null +++ b/e2e/hierarchy-scroll.spec.ts @@ -0,0 +1,131 @@ +import { test, expect, type Page } from '@playwright/test'; + +/** + * The event hierarchy holds its place when you click something in it. + * + * Curators, re-testing #137: "no jumping in the search and the analysis table + * and none in the upper section of event hierarchy, but pathways near the bottom + * of the hierarchy (e.g. subevents) still jump." + * + * They were right, and the cause was not the revealing. Rebuilding the tree + * starts by emptying it -- a workaround for an Angular Material bug where nested + * children otherwise do not render -- and emptying it destroys every row, so the + * container collapses and the browser resets the scroll to the top. Measured on + * a deep sub-event: 274px, then 0, then 6. Expansion state was already carried + * across that rebuild; the scroll position was not. + * + * Both halves matter, so both are checked: clicking a row you can already see + * must not move the tree, and selecting one you cannot see must still bring it + * into view. + */ + +const BOOT_TIMEOUT = 90_000; +// Intrinsic Pathway for Apoptosis, with its ancestors expanded. +const DEEP = '/PathwayBrowser/R-HSA-109606?path=R-HSA-5357801,R-HSA-109581'; +const TREE = 'cr-event-hierarchy'; +const SCROLLER = '#events-container'; + +async function openDeepHierarchy(page: Page) { + await page.goto(DEEP); + await page.waitForSelector('#cytoscape canvas', { timeout: BOOT_TIMEOUT }); + await page.waitForSelector(`${TREE} [role="treeitem"]`, { timeout: BOOT_TIMEOUT }); + await page.waitForTimeout(3000); + + // Open whatever is still closed, so there is something below the fold. + for (let round = 0; round < 3; round++) { + const toggles = page.locator(`${TREE} [role="treeitem"] button`); + const count = await toggles.count(); + let opened = 0; + for (let index = 0; index < count && opened < 6; index++) { + const toggle = toggles.nth(index); + if ((await toggle.getAttribute('aria-expanded').catch(() => null)) === 'false') { + await toggle.click({ timeout: 5000 }).catch(() => undefined); + opened++; + await page.waitForTimeout(250); + } + } + if (!opened) break; + } + await page.waitForTimeout(1000); +} + +/** Scroll to the bottom and mark a row that is comfortably in view there. */ +async function markVisibleRowNearBottom(page: Page) { + return page.evaluate(async (selector) => { + const scroller = document.querySelector(selector) as HTMLElement | null; + if (!scroller) return null; + scroller.scrollTop = scroller.scrollHeight; + await new Promise((resolve) => setTimeout(resolve, 400)); + + const box = scroller.getBoundingClientRect(); + const rows = [...document.querySelectorAll('cr-event-hierarchy [role="treeitem"]')].filter( + (row) => { + const rect = row.getBoundingClientRect(); + return rect.top > box.top + 20 && rect.bottom < box.bottom - 20; + } + ); + const pick = rows[Math.floor(rows.length / 2)]; + if (!pick) return null; + pick.setAttribute('data-pick', '1'); + return { scrollTop: Math.round(scroller.scrollTop), label: (pick.textContent ?? '').trim() }; + }, SCROLLER); +} + +const scrollTop = (page: Page) => + page.evaluate( + (selector) => Math.round((document.querySelector(selector) as HTMLElement).scrollTop), + SCROLLER + ); + +test.describe('Event hierarchy scrolling', () => { + test.describe.configure({ timeout: 4 * 60 * 1000 }); + + test('does not move when you click a row you can already see', async ({ page }) => { + await openDeepHierarchy(page); + + const marked = await markVisibleRowNearBottom(page); + test.skip(!marked || marked.scrollTop === 0, 'the hierarchy here does not scroll'); + + const before = await scrollTop(page); + await page.locator(`${TREE} [data-pick="1"]`).click(); + // Long enough for the rebuild, the expansion restore and any revealing. + await page.waitForTimeout(2500); + const after = await scrollTop(page); + + // This was 274 -> 0 -> 6. + expect(Math.abs(after - before), `before ${before}, after ${after}`).toBeLessThan(24); + }); + + test('still brings a row you cannot see into view', async ({ page }) => { + await openDeepHierarchy(page); + + const target = await page.evaluate(async (selector) => { + const scroller = document.querySelector(selector) as HTMLElement; + scroller.scrollTop = scroller.scrollHeight; + await new Promise((resolve) => setTimeout(resolve, 400)); + const box = scroller.getBoundingClientRect(); + const above = [...document.querySelectorAll('cr-event-hierarchy [role="treeitem"]')].filter( + (row) => row.getBoundingClientRect().bottom < box.top + ); + const pick = above[Math.floor(above.length / 2)]; + if (!pick) return null; + pick.setAttribute('data-target', '1'); + return { scrollTop: Math.round(scroller.scrollTop), above: above.length }; + }, SCROLLER); + + test.skip(!target || target.above === 0, 'nothing is scrolled out of view here'); + + const before = await scrollTop(page); + // It is off-screen, so dispatch the click rather than asking Playwright to + // scroll it into view first -- that would defeat the test. + await page.evaluate(() => + document + .querySelector('[data-target="1"]') + ?.dispatchEvent(new MouseEvent('click', { bubbles: true })) + ); + await page.waitForTimeout(2500); + const after = await scrollTop(page); + + expect(after, `it should have scrolled up from ${before}`).toBeLessThan(before - 20); + }); +}); diff --git a/projects/pathway-browser/src/app/event-hierarchy/event-hierarchy.component.html b/projects/pathway-browser/src/app/event-hierarchy/event-hierarchy.component.html index 26c33c5c..ca5a046c 100644 --- a/projects/pathway-browser/src/app/event-hierarchy/event-hierarchy.component.html +++ b/projects/pathway-browser/src/app/event-hierarchy/event-hierarchy.component.html @@ -37,7 +37,7 @@ -
+
>('eventsContainer'); + treeDataSource = new MatTreeNestedDataSource(); breadcrumbs: Event[] = []; @@ -218,12 +222,29 @@ export class EventHierarchyComponent implements AfterViewInit, OnDestroy { this.eventService.treeData$.pipe(untilDestroyed(this)).subscribe((events) => { // Save expanded node stIds before resetting the data source const expandedIds = this.collectExpandedIds(this.treeDataSource.data); + // The workaround below empties the tree, which destroys every row: the + // container collapses to nothing and the browser resets its scroll to the + // top. Clicking a sub-event near the bottom of the hierarchy therefore + // jumped to the top and scrolled back -- measured at 274px, then 0, then + // 6. Expansion state is already carried across this rebuild; the scroll + // position has to be carried the same way. + const scroller = this.eventsContainer()?.nativeElement; + const scrollTop = scroller?.scrollTop ?? 0; // Mat tree has a bug causing children to not be rendered in the UI without first setting the data to null // This is a workaround to add child data to tree and update the view. see details: https://github.com/angular/components/issues/11381 this.treeDataSource.data = []; //todo: check performance issue this.treeDataSource.data = events as Event[]; // Restore expansion state this.restoreExpandedIds(events as Event[], expandedIds); + if (scroller && scrollTop > 0) { + scroller.scrollTop = scrollTop; + // Again once the rows have been laid out: the height is not final until + // the restored branches have rendered, and the browser clamps a + // scrollTop set against a container that is still short. + requestAnimationFrame(() => { + if (scroller.scrollTop !== scrollTop) scroller.scrollTop = scrollTop; + }); + } this.adjustWidths(); });