From 0a55fb1ef6935eabe83a0cf62c500b690620b921 Mon Sep 17 00:00:00 2001 From: felix-hoc Date: Wed, 18 Dec 2024 16:42:16 +0100 Subject: [PATCH 01/11] Use span element instead of `::after` for caret --- .../skin-modern/components/_seekbarlabel.scss | 30 +++++++++---------- src/ts/components/seekbarlabel.ts | 25 ++++++++++------ 2 files changed, 30 insertions(+), 25 deletions(-) diff --git a/src/scss/skin-modern/components/_seekbarlabel.scss b/src/scss/skin-modern/components/_seekbarlabel.scss index f76c8f1d7..bf551bc07 100644 --- a/src/scss/skin-modern/components/_seekbarlabel.scss +++ b/src/scss/skin-modern/components/_seekbarlabel.scss @@ -23,25 +23,23 @@ @extend %center-on-left-edge; } + // bottom arrow from http://www.cssarrowplease.com/ + .#{$prefix}-seekbar-label-caret { + border: solid transparent; + border-color: transparent; + border-top-color: $color-primary; + border-width: .5em; + height: 0; + margin-left: -1em; + pointer-events: none; + position: absolute; + top: 100%; + width: 0; + } + .#{$prefix}-seekbar-label-inner { border-bottom: .2em solid $color-primary; - // bottom arrow from http://www.cssarrowplease.com/ - &::after { - border: solid transparent; - border-color: transparent; - border-top-color: $color-primary; - border-width: .5em; - content: ' '; - height: 0; - left: 50%; - margin-left: -.5em; - pointer-events: none; - position: absolute; - top: 100%; - width: 0; - } - > .#{$prefix}-container-wrapper { position: relative; diff --git a/src/ts/components/seekbarlabel.ts b/src/ts/components/seekbarlabel.ts index eefb26beb..4673810c0 100644 --- a/src/ts/components/seekbarlabel.ts +++ b/src/ts/components/seekbarlabel.ts @@ -36,6 +36,8 @@ export class SeekBarLabel extends Container { private appliedMarkerCssClasses: string[] = []; private player: PlayerAPI; private uiManager: UIInstanceManager; + private readonly container: Container; + private readonly caret: Label; constructor(config: SeekBarLabelConfig = {}) { super(config); @@ -45,17 +47,22 @@ export class SeekBarLabel extends Container { this.thumbnail = new Component({ cssClasses: ['seekbar-thumbnail'], role: 'img' }); this.thumbnailImageLoader = new ImageLoader(); + this.container = new Container({ + components: [ + this.thumbnail, + new Container({ + components: [this.titleLabel, this.timeLabel], + cssClass: 'seekbar-label-metadata', + }), + ], + cssClass: 'seekbar-label-inner', + }); + + this.caret = new Label({ cssClasses: ['seekbar-label-caret'] }); + this.config = this.mergeConfig(config, { cssClass: 'ui-seekbar-label', - components: [new Container({ - components: [ - this.thumbnail, - new Container({ - components: [this.titleLabel, this.timeLabel], - cssClass: 'seekbar-label-metadata', - })], - cssClass: 'seekbar-label-inner', - })], + components: [this.container, this.caret], hidden: true, }, this.config); } From fefcd7f9044176c48005754733d94b571a5b1120 Mon Sep 17 00:00:00 2001 From: felix-hoc Date: Wed, 18 Dec 2024 16:43:03 +0100 Subject: [PATCH 02/11] Limit thumbnail preview to UI bounds --- src/ts/components/seekbar.ts | 23 ++++++++++++++++++++--- src/ts/components/seekbarlabel.ts | 28 ++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/src/ts/components/seekbar.ts b/src/ts/components/seekbar.ts index 8227e0a34..203387653 100644 --- a/src/ts/components/seekbar.ts +++ b/src/ts/components/seekbar.ts @@ -114,7 +114,10 @@ export class SeekBar extends Component { private seekBarMarkersContainer: DOM; private timelineMarkersHandler: TimelineMarkersHandler; + private uiBoundingRect: DOMRect; + private player: PlayerAPI; + private uiManager: UIInstanceManager; protected seekBarType: SeekBarType; @@ -220,6 +223,7 @@ export class SeekBar extends Component { super.configure(player, uimanager); this.player = player; + this.uiManager = uimanager; // Apply scaling transform to the backdrop bar to have all bars rendered similarly // (the call must be up here to be executed for the volume slider as well) @@ -453,6 +457,7 @@ export class SeekBar extends Component { // is positioned absolutely and must therefore be updated when the size of the seekbar changes. player.on(player.exports.PlayerEvent.PlayerResized, () => { this.refreshPlaybackPosition(); + this.uiBoundingRect = this.uiManager.getUI().getDomElement().get(0).getBoundingClientRect(); }); // Additionally, when this code is called, the seekbar is not part of the UI yet and therefore does not have a size, // resulting in a wrong initial position of the marker. Refreshing it once the UI is configured solved this issue. @@ -1020,6 +1025,20 @@ export class SeekBar extends Component { this.seekBarEvents.onSeek.dispatch(this); } + private updateLabelPosition = (seekPositionPercentage: number) => { + const labelDomElement = this.label.getDomElement(); + labelDomElement.css({ + 'left': seekPositionPercentage + '%', + 'transform': null, + }); + + // TODO: Re-test `requestAnimationFrame` to prevent forced synchronous layout calculation + if (!this.uiBoundingRect) { + this.uiBoundingRect = this.uiManager.getUI().getDomElement().get(0).getBoundingClientRect(); + } + this.label.ensureWithinBounds(this.uiBoundingRect); + }; + protected onSeekPreviewEvent(percentage: number, scrubbing: boolean) { let snappedMarker = this.timelineMarkersHandler && this.timelineMarkersHandler.getMarkerAtPosition(percentage); @@ -1043,9 +1062,7 @@ export class SeekBar extends Component { } if (this.label) { - this.label.getDomElement().css({ - 'left': seekPositionPercentage + '%', - }); + this.updateLabelPosition(seekPositionPercentage); } this.seekBarEvents.onSeekPreview.dispatch(this, { diff --git a/src/ts/components/seekbarlabel.ts b/src/ts/components/seekbarlabel.ts index 4673810c0..8c652bdb2 100644 --- a/src/ts/components/seekbarlabel.ts +++ b/src/ts/components/seekbarlabel.ts @@ -136,6 +136,34 @@ export class SeekBarLabel extends Container { } }; + public ensureWithinBounds = (bounds: DOMRect) => { + // TODO move into CSS + const overflowMargin = 8; + + const labelBounding = this.container.getDomElement().get(0).getBoundingClientRect(); + + let preventOverflowOffset = 0; + if (labelBounding.right + overflowMargin > bounds.right) { + preventOverflowOffset = labelBounding.right - bounds.right + overflowMargin; + } else if (labelBounding.left - overflowMargin < bounds.left) { + preventOverflowOffset = labelBounding.left - bounds.left - overflowMargin; + } + + if (preventOverflowOffset !== 0) { + this.getDomElement().css({ + transform: `translateX(${-preventOverflowOffset}px)`, + }); + + this.caret.getDomElement().css({ + transform: `translateX(${preventOverflowOffset}px)`, + }); + } else { + this.caret.getDomElement().css({ + transform: `translateX(${0}px)`, + }); + } + } + /** * Sets arbitrary text on the label. * @param text the text to show on the label From 5cff8872726c14f62fdc1a48ee559c16c6c65aa1 Mon Sep 17 00:00:00 2001 From: felix-hoc Date: Thu, 19 Dec 2024 11:19:45 +0100 Subject: [PATCH 03/11] Set label position in px fixes stuttering due to a mix of `left` and `transform` positioning --- src/ts/components/seekbar.ts | 31 +++++++++++++++---------------- src/ts/components/seekbarlabel.ts | 8 ++++++-- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/src/ts/components/seekbar.ts b/src/ts/components/seekbar.ts index 203387653..b7ec430b7 100644 --- a/src/ts/components/seekbar.ts +++ b/src/ts/components/seekbar.ts @@ -721,10 +721,13 @@ export class SeekBar extends Component { e.stopPropagation(); } - let targetPercentage = 100 * this.getOffset(e); + const offset = this.getOffset(e); + const targetPercentage = 100 * offset; + const seekPositionPx = offset * this.seekBar.width(); + this.setSeekPosition(targetPercentage); this.setPlaybackPosition(targetPercentage); - this.onSeekPreviewEvent(targetPercentage, true); + this.onSeekPreviewEvent(targetPercentage, seekPositionPx, true); }; let mouseTouchUpHandler = (e: MouseEvent | TouchEvent) => { @@ -782,10 +785,12 @@ export class SeekBar extends Component { mouseTouchMoveHandler(e); } - let position = 100 * this.getOffset(e); - this.setSeekPosition(position); + const offset = this.getOffset(e); + const seekPositionPercentage = 100 * offset; + const seekPositionPx = offset * this.seekBar.width(); - this.onSeekPreviewEvent(position, false); + this.setSeekPosition(seekPositionPercentage); + this.onSeekPreviewEvent(seekPositionPercentage, seekPositionPx, false); if (this.hasLabel() && this.getLabel().isHidden()) { this.getLabel().show(); @@ -1025,21 +1030,15 @@ export class SeekBar extends Component { this.seekBarEvents.onSeek.dispatch(this); } - private updateLabelPosition = (seekPositionPercentage: number) => { - const labelDomElement = this.label.getDomElement(); - labelDomElement.css({ - 'left': seekPositionPercentage + '%', - 'transform': null, - }); - - // TODO: Re-test `requestAnimationFrame` to prevent forced synchronous layout calculation + private updateLabelPosition = (pixelPosition: number) => { if (!this.uiBoundingRect) { this.uiBoundingRect = this.uiManager.getUI().getDomElement().get(0).getBoundingClientRect(); } - this.label.ensureWithinBounds(this.uiBoundingRect); + + this.label.setPositionInBounds(pixelPosition, this.uiBoundingRect); }; - protected onSeekPreviewEvent(percentage: number, scrubbing: boolean) { + protected onSeekPreviewEvent(percentage: number, targetOffsetPx: number, scrubbing: boolean) { let snappedMarker = this.timelineMarkersHandler && this.timelineMarkersHandler.getMarkerAtPosition(percentage); let seekPositionPercentage = percentage; @@ -1062,7 +1061,7 @@ export class SeekBar extends Component { } if (this.label) { - this.updateLabelPosition(seekPositionPercentage); + this.updateLabelPosition(targetOffsetPx); } this.seekBarEvents.onSeekPreview.dispatch(this, { diff --git a/src/ts/components/seekbarlabel.ts b/src/ts/components/seekbarlabel.ts index 8c652bdb2..17550cbec 100644 --- a/src/ts/components/seekbarlabel.ts +++ b/src/ts/components/seekbarlabel.ts @@ -136,7 +136,11 @@ export class SeekBarLabel extends Container { } }; - public ensureWithinBounds = (bounds: DOMRect) => { + public setPositionInBounds(seekPositionPx: number, bounds: DOMRect) { + this.getDomElement().css({ + 'left': seekPositionPx + 'px', + }); + // TODO move into CSS const overflowMargin = 8; @@ -151,7 +155,7 @@ export class SeekBarLabel extends Container { if (preventOverflowOffset !== 0) { this.getDomElement().css({ - transform: `translateX(${-preventOverflowOffset}px)`, + left: seekPositionPx - preventOverflowOffset + 'px', }); this.caret.getDomElement().css({ From 236e5dc64752c172f3a0707e17ff966e6faa150e Mon Sep 17 00:00:00 2001 From: felix-hoc Date: Thu, 19 Dec 2024 11:20:07 +0100 Subject: [PATCH 04/11] Fix caret position --- src/scss/skin-modern/components/_seekbarlabel.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/scss/skin-modern/components/_seekbarlabel.scss b/src/scss/skin-modern/components/_seekbarlabel.scss index bf551bc07..d775f94b7 100644 --- a/src/scss/skin-modern/components/_seekbarlabel.scss +++ b/src/scss/skin-modern/components/_seekbarlabel.scss @@ -30,7 +30,7 @@ border-top-color: $color-primary; border-width: .5em; height: 0; - margin-left: -1em; + margin-left: -.5em; pointer-events: none; position: absolute; top: 100%; From 935380a41f7276b90e61091d49414cfa3b69d250 Mon Sep 17 00:00:00 2001 From: felix-hoc Date: Thu, 19 Dec 2024 11:21:14 +0100 Subject: [PATCH 05/11] Fix unit tests --- spec/components/seekbar.spec.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/components/seekbar.spec.ts b/spec/components/seekbar.spec.ts index 3a09d6888..d09711fcb 100644 --- a/spec/components/seekbar.spec.ts +++ b/spec/components/seekbar.spec.ts @@ -114,7 +114,7 @@ describe('SeekBar', () => { jest.spyOn(playerMock, 'getSeekableRange').mockImplementation(() => ({start: 26, end: 30})); - seekbar['onSeekPreviewEvent'](40, true) + seekbar['onSeekPreviewEvent'](40, 100, true); playerMock.eventEmitter.fireSegmentRequestFinished(); @@ -127,19 +127,19 @@ describe('SeekBar', () => { it('will update the scrubber location after a successful segment request download and the user is not scrubbing', () => { jest.spyOn(playerMock, 'getSeekableRange').mockImplementation(() => ({start: 26, end: 30})); - seekbar['onSeekPreviewEvent'](18, false) + seekbar['onSeekPreviewEvent'](18, 100, false); playerMock.eventEmitter.fireSegmentRequestFinished(); expect(setPlaybackPositionSpy).toHaveBeenLastCalledWith(18); - expect(setBufferPositionSpy).toHaveBeenLastCalledWith(18) + expect(setBufferPositionSpy).toHaveBeenLastCalledWith(18); }); }); }); describe('group playback', () => { beforeEach(() => { - jest.spyOn(playerMock, 'getDuration').mockReturnValue(0) + jest.spyOn(playerMock, 'getDuration').mockReturnValue(0); seekbar.configure(playerMock, uiInstanceManagerMock); }); From 8b126377decad7400f99fe84cb67e222a5c5411b Mon Sep 17 00:00:00 2001 From: felix-hoc Date: Thu, 19 Dec 2024 16:18:38 +0100 Subject: [PATCH 06/11] Move overflow margin into CSS --- src/scss/skin-modern/components/_seekbarlabel.scss | 3 +++ src/ts/components/seekbarlabel.ts | 14 ++++++-------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/scss/skin-modern/components/_seekbarlabel.scss b/src/scss/skin-modern/components/_seekbarlabel.scss index d775f94b7..767fbe2b9 100644 --- a/src/scss/skin-modern/components/_seekbarlabel.scss +++ b/src/scss/skin-modern/components/_seekbarlabel.scss @@ -21,6 +21,9 @@ > .#{$prefix}-container-wrapper { @extend %center-on-left-edge; + + padding-left: 1em; + padding-right: 1em; } // bottom arrow from http://www.cssarrowplease.com/ diff --git a/src/ts/components/seekbarlabel.ts b/src/ts/components/seekbarlabel.ts index 17550cbec..4409f943e 100644 --- a/src/ts/components/seekbarlabel.ts +++ b/src/ts/components/seekbarlabel.ts @@ -141,16 +141,14 @@ export class SeekBarLabel extends Container { 'left': seekPositionPx + 'px', }); - // TODO move into CSS - const overflowMargin = 8; - - const labelBounding = this.container.getDomElement().get(0).getBoundingClientRect(); + // Check parent container as it has a padding that needs to be considered + const labelBounding = this.container.getDomElement().get(0).parentElement.getBoundingClientRect(); let preventOverflowOffset = 0; - if (labelBounding.right + overflowMargin > bounds.right) { - preventOverflowOffset = labelBounding.right - bounds.right + overflowMargin; - } else if (labelBounding.left - overflowMargin < bounds.left) { - preventOverflowOffset = labelBounding.left - bounds.left - overflowMargin; + if (labelBounding.right > bounds.right) { + preventOverflowOffset = labelBounding.right - bounds.right; + } else if (labelBounding.left < bounds.left) { + preventOverflowOffset = labelBounding.left - bounds.left; } if (preventOverflowOffset !== 0) { From 700a08c2b59f6d98bf99234af663b48b592c62c4 Mon Sep 17 00:00:00 2001 From: felix-hoc Date: Thu, 19 Dec 2024 16:20:49 +0100 Subject: [PATCH 07/11] Change CSS setters --- src/ts/components/seekbarlabel.ts | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/ts/components/seekbarlabel.ts b/src/ts/components/seekbarlabel.ts index 4409f943e..1630e5cf5 100644 --- a/src/ts/components/seekbarlabel.ts +++ b/src/ts/components/seekbarlabel.ts @@ -152,17 +152,11 @@ export class SeekBarLabel extends Container { } if (preventOverflowOffset !== 0) { - this.getDomElement().css({ - left: seekPositionPx - preventOverflowOffset + 'px', - }); + this.getDomElement().css('left', seekPositionPx - preventOverflowOffset + 'px'); - this.caret.getDomElement().css({ - transform: `translateX(${preventOverflowOffset}px)`, - }); + this.caret.getDomElement().css('transform', `translateX(${preventOverflowOffset}px)`); } else { - this.caret.getDomElement().css({ - transform: `translateX(${0}px)`, - }); + this.caret.getDomElement().css('transform', null); } } From 77aec0f9b4cf7c4ee258977eb8e78521b1e87c6e Mon Sep 17 00:00:00 2001 From: felix-hoc Date: Thu, 19 Dec 2024 16:38:03 +0100 Subject: [PATCH 08/11] Add changelog entry --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 004c7e5a3..883cde47f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,11 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/). +## [Unreleased] + +### Fixed +- Seek preview thumbnails exceeding the UI dimensions when default size is increased + ## [3.75.0] - 2024-11-19 ### Fixed From e065273fc6572cc1759abb2e0514f60cd0af82b4 Mon Sep 17 00:00:00 2001 From: Andreas Kogler Date: Thu, 19 Dec 2024 19:08:24 +0100 Subject: [PATCH 09/11] Add unit-tests for seekbar level overflows --- spec/components/seekbarlabel.spec.ts | 93 ++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/spec/components/seekbarlabel.spec.ts b/spec/components/seekbarlabel.spec.ts index b68afde4f..89555de8a 100644 --- a/spec/components/seekbarlabel.spec.ts +++ b/spec/components/seekbarlabel.spec.ts @@ -94,4 +94,97 @@ describe('SeekBarLabel', () => { }); }); }); + + describe("calculates correct values for thumbnail positioning", () => { + const uiContainerBoundingRect = { + x: 200, + y: 150, + width: 1600, + height: 900, + top: 150, + right: 1800, + bottom: 1050, + left: 200, + } as DOMRect; + + let containerGetDomElementMock: () => jest.Mocked; + let caretGetDomElementMock: () => jest.Mocked; + + beforeEach(() => { + containerGetDomElementMock = jest + .fn() + .mockReturnValue(MockHelper.generateDOMMock()); + + containerGetDomElementMock().get = jest.fn().mockReturnValue({ + parentElement: jest.fn().mockReturnValue({ + getBoundingClientRect: jest.fn(), + }), + }); + + caretGetDomElementMock = jest.fn().mockReturnValue(MockHelper.generateDOMMock()); + + seekbarLabel["container"].getDomElement = containerGetDomElementMock; + seekbarLabel["caret"].getDomElement = caretGetDomElementMock; + }); + + it("when thumbnail within UI container bounds", () => { + const labelRect = { + x: 400, + y: 700, + width: 200, + height: 120, + top: 700, + right: 600, + bottom: 820, + left: 400, + } as DOMRect; + + containerGetDomElementMock().get(0).parentElement!.getBoundingClientRect = + jest.fn().mockReturnValue(labelRect); + + seekbarLabel.setPositionInBounds(100, uiContainerBoundingRect); + + expect(caretGetDomElementMock().css).toHaveBeenCalledWith('transform', null); + }); + + it("when thumbnail would overflow UI container leftside", () => { + const labelRect = { + x: 180, + y: 700, + width: 200, + height: 120, + top: 700, + right: 380, + bottom: 820, + left: 180, + } as DOMRect; + + containerGetDomElementMock().get(0).parentElement!.getBoundingClientRect = + jest.fn().mockReturnValue(labelRect); + + seekbarLabel.setPositionInBounds(100, uiContainerBoundingRect); + + expect(seekbarLabel.getDomElement().css).toHaveBeenCalledWith('left', '120px'); + }); + + it("when thumbnail would overflow UI container rightside", () => { + const labelRect = { + x: 1650, + y: 700, + width: 200, + height: 120, + top: 700, + right: 1850, + bottom: 820, + left: 1650, + } as DOMRect; + + containerGetDomElementMock().get(0).parentElement!.getBoundingClientRect = + jest.fn().mockReturnValue(labelRect); + + seekbarLabel.setPositionInBounds(100, uiContainerBoundingRect); + + expect(seekbarLabel.getDomElement().css).toHaveBeenCalledWith('left', '50px'); + }); + }); }); From 91802045364ec74c2d8b39b68b455e58abcf9c8f Mon Sep 17 00:00:00 2001 From: Andreas Kogler Date: Thu, 19 Dec 2024 19:08:49 +0100 Subject: [PATCH 10/11] Add missing `DOM` import --- spec/components/seekbarlabel.spec.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/components/seekbarlabel.spec.ts b/spec/components/seekbarlabel.spec.ts index 89555de8a..174e17d6c 100644 --- a/spec/components/seekbarlabel.spec.ts +++ b/spec/components/seekbarlabel.spec.ts @@ -2,6 +2,7 @@ import { MockHelper, TestingPlayerAPI } from '../helper/MockHelper'; import { UIInstanceManager } from '../../src/ts/uimanager'; import { SeekBarLabel } from '../../src/ts/components/seekbarlabel'; import { SeekPreviewEventArgs } from '../../src/ts/components/seekbar'; +import { DOM } from '../../src/ts/dom'; let playerMock: TestingPlayerAPI; let uiInstanceManagerMock: UIInstanceManager; From 62577052c9e71e3385e290722b0889e73f7d42ad Mon Sep 17 00:00:00 2001 From: felix-hoc Date: Fri, 20 Dec 2024 09:56:22 +0100 Subject: [PATCH 11/11] Adjust invocation to match --- src/ts/components/seekbarlabel.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/ts/components/seekbarlabel.ts b/src/ts/components/seekbarlabel.ts index 1630e5cf5..b0aea018b 100644 --- a/src/ts/components/seekbarlabel.ts +++ b/src/ts/components/seekbarlabel.ts @@ -137,9 +137,7 @@ export class SeekBarLabel extends Container { }; public setPositionInBounds(seekPositionPx: number, bounds: DOMRect) { - this.getDomElement().css({ - 'left': seekPositionPx + 'px', - }); + this.getDomElement().css('left', seekPositionPx + 'px'); // Check parent container as it has a padding that needs to be considered const labelBounding = this.container.getDomElement().get(0).parentElement.getBoundingClientRect();