From b02ff27017db362bb6562fdd2ac14cdf27005cb2 Mon Sep 17 00:00:00 2001 From: Theodore Abshire Date: Sat, 2 Oct 2021 14:58:27 -0700 Subject: [PATCH] fix: Fixes updating of nested cues Previously, we added support for respecting the time constraints of nested cues. However, the UI text displayer did not take the time constraints of nested cues into account when determining when and how to update the cues. This changes the UI text displayer to also do that. Issue #3524 Issue #3643 Change-Id: I6b643f2aa21f367a8e40a8aca2ebb62492c071c2 --- lib/text/ui_text_displayer.js | 178 ++++++++++++++++++++-------- test/text/ui_text_displayer_unit.js | 63 ++++++++++ 2 files changed, 189 insertions(+), 52 deletions(-) diff --git a/lib/text/ui_text_displayer.js b/lib/text/ui_text_displayer.js index 76e28021be..051ea1b16d 100644 --- a/lib/text/ui_text_displayer.js +++ b/lib/text/ui_text_displayer.js @@ -73,7 +73,14 @@ shaka.text.UITextDisplayer = class { this.updateCaptions_(); }).tickEvery(updatePeriod); - /** private {Map.} */ + /** + * Maps cues to cue elements. Specifically points out the wrapper element of + * the cue (e.g. the HTML element to put nested cues inside). + * @private {Map.} + */ this.currentCuesMap_ = new Map(); /** @private {shaka.util.EventManager} */ @@ -185,68 +192,143 @@ shaka.text.UITextDisplayer = class { } /** - * Display the current captions. - * @param {boolean=} forceUpdate + * @param {!Array.} cues + * @param {?HTMLElement} container + * @param {number} currentTime + * @param {boolean} isNested * @private */ - updateCaptions_(forceUpdate = false) { - const currentTime = this.video_.currentTime; + updateCuesRecursive_(cues, container, currentTime, isNested) { + // Set to true if the cues have changed in some way, which will require + // DOM changes. E.g. if a cue was added or removed. + let updateDOM = false; + /** + * The elements to remove from the DOM. + * Some of these elements may be added back again, if their corresponding + * cue is in toPlant. + * These elements are only removed if updateDOM is true. + * @type {!Array.} + */ + const toUproot = []; + /** + * The cues whose corresponding elements should be in the DOM. + * Some of these might be new, some might have been displayed beforehand. + * These will only be added if updateDOM is true. + * @type {!Array.} + */ + const toPlant = []; + for (const cue of cues) { + let cueRegistry = this.currentCuesMap_.get(cue); + const shouldBeDisplayed = + cue.startTime <= currentTime && cue.endTime > currentTime; + + if (cueRegistry) { + // If the cues are replanted, all existing cues should be uprooted, + // even ones which are going to be planted again. + toUproot.push(cueRegistry.cueElement); + + // If the cue should not be displayed, remove it entirely. + if (!shouldBeDisplayed) { + // Since something has to be removed, we will need to update the DOM. + updateDOM = true; + this.currentCuesMap_.delete(cue); + cueRegistry = null; + } + } - // Return true if the cue should be displayed at the current time point. - const shouldCueBeDisplayed = (cue) => { - return this.cues_.includes(cue) && this.isTextVisible_ && - cue.startTime <= currentTime && cue.endTime > currentTime; - }; + if (shouldBeDisplayed) { + toPlant.push(cue); + if (!cueRegistry) { + // The cue has to be made! + this.createCue_(cue, isNested); + cueRegistry = this.currentCuesMap_.get(cue); + updateDOM = true; + } + } - // For each cue in the current cues map, if the cue's end time has passed, - // remove the entry from the map, and remove the captions from the page. - for (const cue of this.currentCuesMap_.keys()) { - if (!shouldCueBeDisplayed(cue) || forceUpdate) { - const captions = this.currentCuesMap_.get(cue); - this.textContainer_.removeChild(captions); - this.currentCuesMap_.delete(cue); + // Recursively check the nested cues, to see if they need to be added or + // removed. + if (cue.nestedCues.length > 0) { + this.updateCuesRecursive_( + cue.nestedCues, cueRegistry ? cueRegistry.wrapper : null, + currentTime, /* isNested= */ true); } } - - // Sometimes we don't remove a cue element correctly. So check all the - // child nodes and remove any that don't have an associated cue. - const expectedChildren = new Set(this.currentCuesMap_.values()); - for (const child of Array.from(this.textContainer_.childNodes)) { - if (!expectedChildren.has(child)) { - this.textContainer_.removeChild(child); + // Note that the container might be null, if this is recursing over the + // children of a cue that is not displayed. This can happen if the cue was + // just removed, but its nested cues still need to be removed from + // this.currentCuesMap_. + if (updateDOM && container) { + for (const cueElement of toUproot) { + container.removeChild(cueElement); + } + toPlant.sort((a, b) => { + if (a.startTime != b.startTime) { + return a.startTime - b.startTime; + } else { + return a.endTime - b.endTime; + } + }); + for (const cue of toPlant) { + const cueRegistry = this.currentCuesMap_.get(cue); + goog.asserts.assert(cueRegistry, 'cueRegistry should exist.'); + container.appendChild(cueRegistry.cueElement); } } + } - // Get the current cues that should be added to display. If the cue is not - // being displayed already, add it to the map, and add the captions onto the - // page. - const currentCues = this.cues_.filter((cue) => { - return shouldCueBeDisplayed(cue) && !this.currentCuesMap_.has(cue); - }).sort((a, b) => { - if (a.startTime != b.startTime) { - return a.startTime - b.startTime; - } else { - return a.endTime - b.endTime; + /** + * Display the current captions. + * @param {boolean=} forceUpdate + * @private + */ + updateCaptions_(forceUpdate = false) { + if (!this.textContainer_) { + return; + } + + const currentTime = this.video_.currentTime; + if (!this.isTextVisible_ || forceUpdate) { + if (this.currentCuesMap_.size > 0) { + // Clear away any existing cues. + shaka.util.Dom.removeAllChildren(this.textContainer_); + this.currentCuesMap_.clear(); + } + } + if (this.isTextVisible_) { + // Log currently attached cue elements for verification, later. + const previousCuesMap = new Map(); + for (const cue of this.currentCuesMap_.keys()) { + previousCuesMap.set(cue, this.currentCuesMap_.get(cue)); } - }); - for (const cue of currentCues) { - const cueElement = this.displayCue_( - this.textContainer_, cue, /* isNested= */ false); - this.currentCuesMap_.set(cue, cueElement); + // Update the cues. + this.updateCuesRecursive_( + this.cues_, this.textContainer_, currentTime, false); + + // Sometimes, things fail to remove. Check to make sure that there aren't + // any leftover cue elements attached to things. See #2076. + for (const cue of previousCuesMap.keys()) { + if (!this.currentCuesMap_.has(cue)) { + const cueElement = previousCuesMap.get(cue).cueElement; + const parentNode = cueElement.parentNode; + if (parentNode) { + // It wasn't removed successfully... + parentNode.removeChild(cueElement); + } + } + } } } /** - * Displays a cue + * Creates the object for a cue. * - * @param {Element} container * @param {!shaka.extern.Cue} cue * @param {boolean} isNested - * @return {!Element} the created captions element * @private */ - displayCue_(container, cue, isNested) { + createCue_(cue, isNested) { let type = isNested ? 'span' : 'div'; if (cue.lineBreak || cue.spacer) { if (cue.spacer) { @@ -273,15 +355,7 @@ shaka.text.UITextDisplayer = class { cueElement.appendChild(wrapper); } - const time = this.video_.currentTime; - for (const nestedCue of cue.nestedCues) { - if (nestedCue.startTime <= time && nestedCue.endTime >= time) { - this.displayCue_(wrapper, nestedCue, /* isNested= */ true); - } - } - - container.appendChild(cueElement); - return cueElement; + this.currentCuesMap_.set(cue, {cueElement, wrapper}); } /** diff --git a/test/text/ui_text_displayer_unit.js b/test/text/ui_text_displayer_unit.js index e51f2eb658..cf1d186343 100644 --- a/test/text/ui_text_displayer_unit.js +++ b/test/text/ui_text_displayer_unit.js @@ -320,4 +320,67 @@ describe('UITextDisplayer', () => { // duplicates. expect(captions.length).toBe(3); }); + + it('hides and shows nested cues at appropriate times', async () => { + const parentCue1 = new shaka.text.Cue(0, 100, ''); + const cue1 = new shaka.text.Cue(0, 50, 'One'); + parentCue1.nestedCues.push(cue1); + const cue2 = new shaka.text.Cue(25, 75, 'Two'); + parentCue1.nestedCues.push(cue2); + const cue3 = new shaka.text.Cue(50, 100, 'Three'); + parentCue1.nestedCues.push(cue3); + + const parentCue2 = new shaka.text.Cue(90, 190, ''); + const cue4 = new shaka.text.Cue(90, 130, 'Four'); + parentCue2.nestedCues.push(cue4); + + textDisplayer.setTextVisibility(true); + textDisplayer.append([parentCue1, parentCue2]); + + video.currentTime = 10; + await shaka.test.Util.delay(0.5); + /** @type {Element} */ + const textContainer = videoContainer.querySelector('.shaka-text-container'); + let parentCueElements = textContainer.querySelectorAll('div'); + + expect(parentCueElements.length).toBe(1); + expect(parentCueElements[0].textContent).toBe('One'); + + video.currentTime = 35; + await shaka.test.Util.delay(0.5); + parentCueElements = textContainer.querySelectorAll('div'); + expect(parentCueElements.length).toBe(1); + expect(parentCueElements[0].textContent).toBe('OneTwo'); + + video.currentTime = 60; + await shaka.test.Util.delay(0.5); + parentCueElements = textContainer.querySelectorAll('div'); + expect(parentCueElements.length).toBe(1); + expect(parentCueElements[0].textContent).toBe('TwoThree'); + + video.currentTime = 85; + await shaka.test.Util.delay(0.5); + parentCueElements = textContainer.querySelectorAll('div'); + expect(parentCueElements.length).toBe(1); + expect(parentCueElements[0].textContent).toBe('Three'); + + video.currentTime = 95; + await shaka.test.Util.delay(0.5); + parentCueElements = textContainer.querySelectorAll('div'); + expect(parentCueElements.length).toBe(2); + expect(parentCueElements[0].textContent).toBe('Three'); + expect(parentCueElements[1].textContent).toBe('Four'); + + video.currentTime = 105; + await shaka.test.Util.delay(0.5); + parentCueElements = textContainer.querySelectorAll('div'); + expect(parentCueElements.length).toBe(1); + expect(parentCueElements[0].textContent).toBe('Four'); + + video.currentTime = 150; + await shaka.test.Util.delay(0.5); + parentCueElements = textContainer.querySelectorAll('div'); + expect(parentCueElements.length).toBe(1); + expect(parentCueElements[0].textContent).toBe(''); + }); });