From 2ae734ada2bc671d42ad8500207031576826fe56 Mon Sep 17 00:00:00 2001 From: David HM Morgan <37144605+david-hm-morgan@users.noreply.github.com> Date: Thu, 17 Nov 2022 13:15:24 +0000 Subject: [PATCH 1/3] add failing UT --- test/text/ui_text_displayer_unit.js | 76 +++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/test/text/ui_text_displayer_unit.js b/test/text/ui_text_displayer_unit.js index 306cd45f7c..5b3e0678f3 100644 --- a/test/text/ui_text_displayer_unit.js +++ b/test/text/ui_text_displayer_unit.js @@ -461,4 +461,80 @@ describe('UITextDisplayer', () => { expect(Object.keys(cueCssObj)).not.toContain('left'); } }); + + it('does not lose second item in a region', () => { + 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 have identical nested. + const cue1 = new shaka.text.Cue(168, 181.84, ''); + cue1.nestedCues = [ + new shaka.text.Cue(168, 181.84, ''), + ]; + cue1.region = cueRegion; + + const nested1 = new shaka.text.Cue(168, 170.92, ''); + nested1.nestedCues = [new shaka.text.Cue(0, 170.92, + 'Emo look. I mean listen.')]; + + const nested2 = new shaka.text.Cue(172, 174.84, ''); + nested2.nestedCues = [new shaka.text.Cue(172, 174.84, + 'You have to learn to listen.')]; + + const nested3 = new shaka.text.Cue(175.84, 177.64, ''); + nested3.nestedCues = [new shaka.text.Cue(175.84, 177.64, + 'This is not some game.')]; + + const nested4 = new shaka.text.Cue(177.68, 181.84, ''); + nested4.nestedCues = [new shaka.text.Cue(177.68, 181.84, + 'You - I mean we - we could easily die out here.')]; + + cue1.nestedCues[0].nestedCues = [nested1, nested2, nested3, nested4]; + + video.currentTime = 170; + textDisplayer.setTextVisibility(true); + textDisplayer.append([cue1]); + updateCaptions(); + + /** @type {Element} */ + const textContainer = videoContainer.querySelector('.shaka-text-container'); + let captions = textContainer.querySelectorAll('div'); + expect(captions.length).toBe(1); + let allRegionElements = textContainer.querySelectorAll( + '.shaka-text-region'); + // Verify that the nested cues are all attached to a single region element. + expect(allRegionElements.length).toBe(1); + + // Advance time to where there is none to show + video.currentTime = 171; + updateCaptions(); + + allRegionElements = textContainer.querySelectorAll( + '.shaka-text-region'); + expect(allRegionElements.length).toBe(1); + + // Advance time to where there is something to show + video.currentTime = 173; + updateCaptions(); + + allRegionElements = textContainer.querySelectorAll( + '.shaka-text-region'); + expect(allRegionElements.length).toBe(1); + + captions = textContainer.querySelectorAll('div'); + + expect(captions.length).toBe(1); + expect(captions[0].textContent).toBe('You have to learn to listen.'); + + allRegionElements = textContainer.querySelectorAll( + '.shaka-text-region'); + expect(allRegionElements.length).toBe(1); + }); }); From 1aaefc20ddb586dc33f26619109dbe6b0b687978 Mon Sep 17 00:00:00 2001 From: David HM Morgan <37144605+david-hm-morgan@users.noreply.github.com> Date: Thu, 17 Nov 2022 13:16:29 +0000 Subject: [PATCH 2/3] [fix] to avoid removing regions they could be imminently in use --- lib/text/ui_text_displayer.js | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/lib/text/ui_text_displayer.js b/lib/text/ui_text_displayer.js index f88d9e49fa..b82253b07e 100644 --- a/lib/text/ui_text_displayer.js +++ b/lib/text/ui_text_displayer.js @@ -280,7 +280,10 @@ shaka.text.UITextDisplayer = class { 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) { + // But avoid removing regions here due to risk of missing out + // subsequent captions. + if (element.parentElement && + !element.id.includes('shaka-text-region')) { element.parentElement.removeChild(element); } } @@ -578,7 +581,7 @@ shaka.text.UITextDisplayer = class { // (for vertical growing left) is aligned at the line. // TODO: Implement line alignment with line number. // TODO: Implement lineAlignment of 'CENTER'. - if (cue.line != null) { + if (cue.line) { if (cue.lineInterpretation == Cue.lineInterpretation.PERCENTAGE) { // When setting absolute positioning, you need to set x/y/width/height // so the element is positioned correctly. Set these as default and @@ -591,21 +594,21 @@ shaka.text.UITextDisplayer = class { if (cue.lineAlign == Cue.lineAlign.START) { style.top = cue.line + '%'; } else if (cue.lineAlign == Cue.lineAlign.END) { - style.bottom = (100 - cue.line) + '%'; + style.bottom = cue.line + '%'; } } else if (cue.writingMode == Cue.writingMode.VERTICAL_LEFT_TO_RIGHT) { style.height = '100%'; if (cue.lineAlign == Cue.lineAlign.START) { style.left = cue.line + '%'; } else if (cue.lineAlign == Cue.lineAlign.END) { - style.right = (100 - cue.line) + '%'; + style.right = cue.line + '%'; } } else { style.height = '100%'; if (cue.lineAlign == Cue.lineAlign.START) { style.right = cue.line + '%'; } else if (cue.lineAlign == Cue.lineAlign.END) { - style.left = (100 - cue.line) + '%'; + style.left = cue.line + '%'; } } } @@ -615,7 +618,7 @@ shaka.text.UITextDisplayer = class { // The position defines the indent of the text container in the // direction defined by the writing direction. - if (cue.position != null) { + if (cue.position) { if (cue.writingMode == Cue.writingMode.HORIZONTAL_TOP_TO_BOTTOM) { style.paddingLeft = cue.position; } else { From dd00429aa9aa33d09b19cf2d7c17e08445de4e71 Mon Sep 17 00:00:00 2001 From: David HM Morgan <37144605+david-hm-morgan@users.noreply.github.com> Date: Thu, 17 Nov 2022 13:20:10 +0000 Subject: [PATCH 3/3] undo version differences --- lib/text/ui_text_displayer.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/text/ui_text_displayer.js b/lib/text/ui_text_displayer.js index b82253b07e..bcdd6baea3 100644 --- a/lib/text/ui_text_displayer.js +++ b/lib/text/ui_text_displayer.js @@ -581,7 +581,7 @@ shaka.text.UITextDisplayer = class { // (for vertical growing left) is aligned at the line. // TODO: Implement line alignment with line number. // TODO: Implement lineAlignment of 'CENTER'. - if (cue.line) { + if (cue.line != null) { if (cue.lineInterpretation == Cue.lineInterpretation.PERCENTAGE) { // When setting absolute positioning, you need to set x/y/width/height // so the element is positioned correctly. Set these as default and @@ -594,21 +594,21 @@ shaka.text.UITextDisplayer = class { if (cue.lineAlign == Cue.lineAlign.START) { style.top = cue.line + '%'; } else if (cue.lineAlign == Cue.lineAlign.END) { - style.bottom = cue.line + '%'; + style.bottom = (100 - cue.line) + '%'; } } else if (cue.writingMode == Cue.writingMode.VERTICAL_LEFT_TO_RIGHT) { style.height = '100%'; if (cue.lineAlign == Cue.lineAlign.START) { style.left = cue.line + '%'; } else if (cue.lineAlign == Cue.lineAlign.END) { - style.right = cue.line + '%'; + style.right = (100 - cue.line) + '%'; } } else { style.height = '100%'; if (cue.lineAlign == Cue.lineAlign.START) { style.right = cue.line + '%'; } else if (cue.lineAlign == Cue.lineAlign.END) { - style.left = cue.line + '%'; + style.left = (100 - cue.line) + '%'; } } } @@ -618,7 +618,7 @@ shaka.text.UITextDisplayer = class { // The position defines the indent of the text container in the // direction defined by the writing direction. - if (cue.position) { + if (cue.position != null) { if (cue.writingMode == Cue.writingMode.HORIZONTAL_TOP_TO_BOTTOM) { style.paddingLeft = cue.position; } else {