From 007441cac940c994cc5ca76452df9b856721d09b Mon Sep 17 00:00:00 2001 From: Joey Parrish Date: Tue, 16 Aug 2022 11:29:06 -0700 Subject: [PATCH 1/4] fix(text): Fix cue region rendering in UI Both TTML and VTT regions should be thought of as separate elements from the cue structures they contain. To this end, the UI text displayer now creates separate region elements to represent CueRegion objects, and the Cues attached to them nest inside those region elements in the DOM. Closes #4381 --- lib/text/ui_text_displayer.js | 96 ++++++++++++++++++++++++++++++----- 1 file changed, 82 insertions(+), 14 deletions(-) diff --git a/lib/text/ui_text_displayer.js b/lib/text/ui_text_displayer.js index 8ea77aa383..27229ee4f7 100644 --- a/lib/text/ui_text_displayer.js +++ b/lib/text/ui_text_displayer.js @@ -77,6 +77,7 @@ shaka.text.UITextDisplayer = class { * the cue (e.g. the HTML element to put nested cues inside). * @private {Map.} */ @@ -97,6 +98,9 @@ shaka.text.UITextDisplayer = class { }); this.resizeObserver_.observe(this.textContainer_); } + + /** @private {Map.} */ + this.regionElements_ = new Map(); } @@ -234,6 +238,11 @@ shaka.text.UITextDisplayer = class { // even ones which are going to be planted again. toUproot.push(cueRegistry.cueElement); + // Also uproot all displayed region elements. + if (cueRegistry.regionElement) { + toUproot.push(cueRegistry.regionElement); + } + // 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. @@ -269,7 +278,9 @@ shaka.text.UITextDisplayer = class { } if (updateDOM) { for (const cueElement of toUproot) { - container.removeChild(cueElement); + if (cueElement.parentElement) { + cueElement.parentElement.removeChild(cueElement); + } } toPlant.sort((a, b) => { if (a.startTime != b.startTime) { @@ -281,7 +292,12 @@ shaka.text.UITextDisplayer = class { for (const cue of toPlant) { const cueRegistry = this.currentCuesMap_.get(cue); goog.asserts.assert(cueRegistry, 'cueRegistry should exist.'); - container.appendChild(cueRegistry.cueElement); + if (cueRegistry.regionElement) { + container.appendChild(cueRegistry.regionElement); + cueRegistry.regionElement.appendChild(cueRegistry.cueElement); + } else { + container.appendChild(cueRegistry.cueElement); + } } } } @@ -303,6 +319,16 @@ shaka.text.UITextDisplayer = class { shaka.util.Dom.removeAllChildren(this.textContainer_); this.currentCuesMap_.clear(); } + if (this.regionElements_.size > 0) { + // Clear away any existing regions. + for (const regionElement of this.regionElements_.values()) { + shaka.util.Dom.removeAllChildren(regionElement); + if (regionElement.parentElement) { + regionElement.parentElement.removeChild(regionElement); + } + } + this.regionElements_.clear(); + } } if (this.isTextVisible_) { // Log currently attached cue elements for verification, later. @@ -332,6 +358,54 @@ shaka.text.UITextDisplayer = class { } } + /** + * Get or create a region element corresponding to the cue region. These are + * cached by ID. + * + * @param {!shaka.extern.Cue} cue + * @return {!HTMLElement} + * @private + */ + getRegionElement_(cue) { + const region = cue.region; + + if (this.regionElements_.has(region.id)) { + return this.regionElements_.get(region.id); + } + + const regionElement = shaka.util.Dom.createHTMLElement('span'); + + const percentageUnit = shaka.text.CueRegion.units.PERCENTAGE; + const heightUnit = region.heightUnits == percentageUnit ? '%' : 'px'; + const widthUnit = region.widthUnits == percentageUnit ? '%' : 'px'; + const viewportAnchorUnit = + region.viewportAnchorUnits == percentageUnit ? '%' : 'px'; + + regionElement.id = 'shaka-text-region---' + region.id; + regionElement.classList.add('shaka-text-region'); + + regionElement.style.height = region.height + heightUnit; + regionElement.style.width = region.width + widthUnit; + regionElement.style.position = 'absolute'; + regionElement.style.top = region.viewportAnchorY + viewportAnchorUnit; + regionElement.style.left = region.viewportAnchorX + viewportAnchorUnit; + + regionElement.style.display = 'flex'; + regionElement.style.flexDirection = 'column'; + regionElement.style.alignItems = 'center'; + + if (cue.displayAlign == shaka.text.Cue.displayAlign.BEFORE) { + regionElement.style.justifyContent = 'flex-start'; + } else if (cue.displayAlign == shaka.text.Cue.displayAlign.CENTER) { + regionElement.style.justifyContent = 'center'; + } else { + regionElement.style.justifyContent = 'flex-end'; + } + + this.regionElements_.set(region.id, regionElement); + return regionElement; + } + /** * Creates the object for a cue. * @@ -354,6 +428,11 @@ shaka.text.UITextDisplayer = class { this.setCaptionStyles_(cueElement, cue, parents, needWrapper); } + let regionElement = null; + if (cue.region && cue.region.id) { + regionElement = this.getRegionElement_(cue); + } + let wrapper = cueElement; if (needWrapper) { // Create a wrapper element which will serve to contain all children into @@ -365,7 +444,7 @@ shaka.text.UITextDisplayer = class { cueElement.appendChild(wrapper); } - this.currentCuesMap_.set(cue, {cueElement, wrapper}); + this.currentCuesMap_.set(cue, {cueElement, wrapper, regionElement}); } /** @@ -526,17 +605,6 @@ shaka.text.UITextDisplayer = class { } } } - } else if (cue.region && cue.region.id) { - const percentageUnit = shaka.text.CueRegion.units.PERCENTAGE; - const heightUnit = cue.region.heightUnits == percentageUnit ? '%' : 'px'; - const widthUnit = cue.region.widthUnits == percentageUnit ? '%' : 'px'; - const viewportAnchorUnit = - cue.region.viewportAnchorUnits == percentageUnit ? '%' : 'px'; - style.height = cue.region.height + heightUnit; - style.width = cue.region.width + widthUnit; - style.position = 'absolute'; - style.top = cue.region.viewportAnchorY + viewportAnchorUnit; - style.left = cue.region.viewportAnchorX + viewportAnchorUnit; } style.lineHeight = cue.lineHeight; From b3c321b608a001815911bbe25cce16bdcdc126b9 Mon Sep 17 00:00:00 2001 From: Joey Parrish Date: Tue, 16 Aug 2022 11:29:06 -0700 Subject: [PATCH 2/4] Add new test case for regions --- test/text/ui_text_displayer_unit.js | 65 +++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/test/text/ui_text_displayer_unit.js b/test/text/ui_text_displayer_unit.js index 007d6df5f0..306cd45f7c 100644 --- a/test/text/ui_text_displayer_unit.js +++ b/test/text/ui_text_displayer_unit.js @@ -50,6 +50,7 @@ describe('UITextDisplayer', () => { }); beforeEach(() => { + video.currentTime = 0; textDisplayer = new shaka.text.UITextDisplayer(video, videoContainer); }); @@ -396,4 +397,68 @@ describe('UITextDisplayer', () => { expect(parentCueElements.length).toBe(1); expect(parentCueElements[0].textContent).toBe(''); }); + + it('creates separate elements for cue regions', () => { + const cueRegion = new shaka.text.CueRegion(); + cueRegion.id = 'regionId'; + cueRegion.height = 80; + cueRegion.heightUnits = shaka.text.CueRegion.units.PERCENTAGE; + cueRegion.width = 80; + cueRegion.widthUnits = shaka.text.CueRegion.units.PERCENTAGE; + cueRegion.viewportAnchorX = 10; + cueRegion.viewportAnchorY = 10; + cueRegion.viewportAnchorUnits = shaka.text.CueRegion.units.PERCENTAGE; + + // These all attach to the same region, but only one region element should + // be created. + const cues = [ + new shaka.text.Cue(0, 100, ''), + new shaka.text.Cue(0, 100, ''), + new shaka.text.Cue(0, 100, ''), + ]; + for (const cue of cues) { + cue.displayAlign = shaka.text.Cue.displayAlign.CENTER; + cue.region = cueRegion; + } + + textDisplayer.setTextVisibility(true); + textDisplayer.append(cues); + updateCaptions(); + + const textContainer = videoContainer.querySelector('.shaka-text-container'); + const allRegionElements = textContainer.querySelectorAll( + '.shaka-text-region'); + + // Verify that the nested cues are all attached to a single region element. + expect(allRegionElements.length).toBe(1); + const regionElement = allRegionElements[0]; + const children = Array.from(regionElement.childNodes).filter( + (e) => e.nodeType == Node.ELEMENT_NODE); + expect(children.length).toBe(3); + + // Verify styles applied to the region element. + const regionCssObj = parseCssText(regionElement.style.cssText); + const expectRegionCssObj = { + 'position': 'absolute', + 'height': '80%', + 'width': '80%', + 'top': '10%', + 'left': '10%', + 'display': 'flex', + 'flex-direction': 'column', + 'align-items': 'center', + 'justify-content': 'center', + }; + expect(regionCssObj).toEqual(jasmine.objectContaining(expectRegionCssObj)); + + for (const caption of children) { + // Verify that styles applied to the nested cues _DO NOT_ include region + // placement. + const cueCssObj = parseCssText(caption.style.cssText); + expect(Object.keys(cueCssObj)).not.toContain('height'); + expect(Object.keys(cueCssObj)).not.toContain('width'); + expect(Object.keys(cueCssObj)).not.toContain('top'); + expect(Object.keys(cueCssObj)).not.toContain('left'); + } + }); }); From 812a3e822f4a076b5aba413beb5e417a35c8f7f3 Mon Sep 17 00:00:00 2001 From: Joey Parrish Date: Tue, 16 Aug 2022 15:30:28 -0700 Subject: [PATCH 3/4] Rename local var and add note about parent elements --- lib/text/ui_text_displayer.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/text/ui_text_displayer.js b/lib/text/ui_text_displayer.js index 27229ee4f7..02492435ae 100644 --- a/lib/text/ui_text_displayer.js +++ b/lib/text/ui_text_displayer.js @@ -277,9 +277,11 @@ shaka.text.UITextDisplayer = class { goog.asserts.assert(topCue == cue, 'Parent cues should be kept in order'); } if (updateDOM) { - for (const cueElement of toUproot) { - if (cueElement.parentElement) { - cueElement.parentElement.removeChild(cueElement); + for (const element of toUproot) { + // NOTE: Because we uproot shared region elements, too, we might hit an + // element here that has no parent because we've already processed it. + if (element.parentElement) { + element.parentElement.removeChild(element); } } toPlant.sort((a, b) => { From 2126f8dac600b74610b5f76c97aaace24bf7033d Mon Sep 17 00:00:00 2001 From: Joey Parrish Date: Tue, 16 Aug 2022 15:30:39 -0700 Subject: [PATCH 4/4] Simplify/clarify forced update logic --- lib/text/ui_text_displayer.js | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/lib/text/ui_text_displayer.js b/lib/text/ui_text_displayer.js index 02492435ae..d84e0d5cb6 100644 --- a/lib/text/ui_text_displayer.js +++ b/lib/text/ui_text_displayer.js @@ -316,21 +316,15 @@ shaka.text.UITextDisplayer = class { 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.regionElements_.size > 0) { - // Clear away any existing regions. - for (const regionElement of this.regionElements_.values()) { - shaka.util.Dom.removeAllChildren(regionElement); - if (regionElement.parentElement) { - regionElement.parentElement.removeChild(regionElement); - } - } - this.regionElements_.clear(); + // Remove child elements from all regions. + for (const regionElement of this.regionElements_.values()) { + shaka.util.Dom.removeAllChildren(regionElement); } + // Remove all top-level elements in the text container. + shaka.util.Dom.removeAllChildren(this.textContainer_); + // Clear the element maps. + this.currentCuesMap_.clear(); + this.regionElements_.clear(); } if (this.isTextVisible_) { // Log currently attached cue elements for verification, later.