From 503f009a1fad0464ed9c2855c21ea94edcf3cc83 Mon Sep 17 00:00:00 2001 From: 4gray Date: Sun, 6 Sep 2026 15:13:54 +0200 Subject: [PATCH] fix(m3u): make guide row ids collision-proof and gate the G shortcut Co-Authored-By: Claude Fable 5.1 --- docs/architecture/m3u-playlist-module.md | 22 +- .../m3u-epg-guide-source.service.spec.ts | 55 +++-- .../epg-guide/m3u-epg-guide-source.service.ts | 36 +-- .../video-player-guide-gating.spec.ts | 226 ++++++++++++++++++ .../video-player/video-player.component.ts | 50 +++- 5 files changed, 335 insertions(+), 54 deletions(-) create mode 100644 libs/playlist/m3u/feature-player/src/lib/video-player/video-player-guide-gating.spec.ts diff --git a/docs/architecture/m3u-playlist-module.md b/docs/architecture/m3u-playlist-module.md index f885ca4a1..b54e891eb 100644 --- a/docs/architecture/m3u-playlist-module.md +++ b/docs/architecture/m3u-playlist-module.md @@ -1140,11 +1140,16 @@ are unscoped, like the timeline. Row ids are not channel ids: `createChannel` falls back to the stream URL for an entry without an explicit id, so one stream listed in two groups yields two -channels sharing an id. The adapter numbers repeats within the scope (``, -`#1`, …) — the guide keys its programme, coverage and selection maps by -row id, so without that both copies lit up as playing and activating either -one played the first. The active channel marks the FIRST row carrying its id, -and `activate(rowId)` resolves the row back to its own channel. Opening the +channels sharing an id. The adapter therefore builds SCOPE-LOCAL row ids by +prefixing every row with its position in the scope (`:`) — +the guide keys its programme, coverage and selection maps by row id, so +without that both copies lit up as playing and activating either one played +the first, and suffixing only the repeats was not enough because a real +channel id can itself look like a generated suffix (ids `x`, `x`, `x#1` +produced `x#1` twice). Ids move with the scope and the channel list, so only +ids the guide was just handed may be passed back. The active channel marks the +FIRST row carrying its id (`null` when it is outside the scope), and +`activate(rowId)` resolves the row back to its own channel. Opening the guide mirrors the sidebar view (`applyInitialScope`): favorites stays favorites, and the groups view opens on the group the sidebar's rail is SHOWING — forwarded from `GroupsViewComponent.selectedGroupChange` through the @@ -1170,7 +1175,12 @@ toolbar (`EpgTimelineComponent.openGuide`) and the `G` key on the player page. The header action reports `disabled` whenever the guide cannot open, which greys out the header button and disables its palette command instead of offering a no-op. Player fullscreen, radio, recognised movies, switching to -another playlist and the PWA close or withhold it. Inside the guide: single click on a row or an +another playlist and the PWA close or withhold it. `openGuide()` itself also +refuses while ANY element is fullscreen or a CDK dialog is open — the guide +mounts in the page flow, so it would be painted over and still swallow the +keyboard — and the `G` key additionally ignores a press raised inside a +`.cdk-overlay-pane`, `[role="menu"]` or `[role="dialog"]`, the same surfaces +the PageUp/PageDown zapping yields to. Inside the guide: single click on a row or an "on now" card switches the channel and keeps the guide open, double-click or Enter switches and closes, other cards open the programme dialog; ↑/↓ ←/→ navigate, I details, N now, PgUp/PgDn day, Esc close — the keyboard controller diff --git a/libs/playlist/m3u/feature-player/src/lib/epg-guide/m3u-epg-guide-source.service.spec.ts b/libs/playlist/m3u/feature-player/src/lib/epg-guide/m3u-epg-guide-source.service.spec.ts index 0cc598ec1..beb0450de 100644 --- a/libs/playlist/m3u/feature-player/src/lib/epg-guide/m3u-epg-guide-source.service.spec.ts +++ b/libs/playlist/m3u/feature-player/src/lib/epg-guide/m3u-epg-guide-source.service.spec.ts @@ -111,16 +111,22 @@ describe('M3uEpgGuideSourceService', () => { 1, 2, 3, ]); service.setScope('group:Sports'); + // Row ids are scope-local: the position inside the scope, then the + // channel id. expect(service.channels().map((channel) => channel.id)).toEqual([ - 'b', - 'c', + '0:b', + '1:c', ]); expect(service.channels()[0].number).toBe(1); service.setScope('favorites'); - expect(service.channels().map((channel) => channel.id)).toEqual(['b']); + expect(service.channels().map((channel) => channel.id)).toEqual([ + '0:b', + ]); // Legacy rows saved before URL keys are still matched by channel id. favoriteKeys.set(['c']); - expect(service.channels().map((channel) => channel.id)).toEqual(['c']); + expect(service.channels().map((channel) => channel.id)).toEqual([ + '0:c', + ]); }); it('derives the EPG key from tvg-id, then name, and null for blank names', () => { @@ -142,12 +148,12 @@ describe('M3uEpgGuideSourceService', () => { fromMs: 1, toMs: 2, }); - expect(programs.get('a')?.[0].title).toBe('a.tv show'); - expect(programs.get('b')).toEqual([]); - expect(programs.has('c')).toBe(false); + expect(programs.get('0:a')?.[0].title).toBe('a.tv show'); + expect(programs.get('1:b')).toEqual([]); + expect(programs.has('2:c')).toBe(false); const covered = await service.loadCoverage(window); - expect(covered).toEqual(new Set(['a'])); + expect(covered).toEqual(new Set(['0:a'])); }); it('answers empty results when the bridge is unavailable', async () => { @@ -159,8 +165,8 @@ describe('M3uEpgGuideSourceService', () => { }); it('mirrors the active channel and dispatches playback on activate', () => { - expect(service.activeChannelId()).toBe('a'); - service.activate('b'); + expect(service.activeChannelId()).toBe('0:a'); + service.activate('1:b'); expect(dispatch).toHaveBeenCalledWith( ChannelActions.setActiveChannel({ channel: channels()[1], @@ -173,10 +179,13 @@ describe('M3uEpgGuideSourceService', () => { it('keeps duplicate channel ids apart and activates the row that was clicked', () => { // `createChannel` falls back to the URL as the id, so one stream - // listed twice yields two channels sharing an id. + // listed twice yields two channels sharing an id. A real id can also + // look like a generated suffix (`x#1`), so suffixing the repeats made + // the third row collide with the second; the position prefix cannot. const duplicated = signal([ - makeChannel('dup', { name: 'First copy', group: 'News' }), - makeChannel('dup', { name: 'Second copy', group: 'News' }), + makeChannel('x', { name: 'First copy', group: 'News' }), + makeChannel('x', { name: 'Second copy', group: 'News' }), + makeChannel('x#1', { name: 'Hashed id', group: 'News' }), ]); service.bind({ channels: duplicated, @@ -186,18 +195,26 @@ describe('M3uEpgGuideSourceService', () => { }); const rowIds = service.channels().map((channel) => channel.id); - expect(rowIds).toEqual(['dup', 'dup#1']); + expect(rowIds).toEqual(['0:x', '1:x', '2:x#1']); + expect(new Set(rowIds).size).toBe(3); // The playing channel marks the first of the two rows. - expect(service.activeChannelId()).toBe('dup'); + expect(service.activeChannelId()).toBe('0:x'); - service.activate('dup#1'); - - expect(dispatch).toHaveBeenCalledWith( + service.activate('1:x'); + expect(dispatch).toHaveBeenLastCalledWith( ChannelActions.setActiveChannel({ channel: duplicated()[1], startPlayback: true, }) ); + + service.activate('2:x#1'); + expect(dispatch).toHaveBeenLastCalledWith( + ChannelActions.setActiveChannel({ + channel: duplicated()[2], + startPlayback: true, + }) + ); }); it('seeds the initial scope from the sidebar view', () => { @@ -256,7 +273,7 @@ describe('M3uEpgGuideSourceService', () => { it('forwards programme search to the bridge', async () => { searchPrograms.mockResolvedValue([program('a.tv'), program('x')]); const hits = await service.searchPrograms('news'); - expect(hits.map((hit) => hit.channelId)).toEqual(['a', null]); + expect(hits.map((hit) => hit.channelId)).toEqual(['0:a', null]); expect(hits[1].program.title).toBe('x show'); expect(searchPrograms).toHaveBeenCalledWith('news', 20); }); diff --git a/libs/playlist/m3u/feature-player/src/lib/epg-guide/m3u-epg-guide-source.service.ts b/libs/playlist/m3u/feature-player/src/lib/epg-guide/m3u-epg-guide-source.service.ts index a10c3a8e6..cfa52569f 100644 --- a/libs/playlist/m3u/feature-player/src/lib/epg-guide/m3u-epg-guide-source.service.ts +++ b/libs/playlist/m3u/feature-player/src/lib/epg-guide/m3u-epg-guide-source.service.ts @@ -122,28 +122,28 @@ export class M3uEpgGuideSourceService implements EpgGuideSource { * explicit id, so one stream listed in two groups — or simply repeated — * yields two channels sharing an id. The guide keys its programme, status * and selection maps by row id, so duplicates used to collide: both rows - * lit up as playing and activating either one played the first. The first - * occurrence keeps the channel id (nothing changes for a playlist without - * duplicates) and every later one is suffixed. + * lit up as playing and activating either one played the first. + * Every row is therefore prefixed with its position in the scope: two rows + * always differ in that leading integer, so no channel id can collide with + * another row (suffixing only the repeats did not hold — a playlist whose + * ids are `x`, `x` and `x#1` produced `x#1` twice). Ids are SCOPE-LOCAL: + * they change with the scope and the channel list, and only ids the guide + * was just handed in `channels()` may be passed back. */ private readonly scopedRows = computed< Array<{ rowId: string; channel: Channel }> - >(() => { - const occurrences = new Map(); - return this.scopedChannels().map((channel) => { - const seen = occurrences.get(channel.id) ?? 0; - occurrences.set(channel.id, seen + 1); - return { - rowId: seen === 0 ? channel.id : `${channel.id}#${seen}`, - channel, - }; - }); - }); + >(() => + this.scopedChannels().map((channel, index) => ({ + rowId: `${index}:${channel.id}`, + channel, + })) + ); private readonly channelsByRowId = computed( () => new Map(this.scopedRows().map((row) => [row.rowId, row.channel])) ); + /** `EpgGuideChannel.id` is the scope-local row id, not `Channel.id`. */ readonly channels = computed(() => { const strip = this.settingsStore.stripCountryPrefix?.(); return this.scopedRows().map(({ rowId, channel }, index) => ({ @@ -156,8 +156,10 @@ export class M3uEpgGuideSourceService implements EpgGuideSource { }); /** - * The first row carrying the active channel's id — duplicates are - * indistinguishable from here, so the guide marks the first of them. + * The row id of the first row carrying the active channel's id — + * duplicates are indistinguishable from here, so the guide marks the first + * of them. `null` when the playing channel is outside the current scope: + * row ids are scope-local, so there is nothing to point at. */ readonly activeChannelId = computed(() => { const active = this.inputs()?.activeChannel(); @@ -166,7 +168,7 @@ export class M3uEpgGuideSourceService implements EpgGuideSource { } return ( this.scopedRows().find((row) => row.channel.id === active.id) - ?.rowId ?? active.id + ?.rowId ?? null ); }); diff --git a/libs/playlist/m3u/feature-player/src/lib/video-player/video-player-guide-gating.spec.ts b/libs/playlist/m3u/feature-player/src/lib/video-player/video-player-guide-gating.spec.ts new file mode 100644 index 000000000..eaba192ea --- /dev/null +++ b/libs/playlist/m3u/feature-player/src/lib/video-player/video-player-guide-gating.spec.ts @@ -0,0 +1,226 @@ +import { NO_ERRORS_SCHEMA, signal } from '@angular/core'; +import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { ActivatedRoute, Router } from '@angular/router'; +import { Store } from '@ngrx/store'; +import { StorageMap } from '@ngx-pwa/local-storage'; +import { of } from 'rxjs'; +import { EpgService } from '@iptvnator/epg/data-access'; +import { PlaylistContextFacade } from '@iptvnator/playlist/shared/util'; +import { PORTAL_EXTERNAL_PLAYBACK } from '@iptvnator/portal/shared/util'; +import { + DataService, + PlaylistsService, + RuntimeCapabilitiesService, + SettingsStore, + TmdbEnrichmentService, +} from '@iptvnator/services'; +import { Settings, VideoPlayer } from '@iptvnator/shared/interfaces'; +import type { VideoPlayerComponent as VideoPlayerComponentInstance } from './video-player.component'; +import { + dataServiceMock, + epgServiceMock, + epgUrlSetting, + epgViewMode, + externalSession, + player, + playlistId, + playlistsServiceMock, + routerMock, + sampleChannel, + showCaptions, + storeMock, + stripCountryPrefix, + syncStoreState, + translateServiceProvider, +} from './video-player.spec-harness'; + +jest.unstable_mockModule('video.js', () => ({ + default: jest.fn(), +})); +jest.unstable_mockModule('@yangkghjh/videojs-aspect-ratio-panel', () => ({})); +jest.unstable_mockModule('videojs-contrib-quality-levels', () => ({})); +jest.unstable_mockModule('videojs-quality-selector-hls', () => ({})); + +/** + * What may NOT open the programme guide. The guide mounts in the page flow, + * so a fullscreen element or a dialog paints over it while it still owns the + * keyboard. Kept apart from `video-player.component.spec.ts`, which sits at + * the spec line budget; the template is reduced to the panel's `ng-template` + * so no child stubs are needed. + */ +describe('VideoPlayerComponent — guide gating', () => { + let VideoPlayerComponent: typeof import('./video-player.component').VideoPlayerComponent; + let fixture: ComponentFixture; + let component: VideoPlayerComponentInstance; + + function setFullscreenElement(element: Element | null): void { + Object.defineProperty(document, 'fullscreenElement', { + configurable: true, + get: () => element, + }); + } + + /** A stand-in for the inline live player, inside the component's host. */ + function mountPlayerView(): HTMLElement { + const playerView = document.createElement('app-web-player-view'); + fixture.nativeElement.appendChild(playerView); + return playerView; + } + + function pressGuideKey(target: EventTarget = document): void { + target.dispatchEvent( + new KeyboardEvent('keydown', { key: 'g', bubbles: true }) + ); + } + + beforeAll(async () => { + ({ VideoPlayerComponent } = await import('./video-player.component')); + }); + + beforeEach(async () => { + setFullscreenElement(null); + syncStoreState(sampleChannel); + playlistId.set('playlist-1'); + player.set(VideoPlayer.VideoJs); + showCaptions.set(false); + stripCountryPrefix.set(false); + externalSession.set(null); + storeMock.dispatch.mockClear(); + + await TestBed.configureTestingModule({ + imports: [VideoPlayerComponent], + schemas: [NO_ERRORS_SCHEMA], + providers: [ + { + provide: ActivatedRoute, + useValue: { + params: of({ id: playlistId(), view: 'all' }), + queryParams: of({}), + snapshot: { + data: { layout: 'workspace' }, + queryParams: {}, + }, + }, + }, + { provide: Router, useValue: routerMock }, + { provide: Store, useValue: storeMock }, + translateServiceProvider, + { provide: DataService, useValue: dataServiceMock }, + { + provide: RuntimeCapabilitiesService, + useValue: { + supportsEpg: true, + isElectron: false, + supportsRemoteControl: false, + }, + }, + { provide: PlaylistsService, useValue: playlistsServiceMock }, + { provide: EpgService, useValue: epgServiceMock }, + { + provide: PlaylistContextFacade, + useValue: { resolvedPlaylistId: playlistId }, + }, + { + provide: TmdbEnrichmentService, + useValue: { isEnabled: () => false }, + }, + { + provide: SettingsStore, + useValue: { + player, + showCaptions, + stripCountryPrefix, + m3uVodDetails: signal(true), + resolvedEpgViewMode: epgViewMode, + resolvedEpgOffsetMinutes: signal(0), + epgUrl: epgUrlSetting, + }, + }, + { + provide: StorageMap, + useValue: { + get: jest.fn(() => + of({ player: player() } as Partial) + ), + }, + }, + { + provide: PORTAL_EXTERNAL_PLAYBACK, + useValue: { activeSession: externalSession }, + }, + ], + }) + .overrideComponent(VideoPlayerComponent, { + set: { + imports: [], + template: + '', + }, + }) + .compileComponents(); + + fixture = TestBed.createComponent(VideoPlayerComponent); + component = fixture.componentInstance; + fixture.detectChanges(); + }); + + afterEach(() => { + fixture?.destroy(); + setFullscreenElement(null); + document + .querySelectorAll('.cdk-overlay-container, [role="dialog"]') + .forEach((element) => element.remove()); + }); + + it('leaves the guide closed while the live player owns fullscreen', () => { + expect(component.canOpenGuide()).toBe(true); + setFullscreenElement(mountPlayerView()); + + pressGuideKey(); + + // The fullscreen surface paints over the page: the guide would be + // invisible there and still swallow the keys. + expect(component.guideOpen()).toBe(false); + component.openGuide(); + expect(component.guideOpen()).toBe(false); + + setFullscreenElement(null); + pressGuideKey(); + expect(component.guideOpen()).toBe(true); + }); + + it('leaves the guide closed while any element is fullscreen', () => { + const other = document.createElement('div'); + document.body.appendChild(other); + setFullscreenElement(other); + + component.openGuide(); + + expect(component.guideOpen()).toBe(false); + other.remove(); + }); + + it('leaves the guide closed while a dialog is open', () => { + const overlayContainer = document.createElement('div'); + overlayContainer.className = 'cdk-overlay-container'; + const dialog = document.createElement('div'); + dialog.setAttribute('role', 'dialog'); + const dialogButton = document.createElement('button'); + dialog.appendChild(dialogButton); + overlayContainer.appendChild(dialog); + document.body.appendChild(overlayContainer); + + // The header action and the command palette go through `openGuide()` + // without an event to inspect. + component.openGuide(); + expect(component.guideOpen()).toBe(false); + + // A G from a focused control inside the dialog belongs to the dialog. + pressGuideKey(dialogButton); + expect(component.guideOpen()).toBe(false); + + overlayContainer.remove(); + pressGuideKey(); + expect(component.guideOpen()).toBe(true); + }); +}); diff --git a/libs/playlist/m3u/feature-player/src/lib/video-player/video-player.component.ts b/libs/playlist/m3u/feature-player/src/lib/video-player/video-player.component.ts index 10541045b..104497558 100644 --- a/libs/playlist/m3u/feature-player/src/lib/video-player/video-player.component.ts +++ b/libs/playlist/m3u/feature-player/src/lib/video-player/video-player.component.ts @@ -1239,13 +1239,41 @@ export class VideoPlayerComponent } openGuide(): void { - if (!this.canOpenGuide() || this.guideOpen()) { + if (!this.canOpenGuide() || this.guideOpen() || this.isGuideBlocked()) { return; } this.guideSource.applyInitialScope(this.activeView()); this.guideOpen.set(true); } + /** + * The guide mounts in the page flow, so anything painting over the page + * would hide it while it still swallowed the keyboard: a fullscreen + * element (the live player's own, which has the fullscreen channel panel + * instead — see {@link onFullscreenChange} — or any other) and an open + * CDK dialog. Gated here rather than in the key handler alone so the + * header action and the command palette are covered too. + */ + private isGuideBlocked(): boolean { + return ( + !!document.fullscreenElement || + !!document.querySelector('.cdk-overlay-container [role="dialog"]') + ); + } + + /** Whether the event was raised inside a menu, dialog or CDK overlay. */ + private isInsideOverlaySurface(event: KeyboardEvent): boolean { + return event + .composedPath() + .some( + (target) => + target instanceof Element && + target.matches( + '.cdk-overlay-pane, [role="menu"], [role="dialog"]' + ) + ); + } + closeGuide(): void { this.guideOpen.set(false); } @@ -1301,8 +1329,14 @@ export class VideoPlayerComponent return; } if (isGuideKey && this.canOpenGuide()) { - event.preventDefault(); - this.openGuide(); + // A G from a focused control inside a dialog or menu belongs to + // that surface, not to the page behind it. + if (!this.isInsideOverlaySurface(event)) { + this.openGuide(); + } + if (this.guideOpen()) { + event.preventDefault(); + } return; } if ( @@ -1320,15 +1354,7 @@ export class VideoPlayerComponent // the one way to change channels from the keyboard in fullscreen. if (event.key === 'PageUp' || event.key === 'PageDown') { if ( - event - .composedPath() - .some( - (target) => - target instanceof Element && - target.matches( - '.cdk-overlay-pane, [role="menu"], [role="dialog"]' - ) - ) || + this.isInsideOverlaySurface(event) || isInsideScrollableRegion( event.target, this.hostElement.nativeElement