mirror of
https://github.com/4gray/iptvnator.git
synced 2026-10-15 05:36:17 -08:00
fix(web): snapshot the catalog offset and the panel's focus before the view detaches
Two behaviours relied on a side effect of the animations module: its renderer deferred every DOM removal to the end of the change-detection tick, so a destroy hook still saw its view attached. Without the module Angular detaches the view first, where scrollTop reads 0 and Chromium has already fired a focusout with no related target for the focused control. The first CI run of #1931 failed the Electron catalog-sorting round trip on every OS and the web player-controls keyboard focus return on exactly that. The catalog list now tracks the grid offset from captured scroll events and saves that when it is destroyed detached (a connected grid is still read live); the saved 0 that overwrote the click-time snapshot is gone. The player settings panel decides "focus was inside" in a microtask after a focusout without related target, which the close destroys the panel before, so the removal-induced event no longer cancels the focus return. Both specs detach the view first and fail on the old code; the UI guidelines record the rule for destroy hooks. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
1 parent
e417f09bce
commit
85e979ccf0
5 files changed
+120
-4
No files matched your search
@@ -127,7 +127,16 @@ Everything else is lazy by these rules:
|
||||
Material 22 animates with CSS, deciding from `prefers-reduced-motion` itself,
|
||||
so `app.config.ts` does not call `provideAnimations()`; the Electron
|
||||
`material-motion.e2e.ts` checks that dialogs, selects, menus, snack-bars and
|
||||
tooltips still animate in and out and stop under reduced motion.
|
||||
tooltips still animate in and out and stop under reduced motion. One side
|
||||
effect of the module is gone with it: its renderer deferred every DOM
|
||||
removal to the end of the change-detection tick, so a destroy hook still
|
||||
saw its view attached. Without it Angular detaches the view first, where
|
||||
`scrollTop` reads 0 and Chromium has already fired a `focusout` with no
|
||||
related target for the focused control. A destroy hook therefore never
|
||||
reads geometry or focus from its own view: the catalog list tracks the
|
||||
grid offset from scroll events, and the player settings panel decides
|
||||
"focus was inside" in a microtask after such a `focusout`, which the close
|
||||
destroys the panel before. Both have specs that detach the view first.
|
||||
|
||||
The app theme is the `.dark-theme` class that `SettingsService` puts on
|
||||
`<body>`; a "System" choice resolves the OS preference there and nowhere
|
||||
|
||||
+20
@@ -792,6 +792,26 @@ describe('CategoryContentViewComponent', () => {
|
||||
expect(catalog.saveScrollPosition).toHaveBeenCalledWith(333);
|
||||
});
|
||||
|
||||
it('saves the offset the grid last reported when it is destroyed detached', () => {
|
||||
const infiniteFixture = createInfiniteFixture();
|
||||
infiniteFixture.detectChanges();
|
||||
const grid = infiniteFixture.nativeElement.querySelector(
|
||||
'app-grid-list'
|
||||
) as HTMLElement;
|
||||
// A detached element reports scrollTop 0, as in a browser.
|
||||
Object.defineProperty(grid, 'scrollTop', {
|
||||
configurable: true,
|
||||
get: () => (grid.isConnected ? 420 : 0),
|
||||
});
|
||||
grid.dispatchEvent(new Event('scroll'));
|
||||
|
||||
// The view's DOM leaves the document before ngOnDestroy runs.
|
||||
infiniteFixture.nativeElement.remove();
|
||||
infiniteFixture.destroy();
|
||||
|
||||
expect(catalog.saveScrollPosition).toHaveBeenCalledWith(420);
|
||||
});
|
||||
|
||||
it('consumes a saved scroll position once the list is rendered', () => {
|
||||
catalog.consumeSavedScrollPosition.mockReturnValue(500);
|
||||
const infiniteFixture = createInfiniteFixture();
|
||||
|
||||
+30
-2
@@ -198,6 +198,18 @@ export class CategoryContentViewComponent implements OnInit, OnDestroy {
|
||||
});
|
||||
private previousGridResetKey: string | null = null;
|
||||
private hasAttemptedScrollRestore = false;
|
||||
/**
|
||||
* The grid's last reported offset. On destroy the view is already
|
||||
* detached from the document, where `scrollTop` reads 0, so the hook
|
||||
* saves this instead. Scroll events do not bubble but can be captured.
|
||||
*/
|
||||
private lastGridScrollTop = 0;
|
||||
private readonly trackGridScroll = (event: Event) => {
|
||||
const target = event.target;
|
||||
if (target instanceof HTMLElement && target.matches('app-grid-list')) {
|
||||
this.lastGridScrollTop = target.scrollTop;
|
||||
}
|
||||
};
|
||||
private readonly routeCategory = signal<{ id: string | null } | null>(null);
|
||||
private initializedRoute: {
|
||||
category: { id: string | null };
|
||||
@@ -205,6 +217,16 @@ export class CategoryContentViewComponent implements OnInit, OnDestroy {
|
||||
} | null = null;
|
||||
|
||||
constructor() {
|
||||
const host = this.hostElement.nativeElement;
|
||||
host.addEventListener('scroll', this.trackGridScroll, {
|
||||
capture: true,
|
||||
passive: true,
|
||||
});
|
||||
this.destroyRef.onDestroy(() =>
|
||||
host.removeEventListener('scroll', this.trackGridScroll, {
|
||||
capture: true,
|
||||
})
|
||||
);
|
||||
effect(() => {
|
||||
const category = this.routeCategory();
|
||||
if (!category || this.catalog.routeReady?.() === false) {
|
||||
@@ -305,11 +327,15 @@ export class CategoryContentViewComponent implements OnInit, OnDestroy {
|
||||
ngOnDestroy(): void {
|
||||
// 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.
|
||||
// before ever restoring it. Angular detaches the view's DOM before
|
||||
// this hook runs, and a detached grid reports a scrollTop of 0, so
|
||||
// the tracked offset stands in for it.
|
||||
if (!this.selectedItem()) {
|
||||
const grid = this.gridElement();
|
||||
if (grid) {
|
||||
this.catalog.saveScrollPosition?.(grid.scrollTop);
|
||||
this.catalog.saveScrollPosition?.(
|
||||
grid.isConnected ? grid.scrollTop : this.lastGridScrollTop
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -346,6 +372,8 @@ export class CategoryContentViewComponent implements OnInit, OnDestroy {
|
||||
}
|
||||
|
||||
private scrollGridToTop(): void {
|
||||
// A grid already at the top fires no scroll event for this call.
|
||||
this.lastGridScrollTop = 0;
|
||||
this.gridElement()?.scrollTo?.({ top: 0 });
|
||||
}
|
||||
|
||||
|
||||
+46
@@ -444,6 +444,52 @@ describe('PlayerControlsComponent settings panel', () => {
|
||||
fixture.nativeElement.remove();
|
||||
});
|
||||
|
||||
it('returns focus to tune although removal fires focusout with no related target', async () => {
|
||||
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);
|
||||
|
||||
// Chromium fires focusout with relatedTarget null on the focused
|
||||
// element while its subtree is being removed, in the same task
|
||||
// as the removal; jsdom does not, so the event is dispatched by
|
||||
// hand right before the close destroys the panel.
|
||||
sheet.dispatchEvent(
|
||||
new FocusEvent('focusout', {
|
||||
bubbles: true,
|
||||
relatedTarget: null,
|
||||
})
|
||||
);
|
||||
document.dispatchEvent(
|
||||
new KeyboardEvent('keydown', { key: 'Escape', bubbles: true })
|
||||
);
|
||||
fixture.detectChanges();
|
||||
await fixture.whenStable();
|
||||
await Promise.resolve();
|
||||
|
||||
expect(
|
||||
query('[data-test-id="player-controls-settings-panel"]')
|
||||
).toBeNull();
|
||||
expect(document.activeElement).toBe(
|
||||
query('[data-test-id="player-controls-settings-button"]')
|
||||
);
|
||||
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,20 @@ export class PlayerSettingsPanelComponent implements OnDestroy {
|
||||
|
||||
onFocusOut(event: FocusEvent): void {
|
||||
const next = event.relatedTarget;
|
||||
this.focusInside = next instanceof Node && this.host.contains(next);
|
||||
if (next instanceof Node) {
|
||||
this.focusInside = this.host.contains(next);
|
||||
return;
|
||||
}
|
||||
// No related target: focus went to the page, or the panel is being
|
||||
// removed with focus inside, which Chromium reports with this event
|
||||
// while the panel is still attached. The close destroys the panel in
|
||||
// the same task, before this microtask runs, so a removal leaves the
|
||||
// flag for ngOnDestroy to act on.
|
||||
queueMicrotask(() => {
|
||||
this.focusInside =
|
||||
this.host.isConnected &&
|
||||
this.host.contains(document.activeElement);
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in new issue
Block a user