mirror of
https://github.com/4gray/iptvnator.git
synced 2026-10-14 13:16:16 -08:00
fix(workspace): read scroll and focus state before a view's DOM detaches
Without the @angular/animations renderer a destroyed view's DOM leaves before its destroy hooks run, which the deferred removal used to hide. Two hooks read state from the detached DOM: - CategoryContentView.ngOnDestroy saved the grid's scrollTop, which reads 0 once detached, over the offset saved when a card was clicked: back from a VOD detail landed at the top of the list (catalog-sorting E2E on every OS). The view now keeps the offset of the grid that scrolled last; a grid rendered since (a new category's) saves the top. - Chromium blurs a focused element synchronously while removing it, so removing the focused player settings panel fired a focusout without a relatedTarget before the panel's destroy hook, which cleared focusInside: Escape left focus on the page instead of the tune button (player-controls-keyboard web E2E). The panel now judges a focusout a microtask later: a removed panel is detached then, while a pointer release blurs a panel that stays attached and still drops focus. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
67cf84f490
commit
d8ee38b6d1
5 files changed
+187
-6
No files matched your search
@@ -234,6 +234,15 @@ records no shift. `app.config.spec.ts` keeps the animations renderer out, and
|
||||
`workspace-route-swap.e2e.ts` asserts that no mutation checkpoint holds both
|
||||
pages and that neither the dashboard nor the content pane shifts.
|
||||
|
||||
Destroy hooks therefore run with the view's DOM already detached, where
|
||||
`scrollTop` reads 0 and focus has dropped to `body`. Chromium blurs a focused
|
||||
element synchronously while removing it, so its `focusout` (no
|
||||
`relatedTarget`) arrives before the hook. The deferred removal used to hide
|
||||
this. Record what a hook needs while the view is attached: the catalog keeps
|
||||
the offset of the grid that scrolled last, and the player settings panel
|
||||
judges a `focusout` a microtask later, when a removed panel is detached and a
|
||||
pointer-blurred one is not.
|
||||
|
||||
Do not add `provideAnimations()` or `provideAnimationsAsync()` back; animate
|
||||
with CSS. A leave animation (`animate.leave` included) delays its element's
|
||||
removal in the same way, so a routed page must not carry one.
|
||||
|
||||
+48
@@ -792,6 +792,54 @@ describe('CategoryContentViewComponent', () => {
|
||||
expect(catalog.saveScrollPosition).toHaveBeenCalledWith(333);
|
||||
});
|
||||
|
||||
it('saves the last scrolled offset on destroy, not the detached grid', () => {
|
||||
const infiniteFixture = createInfiniteFixture();
|
||||
infiniteFixture.detectChanges();
|
||||
const grid = infiniteFixture.nativeElement.querySelector(
|
||||
'app-grid-list'
|
||||
) as HTMLElement;
|
||||
Object.defineProperty(grid, 'scrollTop', {
|
||||
configurable: true,
|
||||
value: 640,
|
||||
});
|
||||
grid.dispatchEvent(new Event('scroll'));
|
||||
// Angular detaches a routed view before its destroy hooks run;
|
||||
// a detached grid has no layout and reads 0.
|
||||
Object.defineProperty(grid, 'scrollTop', {
|
||||
configurable: true,
|
||||
value: 0,
|
||||
});
|
||||
|
||||
infiniteFixture.destroy();
|
||||
|
||||
expect(catalog.saveScrollPosition).toHaveBeenCalledWith(640);
|
||||
});
|
||||
|
||||
it('saves the top on destroy for a grid recreated since the last scroll', () => {
|
||||
const infiniteFixture = createInfiniteFixture();
|
||||
infiniteFixture.detectChanges();
|
||||
const scrolledGrid = infiniteFixture.nativeElement.querySelector(
|
||||
'app-grid-list'
|
||||
) as HTMLElement;
|
||||
Object.defineProperty(scrolledGrid, 'scrollTop', {
|
||||
configurable: true,
|
||||
value: 640,
|
||||
});
|
||||
scrolledGrid.dispatchEvent(new Event('scroll'));
|
||||
// An inline detail replaces the grid; closing it renders a new one.
|
||||
selectedItem.set({ xtream_id: 42 });
|
||||
infiniteFixture.detectChanges();
|
||||
selectedItem.set(null);
|
||||
infiniteFixture.detectChanges();
|
||||
expect(
|
||||
infiniteFixture.nativeElement.querySelector('app-grid-list')
|
||||
).not.toBe(scrolledGrid);
|
||||
|
||||
infiniteFixture.destroy();
|
||||
|
||||
expect(catalog.saveScrollPosition).toHaveBeenLastCalledWith(0);
|
||||
});
|
||||
|
||||
it('consumes a saved scroll position once the list is rendered', () => {
|
||||
catalog.consumeSavedScrollPosition.mockReturnValue(500);
|
||||
const infiniteFixture = createInfiniteFixture();
|
||||
|
||||
+37
-5
@@ -7,6 +7,7 @@ import {
|
||||
effect,
|
||||
ElementRef,
|
||||
inject,
|
||||
NgZone,
|
||||
OnDestroy,
|
||||
OnInit,
|
||||
signal,
|
||||
@@ -198,6 +199,9 @@ export class CategoryContentViewComponent implements OnInit, OnDestroy {
|
||||
});
|
||||
private previousGridResetKey: string | null = null;
|
||||
private hasAttemptedScrollRestore = false;
|
||||
/** The offset of the grid that scrolled last (see ngOnDestroy). */
|
||||
private lastGridScroll: { grid: Element; scrollTop: number } | null =
|
||||
null;
|
||||
private readonly routeCategory = signal<{ id: string | null } | null>(null);
|
||||
private initializedRoute: {
|
||||
category: { id: string | null };
|
||||
@@ -205,6 +209,8 @@ export class CategoryContentViewComponent implements OnInit, OnDestroy {
|
||||
} | null = null;
|
||||
|
||||
constructor() {
|
||||
this.trackGridScrollTop();
|
||||
|
||||
effect(() => {
|
||||
const category = this.routeCategory();
|
||||
if (!category || this.catalog.routeReady?.() === false) {
|
||||
@@ -306,11 +312,15 @@ export class CategoryContentViewComponent implements OnInit, OnDestroy {
|
||||
// Leaving the list without opening a detail (e.g. switching portal
|
||||
// tabs) still snapshots the spot; the facade validates the selection
|
||||
// before ever restoring it.
|
||||
if (!this.selectedItem()) {
|
||||
const grid = this.gridElement();
|
||||
if (grid) {
|
||||
this.catalog.saveScrollPosition?.(grid.scrollTop);
|
||||
}
|
||||
// Angular removes a view's DOM before its destroy hooks run, so the
|
||||
// grid is detached here and its scrollTop reads 0. A grid that never
|
||||
// scrolled (a new category's) is still at the top.
|
||||
const grid = this.gridElement();
|
||||
if (!this.selectedItem() && grid) {
|
||||
const last = this.lastGridScroll;
|
||||
this.catalog.saveScrollPosition?.(
|
||||
last?.grid === grid ? last.scrollTop : 0
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -339,6 +349,28 @@ export class CategoryContentViewComponent implements OnInit, OnDestroy {
|
||||
}
|
||||
}
|
||||
|
||||
/** Scroll events do not bubble; a passive capture listener sees them. */
|
||||
private trackGridScrollTop(): void {
|
||||
const host: HTMLElement = this.hostElement.nativeElement;
|
||||
const onScroll = (event: Event) => {
|
||||
if (event.target instanceof HTMLElement) {
|
||||
if (event.target.tagName === 'APP-GRID-LIST') {
|
||||
this.lastGridScroll = {
|
||||
grid: event.target,
|
||||
scrollTop: event.target.scrollTop,
|
||||
};
|
||||
}
|
||||
}
|
||||
};
|
||||
const options = { capture: true, passive: true };
|
||||
inject(NgZone).runOutsideAngular(() =>
|
||||
host.addEventListener('scroll', onScroll, options)
|
||||
);
|
||||
this.destroyRef.onDestroy(() =>
|
||||
host.removeEventListener('scroll', onScroll, options)
|
||||
);
|
||||
}
|
||||
|
||||
private gridElement(): HTMLElement | null {
|
||||
return this.hostElement.nativeElement.querySelector(
|
||||
'app-grid-list'
|
||||
|
||||
+83
@@ -444,6 +444,89 @@ describe('PlayerControlsComponent settings panel', () => {
|
||||
fixture.nativeElement.remove();
|
||||
});
|
||||
|
||||
async function openSheetByKeyboard(): Promise<{
|
||||
sheet: HTMLElement;
|
||||
tune: HTMLButtonElement;
|
||||
}> {
|
||||
const tune = query(
|
||||
'[data-test-id="player-controls-settings-button"]'
|
||||
) as HTMLButtonElement;
|
||||
document.body.appendChild(fixture.nativeElement);
|
||||
tune.focus();
|
||||
const matches = tune.matches.bind(tune);
|
||||
Object.defineProperty(tune, 'matches', {
|
||||
configurable: true,
|
||||
value: (selector: string) =>
|
||||
selector === ':focus-visible' || matches(selector),
|
||||
});
|
||||
tune.click();
|
||||
fixture.detectChanges();
|
||||
await fixture.whenStable();
|
||||
const sheet = query(
|
||||
'[data-test-id="player-controls-settings-panel"]'
|
||||
) as HTMLElement;
|
||||
expect(document.activeElement).toBe(sheet);
|
||||
return { sheet, tune };
|
||||
}
|
||||
|
||||
async function closeSheetWithEscape(): Promise<void> {
|
||||
// Inside a task, as in the app: zone.js drains microtasks after
|
||||
// a top-level listener, which would run them mid-removal.
|
||||
await new Promise<void>((resolve) =>
|
||||
setTimeout(() => {
|
||||
document.dispatchEvent(
|
||||
new KeyboardEvent('keydown', {
|
||||
key: 'Escape',
|
||||
bubbles: true,
|
||||
})
|
||||
);
|
||||
fixture.detectChanges();
|
||||
resolve();
|
||||
})
|
||||
);
|
||||
await fixture.whenStable();
|
||||
await Promise.resolve();
|
||||
expect(
|
||||
query('[data-test-id="player-controls-settings-panel"]')
|
||||
).toBeNull();
|
||||
}
|
||||
|
||||
const blur = (target: HTMLElement) =>
|
||||
target.dispatchEvent(
|
||||
new FocusEvent('focusout', { bubbles: true, relatedTarget: null })
|
||||
);
|
||||
|
||||
it('returns focus to tune when the removed sheet reports a focusout', async () => {
|
||||
const { sheet, tune } = await openSheetByKeyboard();
|
||||
// Chromium blurs a focused element synchronously while removing
|
||||
// it, before the view's destroy hook; jsdom does not, so
|
||||
// emulate it.
|
||||
const remove = sheet.remove.bind(sheet);
|
||||
sheet.remove = () => {
|
||||
blur(sheet);
|
||||
remove();
|
||||
};
|
||||
|
||||
await closeSheetWithEscape();
|
||||
|
||||
expect(document.activeElement).toBe(tune);
|
||||
fixture.nativeElement.remove();
|
||||
});
|
||||
|
||||
it('leaves focus on the page after a pointer release blurred the sheet', async () => {
|
||||
const { sheet, tune } = await openSheetByKeyboard();
|
||||
// A pointer click releases focus in its own task, while the
|
||||
// sheet stays attached.
|
||||
blur(sheet);
|
||||
sheet.blur();
|
||||
await Promise.resolve();
|
||||
|
||||
await closeSheetWithEscape();
|
||||
|
||||
expect(document.activeElement).not.toBe(tune);
|
||||
fixture.nativeElement.remove();
|
||||
});
|
||||
|
||||
it('leaves focus alone when a pointer opens the sheet', async () => {
|
||||
query('[data-test-id="player-controls-settings-button"]')?.click();
|
||||
fixture.detectChanges();
|
||||
|
||||
@@ -122,7 +122,16 @@ export class PlayerSettingsPanelComponent implements OnDestroy {
|
||||
|
||||
onFocusOut(event: FocusEvent): void {
|
||||
const next = event.relatedTarget;
|
||||
this.focusInside = next instanceof Node && this.host.contains(next);
|
||||
const inside = next instanceof Node && this.host.contains(next);
|
||||
// Chromium blurs a focused element synchronously while removing it,
|
||||
// before this view's destroy hook runs. Judge once the removal is
|
||||
// done: a detached panel did not lose focus to anything, while a
|
||||
// pointer release blurs a panel that stays attached.
|
||||
queueMicrotask(() => {
|
||||
if (this.host.isConnected) {
|
||||
this.focusInside = inside;
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in new issue
Block a user