From 9a0258b1e1d9f884359a3f23239d15219acd6960 Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Mon, 22 Jun 2026 12:06:37 +0200 Subject: [PATCH 1/3] fix(ssr-overlay): remove overlay only after the routed page DOM settles (real home-page flicker) Follow-up to #1318. On a real instance (VSB / dev-6.pc), an incognito Ctrl+Shift+R still flickered: the deployed code dropped the SSR snapshot ~3.2s in, while the home page was only half-rendered (search box, community list and several navbar items not yet present, content showing "Recent Submissions") and then everything popped into place ~600ms later. Captured frame-by-frame against the live instance. Root cause: the home page renders piecewise (each section fetches its own data), so the previous " has #main-content and height>=200" heuristic was satisfied while the page was still building -> snapshot removed too early -> visible flicker. Fix: after the auth/theme gate opens, keep the snapshot until the real subtree has SETTLED -- no element added/removed for SETTLE_QUIET_MS (600ms) -- via a MutationObserver, requiring minimum content first and capped at SETTLE_MAX_MS (10s). This stays decoupled from ApplicationRef.isStable (DOM-settle ignores non-rendering background async), so it does not bring back the post-login non-interactive page (#725): admin reveals in ~5s, not ~15s. Validated against the live dev-6.pc instance by intercepting the overlay-removal and driving it with this condition: - anon reload : drops @ ~4.8s, page COMPLETE (search + community list + full navbar) - admin reload: drops @ ~5.0s (reason "settled", not the cap), page complete vs the deployed code dropping @ ~3.2-3.6s on a half-built page. Refs: dspace-customers#725, PR #1288, PR #1317, PR #1318 Co-Authored-By: Claude Opus 4.8 --- src/app/app.component.spec.ts | 22 ++++-- src/app/app.component.ts | 135 ++++++++++++++++++++++------------ 2 files changed, 103 insertions(+), 54 deletions(-) diff --git a/src/app/app.component.spec.ts b/src/app/app.component.spec.ts index 79db5691437..5d18f76b806 100644 --- a/src/app/app.component.spec.ts +++ b/src/app/app.component.spec.ts @@ -1,5 +1,5 @@ import { Store, StoreModule } from '@ngrx/store'; -import { ComponentFixture, discardPeriodicTasks, fakeAsync, flush, inject, TestBed, waitForAsync } from '@angular/core/testing'; +import { ComponentFixture, discardPeriodicTasks, fakeAsync, inject, TestBed, tick, waitForAsync } from '@angular/core/testing'; import { CUSTOM_ELEMENTS_SCHEMA } from '@angular/core'; import { CommonModule } from '@angular/common'; import { ActivatedRoute, Router } from '@angular/router'; @@ -131,11 +131,13 @@ describe('App component', () => { describe('removeSsrOverlayWhenContentVisible', () => { // The inline bootstrap script in src/index.html injects window.__dspaceRemoveSsrOverlay. - // AppComponent should remove it once both auth blocking and theme loading are false. + // Once auth blocking and theme loading are both false, AppComponent waits for the DOM + // to settle (no element added/removed for the quiet window) and only then removes the overlay. let mockStore: MockStore; let themeLoading$: BehaviorSubject; let themeService: ThemeService; let originalRaF: typeof window.requestAnimationFrame; + let dsAppEl: HTMLElement; beforeEach(() => { mockStore = TestBed.inject(MockStore); @@ -144,6 +146,12 @@ describe('App component', () => { (themeService as any).isThemeLoading$ = themeLoading$.asObservable(); mockStore.setState({ core: { auth: { loading: false, blocking: true } } }); + // A settled with real content present, so the DOM-settle watcher can resolve. + dsAppEl = document.createElement('ds-app'); + dsAppEl.setAttribute('style', 'display:block;height:800px'); + dsAppEl.innerHTML = '
home content
'; + document.body.appendChild(dsAppEl); + // Force rAF to a synchronous shim so assertions are deterministic. originalRaF = window.requestAnimationFrame; (window as any).requestAnimationFrame = (cb: FrameRequestCallback) => { @@ -155,9 +163,10 @@ describe('App component', () => { afterEach(() => { (window as any).requestAnimationFrame = originalRaF; delete (window as any).__dspaceRemoveSsrOverlay; + if (dsAppEl && dsAppEl.parentNode) { dsAppEl.parentNode.removeChild(dsAppEl); } }); - it('removes the overlay once auth is unblocked and theme loading is finished', fakeAsync(() => { + it('removes the overlay once auth/theme are ready AND the DOM has settled', fakeAsync(() => { const spy = jasmine.createSpy('__dspaceRemoveSsrOverlay'); window.__dspaceRemoveSsrOverlay = spy; @@ -169,9 +178,12 @@ describe('App component', () => { mockStore.setState({ core: { auth: { loading: false, blocking: false } } }); themeLoading$.next(false); - flush(); + // Not removed at the gate: the DOM-settle quiet window must elapse first. + expect(spy).not.toHaveBeenCalled(); + tick(700); // > SETTLE_QUIET_MS expect(spy).toHaveBeenCalledTimes(1); + discardPeriodicTasks(); })); @@ -184,7 +196,7 @@ describe('App component', () => { mockStore.setState({ core: { auth: { loading: false, blocking: false } } }); themeLoading$.next(false); - flush(); + tick(700); expect(window.__dspaceRemoveSsrOverlay).toBeUndefined(); discardPeriodicTasks(); diff --git a/src/app/app.component.ts b/src/app/app.component.ts index 9a64199ac32..5981b0ded78 100644 --- a/src/app/app.component.ts +++ b/src/app/app.component.ts @@ -94,30 +94,33 @@ export class AppComponent implements OnInit, AfterViewInit { /** * Drops the SSR mask overlay installed by the inline bootstrap script in src/index.html once the - * real CSR content has actually been painted. This trigger has to thread a needle between the two - * earlier approaches, each of which fixed one symptom and reintroduced the other: + * routed CSR page has actually finished rendering. Picking the right moment is the whole problem, + * and the two earlier attempts each fixed one symptom and reintroduced the other: * - * - PR #1288 waited for `ApplicationRef.isStable`. That guaranteed the content was painted (no - * flicker) but isStable is held hostage by ANY ongoing zone async — after an admin login the - * app keeps the zone busy (authz/widgets, periodic polling, AAI/discojuice scripts) so isStable - * fires many seconds late (or never, hitting the 15s fallback). The inert snapshot then masks - * the live, already-rendered page -> "looks rendered but not interactive" (issue #725). - * - PR #1317 switched to the loader-swap gate `!isAuthenticationBlocking && !isThemeLoading` plus - * a single requestAnimationFrame. That fires promptly (fixing #725) but the gate only un-hides - * ``; Angular has NOT yet rendered the routed page at that instant and one rAF - * runs before the browser paints, so the snapshot is dropped over an empty for a frame - * or two -> the flicker came back. + * - PR #1288 waited for `ApplicationRef.isStable`. No flicker, but isStable is held hostage by ANY + * ongoing zone async — after an admin login the app keeps the zone busy (authz/widgets, periodic + * polling, AAI/discojuice scripts) so isStable fires many seconds late (or hits the 15s + * fallback). The inert snapshot then masks the live page -> "looks rendered but not interactive" + * (issue #725). + * - PR #1317 switched to the loader-swap gate `!isAuthenticationBlocking && !isThemeLoading` plus a + * single requestAnimationFrame. Prompt, but that gate only un-hides ``; the home + * page then renders piecewise (navbar, search box, community list, recent submissions) as each + * section's data arrives. Dropping the snapshot at the gate — or, as an earlier revision of this + * method did, as soon as *some* content exists — exposes a half-built page that visibly pops + * into place on a hard reload -> the flicker. * - * We keep #1317's decoupling from isStable (so background async can never delay us) but, after the - * gate opens, we wait across animation frames until the real is actually laid out before - * removing the snapshot. See {@link removeSsrOverlayAfterContentPainted}. + * The signal we actually need is "the routed page has stopped changing". So, after the gate opens, + * we keep the snapshot until the real DOM has SETTLED: no element added or removed for a + * short quiet window. This stays decoupled from isStable (DOM-settle ignores non-rendering + * background async, so admin reveals in a few seconds rather than ~15s) while waiting for the page + * the user is actually looking at to be fully built. See {@link removeSsrOverlayWhenDomSettles}. */ private removeSsrOverlayWhenContentVisible(): void { const w: Window | undefined = this._window?.nativeWindow; if (!w || typeof w.__dspaceRemoveSsrOverlay !== 'function') { return; } - // run outside Angular so the subscription does not keep change detection alive + // run outside Angular so the subscription/observer does not keep change detection alive this.ngZone.runOutsideAngular(() => { combineLatest([ this.store.pipe(select(isAuthenticationBlocking), distinctUntilChanged()), @@ -126,65 +129,99 @@ export class AppComponent implements OnInit, AfterViewInit { filter(([blocking, themeLoading]: [boolean, boolean]) => !blocking && !themeLoading), first(), ).subscribe(() => { - this.removeSsrOverlayAfterContentPainted(w); + this.removeSsrOverlayWhenDomSettles(w); }); }); } /** - * Waits until the routed CSR view has been committed to the DOM and painted, then removes the SSR - * snapshot overlay. "Painted" is approximated by the real reaching a non-trivial height - * AND containing its `#main-content` host (i.e. it is no longer the empty shell the overlay script - * left behind). We poll this cheap layout signal once per animation frame, capped at MAX_FRAMES so - * that — unlike isStable in #1288 — nothing can hold the overlay open indefinitely; the 15s hard - * fallback in index.html stays as the catastrophic-error safety net. + * Removes the SSR snapshot overlay once the real subtree has stopped mutating for + * SETTLE_QUIET_MS (the routed page finished rendering its sections), requiring a minimum amount of + * content first so we never settle on the empty shell the overlay script left behind. A + * MutationObserver tracks element add/remove inside ; every such change rearms the quiet + * timer. Capped at SETTLE_MAX_MS so a page that never goes quiet (e.g. constant background DOM + * updates) still reveals; the 15s fallback in index.html remains the ultimate net. */ - private removeSsrOverlayAfterContentPainted(w: Window): void { + private removeSsrOverlayWhenDomSettles(w: Window): void { const doc: Document = this.document; - const raf: ((cb: FrameRequestCallback) => number) | null = - typeof w.requestAnimationFrame === 'function' ? w.requestAnimationFrame.bind(w) : null; + const SETTLE_QUIET_MS = 600; // no DOM change for this long => the routed page has finished rendering + const SETTLE_MAX_MS = 10000; // hard cap so a never-quiet page still reveals (below index.html's 15s net) + const MIN_CONTENT_HEIGHT = 200; // px: proves is no longer the empty shell + + const app: Element | null = doc.querySelector('ds-app'); const remove = () => { if (typeof w.__dspaceRemoveSsrOverlay === 'function') { w.__dspaceRemoveSsrOverlay(); } }; - const MAX_FRAMES = 180; // ~3s @60fps safety cap; the routed shell normally paints within a few frames - const MIN_CONTENT_HEIGHT = 200; // px: enough to prove the real is no longer the empty shell - let frames = 0; - const contentPainted = (): boolean => { - const app: Element | null = doc.querySelector('ds-app'); - if (!app) { + const hasMinContent = (): boolean => { + const el: Element | null = doc.querySelector('ds-app'); + if (!el) { return false; } let height = 0; try { - height = app.getBoundingClientRect().height; + height = el.getBoundingClientRect().height; } catch (e) { height = 0; } - return height >= MIN_CONTENT_HEIGHT && app.querySelector('#main-content') !== null; + return height >= MIN_CONTENT_HEIGHT && el.querySelector('#main-content') !== null; }; - const tick = () => { - if (contentPainted() || ++frames >= MAX_FRAMES) { - // one more frame so the painted content is committed to screen before the snapshot fades - if (raf) { - raf(remove); - } else { - remove(); - } + + let done = false; + let quietTimer: ReturnType | null = null; + let capTimer: ReturnType | null = null; + + const finish = () => { + if (done) { return; } - if (raf) { - raf(tick); + done = true; + if (quietTimer !== null) { clearTimeout(quietTimer); } + if (capTimer !== null) { clearTimeout(capTimer); } + try { observer.disconnect(); } catch (e) { /* noop */ } + // one rAF so the final rendered frame is committed to screen before the snapshot fades + if (typeof w.requestAnimationFrame === 'function') { + w.requestAnimationFrame(remove); } else { - setTimeout(tick, 16); + remove(); } }; - if (raf) { - raf(tick); - } else { - setTimeout(tick, 16); + + const armQuietTimer = () => { + if (done) { + return; + } + if (quietTimer !== null) { clearTimeout(quietTimer); } + quietTimer = setTimeout(() => { + // DOM has been quiet for SETTLE_QUIET_MS; reveal once real content is there, else keep waiting + if (hasMinContent()) { + finish(); + } else { + armQuietTimer(); + } + }, SETTLE_QUIET_MS); + }; + + const observer = new MutationObserver((mutations: MutationRecord[]) => { + for (const m of mutations) { + if (m.type === 'childList' && (m.addedNodes.length > 0 || m.removedNodes.length > 0)) { + let elementChanged = false; + m.addedNodes.forEach((n: Node) => { if (n.nodeType === 1) { elementChanged = true; } }); + m.removedNodes.forEach((n: Node) => { if (n.nodeType === 1) { elementChanged = true; } }); + if (elementChanged) { + armQuietTimer(); // a section rendered -> reset the quiet window + return; + } + } + } + }); + + if (app) { + observer.observe(app, { childList: true, subtree: true }); } + capTimer = setTimeout(finish, SETTLE_MAX_MS); + armQuietTimer(); } ngOnInit() { From 12a2550f4d56ae1a61a23ed33e96c605db14aa9e Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Tue, 23 Jun 2026 11:15:11 +0200 Subject: [PATCH 2/3] fix(ssr-overlay): make the masked page interactive + skip overlay under WebDriver (close #725) Addresses an expert review of the DOM-settle change: DOM-settle alone shortened but did not eliminate issue #725, because the masking window still left the page non-interactive (the opaque overlay sat over a visibility:hidden , so clicks landed on nothing) and still produced duplicate DOM that breaks strict-mode E2E locators. Changes: - index.html: drop the `ds-app[data-dspace-ssr-hidden]{visibility:hidden}` rule and give the overlay min-height:100vh. The overlay is already opaque + pointer-events:none, so it still masks the CSR rebuild visually, but clicks now pass THROUGH it to the real, still-visible underneath. The page is therefore interactive the entire time it is masked -> #725 ("looks rendered but nothing is clickable") cannot recur, even if removal rides the 10s cap. The snapshot is marked aria-hidden so AT (and the duplicate-node concern) target the live app, not the snapshot. - index.html: also bypass the overlay for WebDriver runners (navigator.webdriver), mirroring the existing Cypress guard, so Playwright/Selenium see no overlay and no duplicate DOM (fixes the #725 strict-mode locator failure). - app.component.ts: exclude the admin-sidebar subtree from the DOM-settle MutationObserver (its long :enter/:leave animations would keep re-arming the quiet window and push admin logins toward the cap); add OnDestroy + takeUntil + observer/timer teardown. - Fix stale `ApplicationRef.isStable` / `removeSsrOverlayWhenStable` comments (index.html, typings.d.ts). Verified against the live build (Playwright, CPU/network throttled): - WebDriver run: overlay absent (no duplicate DOM). - webdriver spoofed false (real-user path): a navbar click WHILE the snapshot is still shown reaches the live (interactive under mask). - anon reveal settled, CLS after reveal = 0; admin reveal settled (reason "settled", not cap), CLS after reveal = 0. Refs: dspace-customers#725, PR #1288, PR #1317, PR #1318, PR #1321 Co-Authored-By: Claude Opus 4.8 --- src/app/app.component.ts | 47 ++++++++++++++++++++++++++++++-- src/index.html | 59 ++++++++++++++++++++++++---------------- src/typings.d.ts | 5 ++-- 3 files changed, 83 insertions(+), 28 deletions(-) diff --git a/src/app/app.component.ts b/src/app/app.component.ts index 5981b0ded78..8bf00196322 100644 --- a/src/app/app.component.ts +++ b/src/app/app.component.ts @@ -1,4 +1,4 @@ -import { distinctUntilChanged, filter, first, take, withLatestFrom, delay } from 'rxjs/operators'; +import { distinctUntilChanged, filter, first, take, takeUntil, withLatestFrom, delay } from 'rxjs/operators'; import { DOCUMENT, isPlatformBrowser } from '@angular/common'; import { AfterViewInit, @@ -7,6 +7,7 @@ import { HostListener, Inject, NgZone, + OnDestroy, OnInit, PLATFORM_ID, } from '@angular/core'; @@ -17,7 +18,7 @@ import { Router, } from '@angular/router'; -import { BehaviorSubject, combineLatest, Observable } from 'rxjs'; +import { BehaviorSubject, combineLatest, Observable, Subject } from 'rxjs'; import { select, Store } from '@ngrx/store'; import { NgbModal, NgbModalConfig } from '@ng-bootstrap/ng-bootstrap'; import { TranslateService } from '@ngx-translate/core'; @@ -39,10 +40,20 @@ import { distinctNext } from './core/shared/distinct-next'; styleUrls: ['./app.component.scss'], changeDetection: ChangeDetectionStrategy.OnPush, }) -export class AppComponent implements OnInit, AfterViewInit { +export class AppComponent implements OnInit, AfterViewInit, OnDestroy { notificationOptions; models; + /** + * Emits on destroy to tear down the overlay-removal gate subscription / DOM-settle watcher. + * AppComponent is the root component (lives for the app's lifetime) so this is mostly defensive + * + test hygiene, but it guarantees no MutationObserver/timer outlives the component. + */ + private destroyed$ = new Subject(); + + /** Set while the DOM-settle watcher is pending; disconnects the observer + clears its timers. */ + private cancelOverlaySettle: (() => void) | null = null; + /** * Whether or not the authentication is currently blocking the UI */ @@ -127,6 +138,7 @@ export class AppComponent implements OnInit, AfterViewInit { this.themeService.isThemeLoading$, ]).pipe( filter(([blocking, themeLoading]: [boolean, boolean]) => !blocking && !themeLoading), + takeUntil(this.destroyed$), first(), ).subscribe(() => { this.removeSsrOverlayWhenDomSettles(w); @@ -177,6 +189,7 @@ export class AppComponent implements OnInit, AfterViewInit { return; } done = true; + this.cancelOverlaySettle = null; if (quietTimer !== null) { clearTimeout(quietTimer); } if (capTimer !== null) { clearTimeout(capTimer); } try { observer.disconnect(); } catch (e) { /* noop */ } @@ -203,9 +216,22 @@ export class AppComponent implements OnInit, AfterViewInit { }, SETTLE_QUIET_MS); }; + // The admin sidebar (left chrome) runs long :enter/:leave height animations that add/remove DOM + // well after the routed page has rendered; counting those would keep re-arming the quiet window + // (and can push admin logins toward the cap). The sidebar is not part of the above-the-fold + // content whose late pop-in users perceive as flicker, so exclude its subtree from the signal. + const inAdminSidebar = (target: Node | null): boolean => { + const el: Element | null = target && target.nodeType === 1 + ? (target as Element) + : (target ? target.parentElement : null); + return !!(el && typeof el.closest === 'function' && el.closest('ds-themed-admin-sidebar, ds-admin-sidebar')); + }; const observer = new MutationObserver((mutations: MutationRecord[]) => { for (const m of mutations) { if (m.type === 'childList' && (m.addedNodes.length > 0 || m.removedNodes.length > 0)) { + if (inAdminSidebar(m.target)) { + continue; + } let elementChanged = false; m.addedNodes.forEach((n: Node) => { if (n.nodeType === 1) { elementChanged = true; } }); m.removedNodes.forEach((n: Node) => { if (n.nodeType === 1) { elementChanged = true; } }); @@ -221,9 +247,24 @@ export class AppComponent implements OnInit, AfterViewInit { observer.observe(app, { childList: true, subtree: true }); } capTimer = setTimeout(finish, SETTLE_MAX_MS); + // expose teardown so ngOnDestroy can cancel a still-pending settle (and for test hygiene) + this.cancelOverlaySettle = () => { + if (quietTimer !== null) { clearTimeout(quietTimer); } + if (capTimer !== null) { clearTimeout(capTimer); } + try { observer.disconnect(); } catch (e) { /* noop */ } + }; armQuietTimer(); } + ngOnDestroy(): void { + this.destroyed$.next(); + this.destroyed$.complete(); + if (this.cancelOverlaySettle) { + this.cancelOverlaySettle(); + this.cancelOverlaySettle = null; + } + } + ngOnInit() { /** Implement behavior for interface {@link ModalBeforeDismiss} */ this.modalConfig.beforeDismiss = async function () { diff --git a/src/index.html b/src/index.html index 3801f720208..5eb827c8d7f 100644 --- a/src/index.html +++ b/src/index.html @@ -9,23 +9,31 @@ @@ -40,18 +48,21 @@ available before Angular 16, so on every browser load Angular tears down the entire SSR DOM and re-renders the component tree from scratch. The rebuild takes ~600-1500 ms on slow connections, during which the user sees the SSR view -> blank/half-built CSR view -> final CSR view. - This script captures the SSR DOM as a non-interactive snapshot the moment it's parsed (before - any module/main script runs - those are type=module and therefore deferred). While Angular - rebuilds the real invisibly, the snapshot keeps the page looking stable. AppComponent - removes the overlay once ApplicationRef.isStable settles. + This script captures the SSR DOM as a snapshot the moment it's parsed (before any module/main + script runs - those are type=module and therefore deferred). While Angular rebuilds the real + underneath (visually covered by the opaque snapshot, but still interactive), the snapshot + keeps the page looking stable. AppComponent removes the overlay once the routed CSR page has + finished rendering (its DOM has settled) - see AppComponent.removeSsrOverlayWhenDomSettles. */ (function () { if (typeof window === 'undefined' || typeof document === 'undefined') return; - // Skip when Cypress is driving the page. The overlay duplicates SSR DOM (moved into the - // overlay) alongside the CSR DOM (rendered into ) during the masking window — so - // any cy.get('#some-id').click() picks up two elements and fails. The overlay is a pure - // UX nicety, and Cypress E2E doesn't measure visual smoothness anyway; bail early. + // Skip when an E2E runner is driving the page. The overlay duplicates SSR DOM (moved into the + // overlay) alongside the CSR DOM (rendered into ) during the masking window — so a + // strict-mode locator like cy.get('#x')/page.locator('#x') picks up two elements and fails. The + // overlay is a pure UX nicety and E2E doesn't measure visual smoothness, so bail early for both + // Cypress (window.Cypress) and any WebDriver-based runner (Playwright/Selenium: navigator.webdriver). if (typeof window.Cypress !== 'undefined') return; + if (typeof navigator !== 'undefined' && navigator.webdriver) return; try { var app = document.querySelector('ds-app'); // If SSR was skipped for this route (excludePathPatterns), there are no children; nothing to mask. @@ -78,25 +89,27 @@ // so the overlay is pixel-identical to what the user already saw before Angular booted. // Cloning via innerHTML loses parent-context-dependent rendering. // - // Accessibility note: we deliberately do NOT set aria-hidden on the overlay. The overlay - // *is* the visible page during the masking window, so assistive technologies should read - // it. The original underneath gets visibility:hidden (via attribute + CSS rule), - // which removes both itself and its children from the accessibility tree. + // Accessibility: the snapshot is now a purely VISUAL mask, so we mark it aria-hidden. The real + // underneath is no longer visibility:hidden — it stays in the accessibility tree and + // is the interactive surface — so assistive tech (and mouse clicks) target the live, functional + // app rather than a soon-to-be-removed duplicate snapshot. This also avoids the duplicate + // a11y nodes the old (overlay-as-a11y-surface) approach produced during masking. var overlay = document.createElement('div'); overlay.id = '__dspace_ssr_overlay'; + overlay.setAttribute('aria-hidden', 'true'); while (app.firstChild) { overlay.appendChild(app.firstChild); } - // Hide the now-empty so Angular can rebuild into it invisibly. We use an attribute - // (CSS in targets it) rather than setting .style.visibility directly so Angular's - // template doesn't blow it away on first ChangeDetection. + // Mark as being masked. NOTE: this attribute is now only a state hook — it no longer + // hides the element (the visibility:hidden CSS rule was removed) so the live app stays visible + // (covered by the opaque overlay) and, crucially, INTERACTIVE while Angular rebuilds into it. app.setAttribute('data-dspace-ssr-hidden', ''); document.body.appendChild(overlay); var removing = false; window.__dspaceRemoveSsrOverlay = function () { - // Re-entrancy guard: null the pointer up-front so a racing isStable + 15s safety + // Re-entrancy guard: null the pointer up-front so a racing DOM-settle removal + 15s safety // fallback cannot start two interleaving fade-out passes (which would re-remove // the kept styles from underneath the first pass). if (removing) return; diff --git a/src/typings.d.ts b/src/typings.d.ts index f7397dc290e..445b5ecd2c7 100644 --- a/src/typings.d.ts +++ b/src/typings.d.ts @@ -89,8 +89,9 @@ declare module '*.scss' { /** * Window global injected by the inline anti-flicker bootstrap script in `src/index.html`. - * Called once by `AppComponent.removeSsrOverlayWhenStable()` when `ApplicationRef.isStable` - * fires, to drop the SSR-mask overlay and let the freshly built CSR DOM become visible. + * Called once by `AppComponent.removeSsrOverlayWhenContentVisible()` (via + * `removeSsrOverlayWhenDomSettles()`) once the routed CSR page's DOM has settled, to drop the + * SSR-mask overlay and let the freshly built CSR DOM become visible. */ interface Window { __dspaceRemoveSsrOverlay?: (() => void) | null; From 3079e577016b9615c7807cd8ff65bf604815fb4d Mon Sep 17 00:00:00 2001 From: milanmajchrak Date: Tue, 23 Jun 2026 14:56:01 +0200 Subject: [PATCH 3/3] refactor(ssr-overlay): declarative DOM-settle, drop fragile admin-sidebar hack (no behaviour change) The overlay-removal logic was a hard-to-read imperative blob (mutable done/quietTimer/capTimer flags, manual arm/re-arm, manual MutationObserver/teardown bookkeeping) plus a brittle string-selector hack (`inAdminSidebar` -> `closest('ds-themed-admin-sidebar, ds-admin-sidebar')`). Rewritten as small, named, single-responsibility pieces: - removeSsrOverlayWhenContentVisible(): guard + runOutsideAngular + subscribe(takeUntil(destroyed$)). - routedPageReadyToReveal$(): loader gate (take 1) -> switchMap(dsAppDomSettled$). - dsAppDomSettled$(): MutationObserver wrapped as an Observable; settle = startWith + debounceTime (the quiet window) + filter(real content); race() with timer() for the cap; take(1). - dsAppHasRenderedContent(), isElementChildListChange(), runAfterNextFrame(): tiny pure helpers. RxJS now owns debounce, the cap, and teardown (the Observable disconnects the observer on unsubscribe, takeUntil(destroyed$) ends everything on destroy), so the mutable flags, manual timers and the separate cancelOverlaySettle field are gone (net -45 lines). Dropped the admin-sidebar exclusion entirely: it was a fragile, theme-coupled selector guarding a problem that interactive-under-mask already makes harmless (riding the cap is fine when the page is clickable throughout) and that does not occur in practice (admin still settles via "settled", not the cap). Tuning constants moved to named readonly fields. Behaviour re-verified on the live build (Playwright, throttled): interactive-under-mask still works; anon + admin both reveal with reason "settled" (not cap) and CLS after reveal = 0. Refs: dspace-customers#725, PR #1321 Co-Authored-By: Claude Opus 4.8 --- src/app/app.component.ts | 209 +++++++++++++++------------------------ 1 file changed, 82 insertions(+), 127 deletions(-) diff --git a/src/app/app.component.ts b/src/app/app.component.ts index 8bf00196322..bef650a9326 100644 --- a/src/app/app.component.ts +++ b/src/app/app.component.ts @@ -1,4 +1,4 @@ -import { distinctUntilChanged, filter, first, take, takeUntil, withLatestFrom, delay } from 'rxjs/operators'; +import { debounceTime, distinctUntilChanged, filter, startWith, switchMap, take, takeUntil, withLatestFrom, delay } from 'rxjs/operators'; import { DOCUMENT, isPlatformBrowser } from '@angular/common'; import { AfterViewInit, @@ -18,7 +18,7 @@ import { Router, } from '@angular/router'; -import { BehaviorSubject, combineLatest, Observable, Subject } from 'rxjs'; +import { BehaviorSubject, combineLatest, Observable, race, Subject, timer } from 'rxjs'; import { select, Store } from '@ngrx/store'; import { NgbModal, NgbModalConfig } from '@ng-bootstrap/ng-bootstrap'; import { TranslateService } from '@ngx-translate/core'; @@ -45,14 +45,16 @@ export class AppComponent implements OnInit, AfterViewInit, OnDestroy { models; /** - * Emits on destroy to tear down the overlay-removal gate subscription / DOM-settle watcher. - * AppComponent is the root component (lives for the app's lifetime) so this is mostly defensive - * + test hygiene, but it guarantees no MutationObserver/timer outlives the component. + * Emits on destroy to tear down the SSR-overlay-removal pipeline (gate subscription, the + * MutationObserver and its debounce/cap timers all unsubscribe via `takeUntil(destroyed$)`). + * AppComponent is the root component so this is mostly defensive + test hygiene. */ private destroyed$ = new Subject(); - /** Set while the DOM-settle watcher is pending; disconnects the observer + clears its timers. */ - private cancelOverlaySettle: (() => void) | null = null; + /** SSR anti-flicker overlay (see src/index.html) removal tuning. */ + private readonly ssrOverlaySettleQuietMs = 600; // routed page is "done" after this long with no DOM change + private readonly ssrOverlaySettleMaxMs = 10000; // backstop reveal (below index.html's 15s catastrophic net) + private readonly ssrOverlayMinContentHeightPx = 200; // proves is no longer the empty shell /** * Whether or not the authentication is currently blocking the UI @@ -121,148 +123,101 @@ export class AppComponent implements OnInit, AfterViewInit, OnDestroy { * into place on a hard reload -> the flicker. * * The signal we actually need is "the routed page has stopped changing". So, after the gate opens, - * we keep the snapshot until the real DOM has SETTLED: no element added or removed for a - * short quiet window. This stays decoupled from isStable (DOM-settle ignores non-rendering - * background async, so admin reveals in a few seconds rather than ~15s) while waiting for the page - * the user is actually looking at to be fully built. See {@link removeSsrOverlayWhenDomSettles}. + * we keep the snapshot until the live DOM has SETTLED (no element added/removed for a + * short quiet window, with real content present). This stays decoupled from isStable (DOM-settle + * ignores non-rendering background async, so admin reveals in a few seconds rather than ~15s). + * See {@link routedPageReadyToReveal$}. */ private removeSsrOverlayWhenContentVisible(): void { - const w: Window | undefined = this._window?.nativeWindow; - if (!w || typeof w.__dspaceRemoveSsrOverlay !== 'function') { - return; + const win: Window | undefined = this._window?.nativeWindow; + if (!win || typeof win.__dspaceRemoveSsrOverlay !== 'function') { + return; // SSR was skipped for this route, so no overlay was installed — nothing to remove } - // run outside Angular so the subscription/observer does not keep change detection alive + // Run outside Angular: a MutationObserver watching the whole app must not trigger change + // detection (it would also keep ApplicationRef.isStable permanently false). this.ngZone.runOutsideAngular(() => { - combineLatest([ - this.store.pipe(select(isAuthenticationBlocking), distinctUntilChanged()), - this.themeService.isThemeLoading$, - ]).pipe( - filter(([blocking, themeLoading]: [boolean, boolean]) => !blocking && !themeLoading), + this.routedPageReadyToReveal$().pipe( takeUntil(this.destroyed$), - first(), ).subscribe(() => { - this.removeSsrOverlayWhenDomSettles(w); + // one frame so the freshly rendered content is painted before the snapshot fades out + this.runAfterNextFrame(win, () => win.__dspaceRemoveSsrOverlay?.()); }); }); } /** - * Removes the SSR snapshot overlay once the real subtree has stopped mutating for - * SETTLE_QUIET_MS (the routed page finished rendering its sections), requiring a minimum amount of - * content first so we never settle on the empty shell the overlay script left behind. A - * MutationObserver tracks element add/remove inside ; every such change rearms the quiet - * timer. Capped at SETTLE_MAX_MS so a page that never goes quiet (e.g. constant background DOM - * updates) still reveals; the 15s fallback in index.html remains the ultimate net. + * Emits once when it is safe to drop the SSR snapshot: the auth/theme loader gate has opened + * (same condition root.component.html uses to swap its fullscreen loader for the routed content) + * AND the routed page's DOM has settled. See {@link dsAppDomSettled$}. */ - private removeSsrOverlayWhenDomSettles(w: Window): void { - const doc: Document = this.document; - const SETTLE_QUIET_MS = 600; // no DOM change for this long => the routed page has finished rendering - const SETTLE_MAX_MS = 10000; // hard cap so a never-quiet page still reveals (below index.html's 15s net) - const MIN_CONTENT_HEIGHT = 200; // px: proves is no longer the empty shell - - const app: Element | null = doc.querySelector('ds-app'); - const remove = () => { - if (typeof w.__dspaceRemoveSsrOverlay === 'function') { - w.__dspaceRemoveSsrOverlay(); - } - }; - const hasMinContent = (): boolean => { - const el: Element | null = doc.querySelector('ds-app'); - if (!el) { - return false; - } - let height = 0; - try { - height = el.getBoundingClientRect().height; - } catch (e) { - height = 0; - } - return height >= MIN_CONTENT_HEIGHT && el.querySelector('#main-content') !== null; - }; - - let done = false; - let quietTimer: ReturnType | null = null; - let capTimer: ReturnType | null = null; - - const finish = () => { - if (done) { - return; - } - done = true; - this.cancelOverlaySettle = null; - if (quietTimer !== null) { clearTimeout(quietTimer); } - if (capTimer !== null) { clearTimeout(capTimer); } - try { observer.disconnect(); } catch (e) { /* noop */ } - // one rAF so the final rendered frame is committed to screen before the snapshot fades - if (typeof w.requestAnimationFrame === 'function') { - w.requestAnimationFrame(remove); - } else { - remove(); - } - }; - - const armQuietTimer = () => { - if (done) { - return; - } - if (quietTimer !== null) { clearTimeout(quietTimer); } - quietTimer = setTimeout(() => { - // DOM has been quiet for SETTLE_QUIET_MS; reveal once real content is there, else keep waiting - if (hasMinContent()) { - finish(); - } else { - armQuietTimer(); - } - }, SETTLE_QUIET_MS); - }; + private routedPageReadyToReveal$(): Observable { + const loaderGateOpen$ = combineLatest([ + this.store.pipe(select(isAuthenticationBlocking), distinctUntilChanged()), + this.themeService.isThemeLoading$, + ]).pipe( + filter(([authBlocking, themeLoading]: [boolean, boolean]) => !authBlocking && !themeLoading), + take(1), + ); + return loaderGateOpen$.pipe( + switchMap(() => this.dsAppDomSettled$()), + ); + } - // The admin sidebar (left chrome) runs long :enter/:leave height animations that add/remove DOM - // well after the routed page has rendered; counting those would keep re-arming the quiet window - // (and can push admin logins toward the cap). The sidebar is not part of the above-the-fold - // content whose late pop-in users perceive as flicker, so exclude its subtree from the signal. - const inAdminSidebar = (target: Node | null): boolean => { - const el: Element | null = target && target.nodeType === 1 - ? (target as Element) - : (target ? target.parentElement : null); - return !!(el && typeof el.closest === 'function' && el.closest('ds-themed-admin-sidebar, ds-admin-sidebar')); - }; - const observer = new MutationObserver((mutations: MutationRecord[]) => { - for (const m of mutations) { - if (m.type === 'childList' && (m.addedNodes.length > 0 || m.removedNodes.length > 0)) { - if (inAdminSidebar(m.target)) { - continue; - } - let elementChanged = false; - m.addedNodes.forEach((n: Node) => { if (n.nodeType === 1) { elementChanged = true; } }); - m.removedNodes.forEach((n: Node) => { if (n.nodeType === 1) { elementChanged = true; } }); - if (elementChanged) { - armQuietTimer(); // a section rendered -> reset the quiet window - return; - } + /** + * Emits once when the live subtree stops being mutated (elements added/removed) for + * `ssrOverlaySettleQuietMs` AND it holds real content — or after `ssrOverlaySettleMaxMs`, whichever + * comes first. The cap guarantees a page that never goes quiet (constant background DOM updates) + * still reveals; the 15s fallback in index.html remains the ultimate net. + */ + private dsAppDomSettled$(): Observable { + const dsApp: Element | null = this.document.querySelector('ds-app'); + if (!dsApp) { + return timer(this.ssrOverlaySettleMaxMs); + } + const elementMutations$ = new Observable((subscriber) => { + const observer = new MutationObserver((records) => { + if (records.some((record) => this.isElementChildListChange(record))) { + subscriber.next(); } - } + }); + observer.observe(dsApp, { childList: true, subtree: true }); + return () => observer.disconnect(); }); + const settled$ = elementMutations$.pipe( + startWith(undefined), // start the quiet window immediately + debounceTime(this.ssrOverlaySettleQuietMs), // ... reset by each render, fires once quiet + filter(() => this.dsAppHasRenderedContent(dsApp)), // ... but never on the empty shell + ); + return race(settled$, timer(this.ssrOverlaySettleMaxMs)).pipe(take(1)); + } + + /** True once the live is no longer the empty shell the overlay script left behind. */ + private dsAppHasRenderedContent(dsApp: Element): boolean { + const height = dsApp.getBoundingClientRect?.().height ?? 0; + return height >= this.ssrOverlayMinContentHeightPx && dsApp.querySelector('#main-content') !== null; + } - if (app) { - observer.observe(app, { childList: true, subtree: true }); + /** A childList mutation that adds or removes at least one element node (ignores text/attr noise). */ + private isElementChildListChange(record: MutationRecord): boolean { + if (record.type !== 'childList') { + return false; + } + const changedNodes = [...Array.from(record.addedNodes), ...Array.from(record.removedNodes)]; + return changedNodes.some((node) => node.nodeType === Node.ELEMENT_NODE); + } + + /** Runs `callback` after the next paint (or synchronously if requestAnimationFrame is unavailable). */ + private runAfterNextFrame(win: Window, callback: () => void): void { + if (typeof win.requestAnimationFrame === 'function') { + win.requestAnimationFrame(() => callback()); + } else { + callback(); } - capTimer = setTimeout(finish, SETTLE_MAX_MS); - // expose teardown so ngOnDestroy can cancel a still-pending settle (and for test hygiene) - this.cancelOverlaySettle = () => { - if (quietTimer !== null) { clearTimeout(quietTimer); } - if (capTimer !== null) { clearTimeout(capTimer); } - try { observer.disconnect(); } catch (e) { /* noop */ } - }; - armQuietTimer(); } ngOnDestroy(): void { this.destroyed$.next(); this.destroyed$.complete(); - if (this.cancelOverlaySettle) { - this.cancelOverlaySettle(); - this.cancelOverlaySettle = null; - } } ngOnInit() {