Skip to content

Commit

Permalink
fix: Fix subtitles not added to DOM region (#4733)
Browse files Browse the repository at this point in the history
Reinstate previously removed region elements when next caption finds it
is not there: Detect the absence and ensure `updateDOM` is true so its
reinstated and the next caption can be shown.

Closes #4680

Co-authored-by: Joey Parrish <joeyparrish@users.noreply.github.com>
  • Loading branch information
david-hm-morgan and joeyparrish authored Dec 7, 2022
1 parent 67a5d56 commit 4081434
Show file tree
Hide file tree
Showing 2 changed files with 93 additions and 0 deletions.
17 changes: 17 additions & 0 deletions lib/text/ui_text_displayer.js
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,19 @@ shaka.text.UITextDisplayer = class {
this.isTextVisible_ = on;
}

/**
* @private
*/
isElementUnderTextContainer_(elemToCheck) {
while (elemToCheck != null) {
if (elemToCheck == this.textContainer_) {
return true;
}
elemToCheck = elemToCheck.parentElement;
}
return false;
}

/**
* @param {!Array.<!shaka.extern.Cue>} cues
* @param {!HTMLElement} container
Expand Down Expand Up @@ -260,6 +273,9 @@ shaka.text.UITextDisplayer = class {
cueRegistry = this.currentCuesMap_.get(cue);
wrapper = cueRegistry.wrapper;
updateDOM = true;
} else if (!this.isElementUnderTextContainer_(wrapper)) {
// We found that the wrapper needs to be in the DOM
updateDOM = true;
}
}

Expand All @@ -276,6 +292,7 @@ shaka.text.UITextDisplayer = class {
const topCue = parents.pop();
goog.asserts.assert(topCue == cue, 'Parent cues should be kept in order');
}

if (updateDOM) {
for (const element of toUproot) {
// NOTE: Because we uproot shared region elements, too, we might hit an
Expand Down
76 changes: 76 additions & 0 deletions test/text/ui_text_displayer_unit.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});

0 comments on commit 4081434

Please sign in to comment.