Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass document to onDocumentReady callback #4302

Merged
merged 5 commits into from Aug 2, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions extensions/amp-form/0.1/amp-form.js
Expand Up @@ -280,8 +280,8 @@ function reportInputValidity(input) {
* @param {!Window} win
*/
function installSubmissionHandlers(win) {
onDocumentReady(win.document, () => {
toArray(win.document.forms).forEach(form => {
onDocumentReady(win.document, doc => {
toArray(doc.forms).forEach(form => {
new AmpForm(form);
});
});
Expand Down
8 changes: 4 additions & 4 deletions src/document-ready.js
Expand Up @@ -28,18 +28,18 @@ export function isDocumentReady(doc) {
/**
* Calls the callback when document is ready.
* @param {!Document} doc
* @param {!Function} callback
* @param {function(!Document)} callback
*/
export function onDocumentReady(doc, callback) {
let ready = isDocumentReady(doc);
if (ready) {
callback();
callback(doc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated to your change, but it bothers me that this is synchronous making the promise and callback versions behave differently...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

M̸̲͖̼̖̭̻̊̔̄͂͛̀̾́̐͡á̴̱̬̞̜̮͍̟̩̜̃̍̅̓͛y̶̢̘̩̱̮̥͙̳̣̑͊̅͊͗̍̎͝ H̭̩͎͙̬̭̻͈̻̅͋̆́̆̐̀͋̔̾e̺̗̤͓̜̝̭̓̎̐̀͊͂̊̔͌͟͝ Ẅ̢̯̬̘͓̩͙͕̼́̉͑̅̚͟h̷͖͉̳̠͔̠͌̋̅̾̉̑̅̚͠o̵̡̧̡͙͎̜̱̐̍̐̿̿́͒͐̂ W̵͚̳͔͇̝͍̙͎̤͈̓̎̈́̃͆ạ̵̢̯̩̑̑̍͂͜͝͡ͅi̷̢̢̨̗̞̼͖̠͂̔́̉̑͒̇̈͋͠ẗ̴̨̛̞̰̫̺̮̥͉̙̼̂͂̔̂͊̊s̷̛̫̦̠̝̥̬̀̓̔͛́͗͑͘͜͢͢ B͚͎̣̟̰͈̰͌́͐͘͘ę̖̦̩̱͔͎̤̊̊̑͊͌͒̾͘͟͠͞h̻͍͍̲̦̑͒̀̉̍ì̡̬̦̬̪͚̖̿̌̾̒͋̄͊̚͞n̡̧̛̮̝̰̣̥͓͓̅͗̀̆̾̒̎͗͟d̷̡͙̙̪̬̾͒͐͂̆ͅ t̡̨͉͔̻͐́͂̋̔̀͌͘̕͠h̵̲̟͉̞̝̮̞̝͓̼̅͂͊͆̽̄̔̇̕è̸̙̪̻̪̮̭̌͢͟͡͠͠ W̩̳̩͕̪̠̻̺̣̱͐̆͗̈́̄͂͒͆́ą̖̯͕̰͖̹̗͔͒͌͊͋̉̚͡ļ̩̺̗̠̯̐̓͗͑͘l̞̣͚͕̬̳̓̓̉̀͛̚͢ N̨̲̟͇̘̅̽́͊̀̓ē̘̺͉̗̪̝̪̫̋͗͆̌͛͑v̷̢̡̮̫͎̱͕̱̹̭̆͗̏̅́͗́̈́́͠e̵̹̮̗͈̹͂͂͆͗̀͊̊̕͠r̶̢̨͇̞̳̱͆̃̽̂̅̎́̑͡ L̗̩͕̠̼̅̆͗͆̃͞e̶̤̻̮̟̺̤̖̤̣̬͐̅͊̋͘a̴̢̺̫̯̻̫̺̲̔̌̾̃̾̐v̶̞̙̭̞̀͑̑̀̅͊͜͝ę̖͔̤̤͎͍͕̪̦͂̊̋͋̊̅͘ Ǫ̷͓̼̞͓̉͛̍̎͡ͅũ̶͉͙͖̞͉̺̻͒̏̑̊̍͘͘͞ŗ̶̡̠͓̩͚̱̒͛͌̋̌̎͐͜͝͝ͅ C̶̨͙̟͓̠̝̟̾̏̆̉̎̋̃̃͟͡͠ơ̴̤͕͉̟͉̬̲̪̦̪̈̂͆́͂́̆̿d̝̱̭͇̻̿̆̈̎́͌͆̊̊é̶̻͉͖̻̭̼̥̀̍̑̿̀͞b̴̧̤͎͎͎͇̗̈́̊̉̄͜͝ȧ̞̜͖̙͇̀͌̏̾͢s̵̨̪̞̭͎̲̜̥͍͍̀̄̏̒̐̐͗͘̕ḛ̸͎͖̠̼̲̫̺̰̅̽͊́̊̋̉̂͊͒͜

} else {
const readyListener = () => {
if (isDocumentReady(doc)) {
if (!ready) {
ready = true;
callback();
callback(doc);
}
doc.removeEventListener('readystatechange', readyListener);
}
Expand All @@ -51,7 +51,7 @@ export function onDocumentReady(doc, callback) {
/**
* Returns a promise that is resolved when document is ready.
* @param {!Document} doc
* @return {!Promise}
* @return {!Promise<!Document>}
*/
export function whenDocumentReady(doc) {
return new Promise(resolve => {
Expand Down
19 changes: 9 additions & 10 deletions src/service/performance-impl.js
Expand Up @@ -15,7 +15,7 @@
*/

import {documentInfoFor} from '../document-info';
import {onDocumentReady} from '../document-ready';
import {whenDocumentReady} from '../document-ready';
import {fromClass} from '../service';
import {loadPromise} from '../event-helper';
import {resourcesFor} from '../resources';
Expand Down Expand Up @@ -94,15 +94,14 @@ export class Performance {
this.isMessagingReady_ = false;

/** @private @const {!Promise} */
this.whenReadyToRetrieveResourcesPromise_ = new Promise(resolve => {
onDocumentReady(this.win.document, () => {
// We need to add a delay, since this can execute earlier
// than the onReady callback registered inside of `Resources`.
// Should definitely think of making `getResourcesInViewport` async.
timer.delay(resolve);
});
});

this.whenReadyToRetrieveResourcesPromise_ =
whenDocumentReady(this.win.document)
.then(() => {
// Two fold. First, resolve the promise to undefined.
// Second, causes a delay by introducing another async request
// (this `#then` block) so that Resources' onDocumentReady event
// is guaranteed to fire.
});
// Tick window.onload event.
loadPromise(win).then(() => {
this.tick('ol');
Expand Down
27 changes: 10 additions & 17 deletions src/service/viewport-impl.js
Expand Up @@ -23,7 +23,7 @@ import {getService} from '../service';
import {layoutRectLtwh} from '../layout-rect';
import {dev} from '../log';
import {numeric} from '../transition';
import {onDocumentReady} from '../document-ready';
import {onDocumentReady, whenDocumentReady} from '../document-ready';
import {platform} from '../platform';
import {px, setStyle, setStyles} from '../style';
import {timer} from '../timer';
Expand Down Expand Up @@ -212,9 +212,8 @@ export class Viewport {
* @param {number} paddingBottom
*/
updatePaddingBottom(paddingBottom) {
onDocumentReady(this.win_.document, () => {
this.win_.document.body.style.borderBottom =
`${paddingBottom}px solid transparent`;
onDocumentReady(this.win_.document, doc => {
doc.body.style.borderBottom = `${paddingBottom}px solid transparent`;
});
}

Expand Down Expand Up @@ -906,13 +905,9 @@ export class ViewportBindingNaturalIosEmbed_ {
/** @private {number} */
this.paddingTop_ = 0;

onDocumentReady(this.win.document, () => {
// Microtask is necessary here to let Safari to recalculate scrollWidth
// post DocumentReady signal.
timer.delay(() => {
this.setup_();
}, 0);
});
// Microtask is necessary here to let Safari to recalculate scrollWidth
// post DocumentReady signal.
whenDocumentReady(this.win.document).then(() => this.setup_());
this.win.addEventListener('resize', () => this.resizeObservable_.fire());

dev.fine(TAG_, 'initialized natural viewport for iOS embeds');
Expand Down Expand Up @@ -1013,22 +1008,20 @@ export class ViewportBindingNaturalIosEmbed_ {

/** @override */
updatePaddingTop(paddingTop) {
onDocumentReady(this.win.document, () => {
onDocumentReady(this.win.document, doc => {
this.paddingTop_ = paddingTop;
// Also tried `paddingTop` but it didn't work for `position:absolute`
// on iOS.
this.win.document.body.style.borderTop =
`${paddingTop}px solid transparent`;
doc.body.style.borderTop = `${paddingTop}px solid transparent`;
});
}

/** @override */
updateLightboxMode(lightboxMode) {
// This code will no longer be needed with the newer iOS viewport
// implementation.
onDocumentReady(this.win.document, () => {
this.win.document.body.style.borderStyle =
lightboxMode ? 'none' : 'solid';
onDocumentReady(this.win.document, doc => {
doc.body.style.borderStyle = lightboxMode ? 'none' : 'solid';
});
}

Expand Down
5 changes: 5 additions & 0 deletions test/functional/test-document-ready.js
Expand Up @@ -62,6 +62,7 @@ describe('documentReady', () => {
const callback = sandbox.spy();
onDocumentReady(testDoc, callback);
expect(callback.callCount).to.equal(1);
expect(callback.getCall(0).args).to.deep.equal([testDoc]);
});

it('should wait to call callback until ready', () => {
Expand All @@ -75,6 +76,7 @@ describe('documentReady', () => {
testDoc.readyState = 'complete';
eventListeners['readystatechange']();
expect(callback.callCount).to.equal(1);
expect(callback.getCall(0).args).to.deep.equal([testDoc]);
expect(eventListeners['readystatechange']).to.equal(undefined);
});

Expand All @@ -94,6 +96,7 @@ describe('documentReady', () => {
testDoc.readyState = 'complete';
eventListeners['readystatechange']();
expect(callback.callCount).to.equal(1);
expect(callback.getCall(0).args).to.deep.equal([testDoc]);
expect(eventListeners['readystatechange']).to.equal(undefined);
});

Expand All @@ -115,6 +118,7 @@ describe('documentReady', () => {

return timer.promise().then(() => {
expect(spy.callCount).to.equal(1);
expect(spy.getCall(0).args).to.deep.equal([testDoc]);
expect(spy2.callCount).to.equal(1);
expect(spy3.callCount).to.equal(1);
});
Expand Down Expand Up @@ -144,6 +148,7 @@ describe('documentReady', () => {

return timer.promise().then(() => {
expect(callback.callCount).to.equal(1);
expect(callback.getCall(0).args).to.deep.equal([testDoc]);
expect(eventListeners['readystatechange']).to.equal(undefined);
});
});
Expand Down