From 83c57a3b0fb461852149f6bdf4ec0fc9fc98326d Mon Sep 17 00:00:00 2001 From: Dermot Duffy Date: Sat, 11 Jun 2022 20:01:06 -0700 Subject: [PATCH 1/4] Fix malformed live carousel bug for some users. --- src/components/media-carousel.ts | 46 +++++++++++++++++++++++++++----- 1 file changed, 40 insertions(+), 6 deletions(-) diff --git a/src/components/media-carousel.ts b/src/components/media-carousel.ts index 414dd21a..ad0166da 100644 --- a/src/components/media-carousel.ts +++ b/src/components/media-carousel.ts @@ -18,6 +18,21 @@ const getEmptyImageSrc = (width: number, height: number) => `data:image/svg+xml,%3Csvg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 ${width} ${height}"%3E%3C/svg%3E`; export const IMG_EMPTY = getEmptyImageSrc(16, 9); +/** + * A note on carousel reinitialization: + * - The carousel needs to be reinitialized when slide sizes change, and slide + * sizes change when content is loaded (lazily or otherwise). Reinitializing + * the carousel when it's moving causes a jarring 'reset', so complexity is + * required here to reinitialize only when the carousel is stable (including + * on first media load, which may be after first carousel initialization). + * - When the carousel is dragged, it can be reinitialized via the settle event. + * - On very first load, or when the user has configured no slide transition + * effect it can be reinitialized on the media load itself as long as + * clickAllowed() confirms the carousel is otherwise stable. + * - Example bug when this reinitialization is not performed: + * https://github.com/dermotduffy/frigate-hass-card/issues/651 + */ + @customElement('frigate-card-media-carousel') export class FrigateCardMediaCarousel extends FrigateCardCarousel { // A "map" from slide number to MediaShowInfo object. @@ -26,6 +41,7 @@ export class FrigateCardMediaCarousel extends FrigateCardCarousel { protected _previousControlRef: Ref = createRef(); protected _titleControlRef: Ref = createRef(); protected _titleTimerID: number | null = null; + protected _needReInit = false; // This carousel may be resized by Lovelace resizes, window resizes, // fullscreen, etc. Always call the adaptive height handler when the size @@ -134,6 +150,22 @@ export class FrigateCardMediaCarousel extends FrigateCardCarousel { // Dispatch MediaShow events as the carousel is moved. carousel?.on('init', this._selectSlideMediaShowHandler.bind(this)); carousel?.on('select', this._selectSlideMediaShowHandler.bind(this)); + + carousel?.on('settle', this._reinitIfNecessary.bind(this)); + } + + /** + * Reinitialize the carousel if necessary. See 'A note on carousel + * reinitialization' above. + */ + protected _reinitIfNecessary(): void { + if (this._needReInit) { + //The original options are included here, although this should not be + // necessary (without including them, Safari ends up not having a looping + // live carousel). + this._carousel?.reInit(this._getOptions()); + this._needReInit = false; + } } /** @@ -237,12 +269,14 @@ export class FrigateCardMediaCarousel extends FrigateCardCarousel { // isValidMediaShowInfo is used to prevent saving media info that will be // rejected upstream (empty 1x1 images will be rejected here). if (mediaShowInfo && isValidMediaShowInfo(mediaShowInfo)) { - if (!Object.keys(this._mediaShowInfo).length) { - // The carousel will be malformed on Safari unless we re-init the - // carousel after the first media load. The original options are - // included here, although this should not be necessary (without - // including them, Safari ends up not having a looping live carousel). - this._carousel?.reInit(this._getOptions()); + // If this is the first media load for this slide, the carousel needs to + // reinitialized as the slide size has changed. See 'A note on carousel + // reinitialization' above. + if (!(slideIndex in this._mediaShowInfo)) { + this._needReInit = true; + if (this._carousel?.clickAllowed()) { + this._reinitIfNecessary(); + } } this._mediaShowInfo[slideIndex] = mediaShowInfo; if (this._carousel && this._carousel?.selectedScrollSnap() === slideIndex) { From b63fee9d8b8b2c9f560ea85504d971751f19fe66 Mon Sep 17 00:00:00 2001 From: Dermot Duffy Date: Sat, 11 Jun 2022 20:04:32 -0700 Subject: [PATCH 2/4] Micro comment spacing fix. --- src/components/media-carousel.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/media-carousel.ts b/src/components/media-carousel.ts index ad0166da..c2177937 100644 --- a/src/components/media-carousel.ts +++ b/src/components/media-carousel.ts @@ -160,7 +160,7 @@ export class FrigateCardMediaCarousel extends FrigateCardCarousel { */ protected _reinitIfNecessary(): void { if (this._needReInit) { - //The original options are included here, although this should not be + // The original options are included here, although this should not be // necessary (without including them, Safari ends up not having a looping // live carousel). this._carousel?.reInit(this._getOptions()); From 8d7df67ffa157a4edf2f0d09b1a5affa7804dedc Mon Sep 17 00:00:00 2001 From: Dermot Duffy Date: Mon, 13 Jun 2022 21:05:41 -0700 Subject: [PATCH 3/4] Use an IntersectionObserver to reinit the carousel. --- src/components/media-carousel.ts | 78 ++++++++++++++++---------------- 1 file changed, 38 insertions(+), 40 deletions(-) diff --git a/src/components/media-carousel.ts b/src/components/media-carousel.ts index c2177937..f4a7f36d 100644 --- a/src/components/media-carousel.ts +++ b/src/components/media-carousel.ts @@ -18,21 +18,6 @@ const getEmptyImageSrc = (width: number, height: number) => `data:image/svg+xml,%3Csvg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 ${width} ${height}"%3E%3C/svg%3E`; export const IMG_EMPTY = getEmptyImageSrc(16, 9); -/** - * A note on carousel reinitialization: - * - The carousel needs to be reinitialized when slide sizes change, and slide - * sizes change when content is loaded (lazily or otherwise). Reinitializing - * the carousel when it's moving causes a jarring 'reset', so complexity is - * required here to reinitialize only when the carousel is stable (including - * on first media load, which may be after first carousel initialization). - * - When the carousel is dragged, it can be reinitialized via the settle event. - * - On very first load, or when the user has configured no slide transition - * effect it can be reinitialized on the media load itself as long as - * clickAllowed() confirms the carousel is otherwise stable. - * - Example bug when this reinitialization is not performed: - * https://github.com/dermotduffy/frigate-hass-card/issues/651 - */ - @customElement('frigate-card-media-carousel') export class FrigateCardMediaCarousel extends FrigateCardCarousel { // A "map" from slide number to MediaShowInfo object. @@ -47,10 +32,14 @@ export class FrigateCardMediaCarousel extends FrigateCardCarousel { // fullscreen, etc. Always call the adaptive height handler when the size // changes. protected _resizeObserver: ResizeObserver; + protected _intersectionObserver: IntersectionObserver; constructor() { super(); this._resizeObserver = new ResizeObserver(this._adaptiveHeightHandler.bind(this)); + this._intersectionObserver = new IntersectionObserver( + this._intersectionHandler.bind(this), + ); } /** @@ -105,6 +94,7 @@ export class FrigateCardMediaCarousel extends FrigateCardCarousel { this.addEventListener('frigate-card:media-show', this._adaptiveHeightHandler); this.addEventListener('frigate-card:media-show', this._titleHandler); this._resizeObserver.observe(this); + this._intersectionObserver.observe(this); } /** @@ -117,6 +107,39 @@ export class FrigateCardMediaCarousel extends FrigateCardCarousel { this.removeEventListener('frigate-card:media-show', this._adaptiveHeightHandler); this.removeEventListener('frigate-card:media-show', this._titleHandler); this._resizeObserver.disconnect(); + this._intersectionObserver.disconnect(); + } + + /** + * Called when the carousel intersects with the viewport. + * @param entries The IntersectionObserverEntry entries (should be only 1). + */ + protected _intersectionHandler(entries: IntersectionObserverEntry[]): void { + /** + * - If the DOM that contains this carousel changes such that it causes + * slides to entirely appear/disappear (e.g. `display: none` or hidden), + * then the displayed slide sizes will significantly change and the + * carousel will need to be reinitialized. Without this, odd bugs may + * occur for some users in some circumstances causing the carousel to + * appear 'stuck'. + * - Example bug when this reinitialization is not performed: + * https://github.com/dermotduffy/frigate-hass-card/issues/651 + */ + + const reinit = (): void => { + this._carousel?.reInit(); + }; + + if (entries.some((entry) => entry.isIntersecting)) { + // For performance, run the reinit in idle cycles if the browser supports + // it, but only give it 400ms before running as it may otherwise be + // noticeable to the user. + if (window.requestIdleCallback !== undefined) { + window.requestIdleCallback(reinit, { timeout: 400 }); + } else { + reinit(); + } + } } protected _destroyCarousel(): void { @@ -150,22 +173,6 @@ export class FrigateCardMediaCarousel extends FrigateCardCarousel { // Dispatch MediaShow events as the carousel is moved. carousel?.on('init', this._selectSlideMediaShowHandler.bind(this)); carousel?.on('select', this._selectSlideMediaShowHandler.bind(this)); - - carousel?.on('settle', this._reinitIfNecessary.bind(this)); - } - - /** - * Reinitialize the carousel if necessary. See 'A note on carousel - * reinitialization' above. - */ - protected _reinitIfNecessary(): void { - if (this._needReInit) { - // The original options are included here, although this should not be - // necessary (without including them, Safari ends up not having a looping - // live carousel). - this._carousel?.reInit(this._getOptions()); - this._needReInit = false; - } } /** @@ -269,15 +276,6 @@ export class FrigateCardMediaCarousel extends FrigateCardCarousel { // isValidMediaShowInfo is used to prevent saving media info that will be // rejected upstream (empty 1x1 images will be rejected here). if (mediaShowInfo && isValidMediaShowInfo(mediaShowInfo)) { - // If this is the first media load for this slide, the carousel needs to - // reinitialized as the slide size has changed. See 'A note on carousel - // reinitialization' above. - if (!(slideIndex in this._mediaShowInfo)) { - this._needReInit = true; - if (this._carousel?.clickAllowed()) { - this._reinitIfNecessary(); - } - } this._mediaShowInfo[slideIndex] = mediaShowInfo; if (this._carousel && this._carousel?.selectedScrollSnap() === slideIndex) { dispatchExistingMediaShowInfoAsEvent(this, mediaShowInfo); From 08758fd9cbbd35744e035eda3edff28fda882dec Mon Sep 17 00:00:00 2001 From: Dermot Duffy Date: Mon, 13 Jun 2022 21:07:23 -0700 Subject: [PATCH 4/4] Micro fixes. --- src/components/media-carousel.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/components/media-carousel.ts b/src/components/media-carousel.ts index f4a7f36d..aba35b83 100644 --- a/src/components/media-carousel.ts +++ b/src/components/media-carousel.ts @@ -26,7 +26,6 @@ export class FrigateCardMediaCarousel extends FrigateCardCarousel { protected _previousControlRef: Ref = createRef(); protected _titleControlRef: Ref = createRef(); protected _titleTimerID: number | null = null; - protected _needReInit = false; // This carousel may be resized by Lovelace resizes, window resizes, // fullscreen, etc. Always call the adaptive height handler when the size @@ -126,7 +125,7 @@ export class FrigateCardMediaCarousel extends FrigateCardCarousel { * https://github.com/dermotduffy/frigate-hass-card/issues/651 */ - const reinit = (): void => { + const reInit = (): void => { this._carousel?.reInit(); }; @@ -135,9 +134,9 @@ export class FrigateCardMediaCarousel extends FrigateCardCarousel { // it, but only give it 400ms before running as it may otherwise be // noticeable to the user. if (window.requestIdleCallback !== undefined) { - window.requestIdleCallback(reinit, { timeout: 400 }); + window.requestIdleCallback(reInit, { timeout: 400 }); } else { - reinit(); + reInit(); } } }