Skip to content

Commit

Permalink
🐛 Page layout improvements (ampproject#22369)
Browse files Browse the repository at this point in the history
* move markPageAsLoaded_ to buildCallback

* move mediaLayoutPromise declaration until it is needed

* change to a deferred pattern

* replace whenLoaded with CommonSignals.LOAD_END

* actually call media fn

* fix test in amp story to wait for page signals to end

* mark page as loaded right after media is loaded

* remove markpageasloaded

* Revert "remove markpageasloaded"

This reverts commit 7cf6f24.
  • Loading branch information
Enriqe authored May 28, 2019
1 parent 2f4630b commit 93fc3cb
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 26 deletions.
18 changes: 2 additions & 16 deletions extensions/amp-story/1.0/amp-story-page.js
Original file line number Diff line number Diff line change
Expand Up @@ -226,14 +226,6 @@ export class AmpStoryPage extends AMP.BaseElement {
/** @private @const {!../../../src/service/resources-impl.Resources} */
this.resources_ = Services.resourcesForDoc(getAmpdoc(this.win.document));

/** @private @const {!Promise} */
this.mediaLayoutPromise_ = this.waitForMediaLayout_();

/** @private @const {!Promise} */
this.pageLoadPromise_ = this.mediaLayoutPromise_.then(() => {
this.markPageAsLoaded_();
});

const deferred = new Deferred();

/** @private @const {!Promise<!MediaPool>} */
Expand Down Expand Up @@ -448,7 +440,7 @@ export class AmpStoryPage extends AMP.BaseElement {
);
return Promise.all([
this.beforeVisible(),
this.mediaLayoutPromise_,
this.waitForMediaLayout_(),
this.mediaPoolPromise_,
]);
}
Expand Down Expand Up @@ -502,8 +494,7 @@ export class AmpStoryPage extends AMP.BaseElement {
mediaEl.addEventListener('error', resolve, true /* useCapture */);
});
});

return Promise.all(mediaPromises);
return Promise.all(mediaPromises).then(() => this.markPageAsLoaded_());
}

/**
Expand Down Expand Up @@ -567,11 +558,6 @@ export class AmpStoryPage extends AMP.BaseElement {
});
}

/** @return {!Promise} */
whenLoaded() {
return this.pageLoadPromise_;
}

/** @private */
markPageAsLoaded_() {
dispatch(
Expand Down
9 changes: 6 additions & 3 deletions extensions/amp-story/1.0/amp-story.js
Original file line number Diff line number Diff line change
Expand Up @@ -1052,7 +1052,9 @@ export class AmpStory extends AMP.BaseElement {
: [this.pages_[0]];

const storyLoadPromise = Promise.all(
pagesToWaitFor.filter(page => !!page).map(page => page.whenLoaded())
pagesToWaitFor
.filter(page => !!page)
.map(page => page.element.signals().whenSignal(CommonSignals.LOAD_END))
);

return this.timer_
Expand Down Expand Up @@ -2214,8 +2216,9 @@ export class AmpStory extends AMP.BaseElement {
// Once the media pool is ready, registers and preloads the background
// audio, and then gets the swapped element from the DOM to mute/unmute/play
// it programmatically later.
this.activePage_
.whenLoaded()
this.activePage_.element
.signals()
.whenSignal(CommonSignals.LOAD_END)
.then(() => {
backgroundAudioEl = /** @type {!HTMLMediaElement} */ (backgroundAudioEl);
this.mediaPool_.register(backgroundAudioEl);
Expand Down
19 changes: 19 additions & 0 deletions extensions/amp-story/1.0/test/test-amp-story-page.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,25 @@ describes.realWin('amp-story-page', {amp: true}, env => {
});
});

it('should call waitForMedia after layoutCallback resolves', () => {
const spy = sandbox.spy(page, 'waitForMediaLayout_');
page.buildCallback();
return page.layoutCallback().then(() => {
expect(spy).to.have.been.calledOnce;
});
});

it('should mark page as loaded after media is loaded', () => {
const waitForMediaLayoutSpy = sandbox.spy(page, 'waitForMediaLayout_');
const markPageAsLoadedSpy = sandbox.spy(page, 'markPageAsLoaded_');
page.buildCallback();
return page.layoutCallback().then(() => {
expect(markPageAsLoadedSpy).to.have.been.calledAfter(
waitForMediaLayoutSpy
);
});
});

it('should start the animations if needed when state becomes active', () => {
// Adding an element that has to be animated.
const animatedEl = win.document.createElement('div');
Expand Down
21 changes: 14 additions & 7 deletions extensions/amp-story/1.0/test/test-amp-story.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {ActionTrust} from '../../../../src/action-constants';
import {AmpStory} from '../amp-story';
import {AmpStoryBookend} from '../bookend/amp-story-bookend';
import {AmpStoryConsent} from '../amp-story-consent';
import {CommonSignals} from '../../../../src/common-signals';
import {Keys} from '../../../../src/utils/key-codes';
import {LocalizationService} from '../../../../src/service/localization';
import {MediaType} from '../media-pool';
Expand Down Expand Up @@ -939,13 +940,19 @@ describes.realWin(
.resolves();

createPages(story.element, 2, ['cover', 'page-1']);

return story.layoutCallback().then(() => {
expect(story.backgroundAudioEl_).to.exist;
expect(story.backgroundAudioEl_.src).to.equal(src);
expect(registerStub).to.have.been.calledOnce;
expect(preloadStub).to.have.been.calledOnce;
});
story
.layoutCallback()
.then(() =>
story.activePage_.element
.signals()
.whenSignal(CommonSignals.LOAD_END)
)
.then(() => {
expect(story.backgroundAudioEl_).to.exist;
expect(story.backgroundAudioEl_.src).to.equal(src);
expect(registerStub).to.have.been.calledOnce;
expect(preloadStub).to.have.been.calledOnce;
});
});

it('should bless the media on unmute', () => {
Expand Down

0 comments on commit 93fc3cb

Please sign in to comment.