diff --git a/lint-baseline.json b/lint-baseline.json index e0ad8d5c..ac58d760 100644 --- a/lint-baseline.json +++ b/lint-baseline.json @@ -1,3 +1,3 @@ { - "warnings": 654 + "warnings": 653 } diff --git a/projects/pathway-browser/src/app/event-hierarchy/event-hierarchy.component.ts b/projects/pathway-browser/src/app/event-hierarchy/event-hierarchy.component.ts index 22182e83..df9cf394 100644 --- a/projects/pathway-browser/src/app/event-hierarchy/event-hierarchy.component.ts +++ b/projects/pathway-browser/src/app/event-hierarchy/event-hierarchy.component.ts @@ -241,6 +241,12 @@ export class EventHierarchyComponent implements AfterViewInit, OnDestroy { // 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. + // + // One frame, and it has to stay one: RevealDirective brings a selected + // row into view on its *second* frame, so restoring first is what lets + // its `block: 'nearest'` see the reader's real position and decide the + // row is already visible. Restore on a later frame than reveal and every + // click scrolls twice again. requestAnimationFrame(() => { if (scroller.scrollTop !== scrollTop) scroller.scrollTop = scrollTop; }); diff --git a/projects/pathway-browser/src/app/services/url-state.service.ts b/projects/pathway-browser/src/app/services/url-state.service.ts index 91284e58..27e1b82f 100644 --- a/projects/pathway-browser/src/app/services/url-state.service.ts +++ b/projects/pathway-browser/src/app/services/url-state.service.ts @@ -1,4 +1,4 @@ -import { effect, inject, Injectable, signal, WritableSignal } from '@angular/core'; +import { effect, inject, Injectable, signal, untracked, WritableSignal } from '@angular/core'; import { ActivatedRoute, NavigationEnd, NavigationExtras, Params, Router } from '@angular/router'; import { catchError, filter, firstValueFrom, map, of, switchMap } from 'rxjs'; import { isArray, isNumber } from 'lodash'; @@ -273,35 +273,25 @@ export class UrlStateService implements State { })().catch((error) => console.error('Could not apply URL parameters', error)); }); effect(() => { - const queryParams = {} as any; - for (const key in this.values) { - const param = this.values[key as keyof State]; - let paramValue = param(); - if ( - paramValue === undefined || - paramValue === null || - (isArray(paramValue) && paramValue.length === 0) || - paramValue === param.initialValue - ) - continue; - if (typeof paramValue === 'string') paramValue = paramValue.replaceAll(' ', '__'); - queryParams[key] = isArray(paramValue) ? paramValue.join(';') : paramValue; - } - // console.log('Updating URL from state', queryParams) - if (this.router.url.includes('content') || this.router.url.includes('query')) { - // console.log('In content or search route, not navigating on state change'); - return; - } - // Settling is not a step the reader took, so it replaces rather than - // adds. Opening a pathway used to write ?tab=info and then ?tab=details, - // two entries for a choice nobody made -- so pressing Back changed the - // tab twice before it left the page. - const settling = this.settling; - this.settling = false; + const queryParams = this.currentQueryParams(); + if (this.router.url.includes('content') || this.router.url.includes('query')) return; + + // Settling is not a step the reader took, so it replaces rather than adds: + // opening a pathway wrote ?tab=info and then ?tab=details, two entries for a + // choice nobody made, so Back changed the tab twice before it left. + // + // Decided from the values rather than from a flag. A flag raised by + // settle() and read here outlives its turn whenever this does not run -- + // the early return above, or a default that was already the current value, + // so no signal changed at all -- and the reader's next write, a real one, + // would then quietly replace their history instead of adding to it. + const settled = this.settledParams; + this.settledParams = null; + void this.navigateTo(this.pathwayId() ?? null, { queryParams, preserveFragment: !this.carriesLegacyPathway(), - replaceUrl: settling, + replaceUrl: settled !== null && settled === JSON.stringify(queryParams), }); }); } @@ -330,19 +320,45 @@ export class UrlStateService implements State { return FRAGMENT_PATTERN.test(this.route.snapshot.fragment ?? ''); } - /** Whether the URL write now pending is the app settling its own defaults. */ - private settling = false; + /** The params settle() last wrote, so the writer can recognise its own work. */ + private settledParams: string | null = null; /** - * Make a state change that the reader did not ask for. + * Make a state change the reader did not ask for. * * The URL still has to say what is on screen -- a link has to be shareable -- * but writing a default nobody chose should not cost them a press of Back. * Choosing a tab pushes; being given one replaces. */ settle(change: () => void): void { - this.settling = true; change(); + // What the URL should now say. The writer replaces only if it is about to + // write exactly this, which is what keeps the decision out of a flag. + // + // Untracked, and that is the whole of it: settle() is called from inside an + // effect, so reading every param here made that effect depend on every + // param. Choosing a tab then re-ran the defaulting effect, which set the + // tab back to its default -- the reader's click did nothing at all. + this.settledParams = JSON.stringify(untracked(() => this.currentQueryParams())); + } + + /** The query params the current state should put in the URL. */ + private currentQueryParams(): Record { + const queryParams: Record = {}; + for (const key in this.values) { + const param = this.values[key as keyof State]; + let paramValue: unknown = param(); + if ( + paramValue === undefined || + paramValue === null || + (isArray(paramValue) && paramValue.length === 0) || + paramValue === param.initialValue + ) + continue; + if (typeof paramValue === 'string') paramValue = paramValue.replaceAll(' ', '__'); + queryParams[key] = isArray(paramValue) ? paramValue.join(';') : paramValue; + } + return queryParams; } navigateTo(pathwayId: string | null, extras: NavigationExtras = {}): Promise { diff --git a/projects/pathway-browser/src/app/utils/reveal.directive.ts b/projects/pathway-browser/src/app/utils/reveal.directive.ts index b4775c9b..ec3e1dc2 100644 --- a/projects/pathway-browser/src/app/utils/reveal.directive.ts +++ b/projects/pathway-browser/src/app/utils/reveal.directive.ts @@ -47,6 +47,13 @@ export class RevealDirective { // table that is still expanding rows, or paging to a different page, moves // after the first frame. Revealing then left it 28px past the edge of its // container. The second frame is after that layout has been painted. + // + // It is also what keeps the event hierarchy still. That tree rebuilds + // itself from scratch on every click and restores the reader's scroll + // position on frame one; arriving on frame two means `nearest` below is + // comparing against where the reader actually was, and so does nothing + // when the row was already on screen. Cut this to one frame and clicking + // a visible row jumps the hierarchy again. let frame = requestAnimationFrame(() => { frame = requestAnimationFrame(() => { this.element.nativeElement.scrollIntoView({