Skip to content

Commit

Permalink
🐛 Fix video pausing itself after second layout (#32220)
Browse files Browse the repository at this point in the history
Fixes #32173

1. When closing a container lightbox, the `<amp-youtube>` element is unlaid out. 
2. When opening the lightbox a second time, the `<amp-youtube>` element is laid out again.

The `VideoManager` then incorrectly stores two entries. When playing for a second time it will `pauseOtherVideos()`, incorrectly pausing itself—so the video never plays.

The reused `VideoEntry` maintains its listeners at the e.g. `<amp-video>` level, so they continue to work.
  • Loading branch information
alanorozco committed Jan 26, 2021
1 parent f41acfb commit beac005
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 5 deletions.
26 changes: 21 additions & 5 deletions src/service/video-manager-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,12 @@ export class VideoManager {
if (!video.supportsPlatform()) {
return;
}

if (this.getEntryOrNull_(video)) {
// already registered
return;
}

if (!this.viewportObserver_) {
const viewportCallback = (
/** @type {!Array<!IntersectionObserverEntry>} */ records
Expand Down Expand Up @@ -270,25 +276,35 @@ export class VideoManager {

/**
* Returns the entry in the video manager corresponding to the video or
* element provided
* element provided, or null if unavailable.
* @param {!../video-interface.VideoOrBaseElementDef|!Element} videoOrElement
* @return {VideoEntry} entry
* @return {?VideoEntry} entry
*/
getEntry_(videoOrElement) {
getEntryOrNull_(videoOrElement) {
if (isEntryFor(this.lastFoundEntry_, videoOrElement)) {
return this.lastFoundEntry_;
}

for (let i = 0; i < this.entries_.length; i++) {
for (let i = 0; this.entries_ && i < this.entries_.length; i++) {
const entry = this.entries_[i];
if (isEntryFor(entry, videoOrElement)) {
this.lastFoundEntry_ = entry;
return entry;
}
}

return null;
}

/**
* Returns the entry in the video manager corresponding to the video or
* element provided
* @param {!../video-interface.VideoOrBaseElementDef|!Element} videoOrElement
* @return {VideoEntry} entry
*/
getEntry_(videoOrElement) {
return devAssert(
null,
this.getEntryOrNull_(videoOrElement),
'%s not registered to VideoManager',
videoOrElement.element || videoOrElement
);
Expand Down
7 changes: 7 additions & 0 deletions test/integration/test-video-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,13 @@ describe
let video;
let impl;

it('should not duplicate entries if laid out twice', async () => {
videoManager.register(impl);
expect(videoManager.entries_).to.have.length(1);
videoManager.register(impl);
expect(videoManager.entries_).to.have.length(1);
});

it('should receive i-amphtml-video-interface class when registered', () => {
const expectedClass = 'i-amphtml-video-interface';
expect(toArray(video.classList)).to.not.contain(expectedClass);
Expand Down

0 comments on commit beac005

Please sign in to comment.