From cdbb4968378f68d2e45e4afe09bc380e42b5fb61 Mon Sep 17 00:00:00 2001 From: Jake Fried Date: Wed, 15 Apr 2020 20:58:14 -0400 Subject: [PATCH] toggleLoading: remove force paramater (#27508) * toggleLoading: remove force parameter * Oops. --- extensions/amp-list/0.1/amp-list.js | 2 +- extensions/amp-list/0.1/test/test-amp-list.js | 7 ++-- extensions/amp-twitter/0.1/amp-twitter.js | 2 +- src/base-element.js | 8 ++--- src/custom-element.js | 36 ++++++++----------- 5 files changed, 22 insertions(+), 33 deletions(-) diff --git a/extensions/amp-list/0.1/amp-list.js b/extensions/amp-list/0.1/amp-list.js index 5da563c3ef53..24d17e9e9c05 100644 --- a/extensions/amp-list/0.1/amp-list.js +++ b/extensions/amp-list/0.1/amp-list.js @@ -539,7 +539,7 @@ export class AmpList extends AMP.BaseElement { ) { // Placeholder and loading don't need a mutate context. this.togglePlaceholder(true); - this.toggleLoading(true, /* opt_force */ true); + this.toggleLoading(true); this.mutateElement(() => { this.toggleFallback_(false); // Clean up bindings in children before removing them from DOM. diff --git a/extensions/amp-list/0.1/test/test-amp-list.js b/extensions/amp-list/0.1/test/test-amp-list.js index 5c62a37b6ffa..27e68c3fec37 100644 --- a/extensions/amp-list/0.1/test/test-amp-list.js +++ b/extensions/amp-list/0.1/test/test-amp-list.js @@ -160,7 +160,7 @@ describes.repeated( // If "reset-on-refresh" is set, show loading/placeholder before fetch. if (opts.resetOnRefresh) { listMock.expects('togglePlaceholder').withExactArgs(true).once(); - listMock.expects('toggleLoading').withExactArgs(true, true).once(); + listMock.expects('toggleLoading').withExactArgs(true).once(); } // Stub the rendering of the template. @@ -1048,10 +1048,7 @@ describes.repeated( listMock.expects('fetchList_').never(); // Expect display of placeholder/loading before render. listMock.expects('togglePlaceholder').withExactArgs(true).once(); - listMock - .expects('toggleLoading') - .withExactArgs(true, true) - .once(); + listMock.expects('toggleLoading').withExactArgs(true).once(); // Expect hiding of placeholder/loading after render. listMock.expects('togglePlaceholder').withExactArgs(false).once(); listMock.expects('toggleLoading').withExactArgs(false).once(); diff --git a/extensions/amp-twitter/0.1/amp-twitter.js b/extensions/amp-twitter/0.1/amp-twitter.js index cf3ad5653f0a..9ede7a8da73c 100644 --- a/extensions/amp-twitter/0.1/amp-twitter.js +++ b/extensions/amp-twitter/0.1/amp-twitter.js @@ -201,7 +201,7 @@ class AmpTwitter extends AMP.BaseElement { mutatedAttributesCallback(mutations) { if (this.iframe_ && mutations['data-tweetid'] != null) { this.unlayoutCallback(); - this.toggleLoading(true, /* opt_force */ true); + this.toggleLoading(true); this.layoutCallback(); } } diff --git a/src/base-element.js b/src/base-element.js index 345b8d452f2c..a250f0086726 100644 --- a/src/base-element.js +++ b/src/base-element.js @@ -722,14 +722,12 @@ export class BaseElement { } /** - * Hides or shows the loading indicator. This function must only - * be called inside a mutate context. + * Hides or shows the loading indicator. * @param {boolean} state - * @param {boolean=} opt_force * @public @final */ - toggleLoading(state, opt_force) { - this.element.toggleLoading(state, {force: !!opt_force}); + toggleLoading(state) { + this.element.toggleLoading(state); } /** diff --git a/src/custom-element.js b/src/custom-element.js index 72ea06a7ea1c..29a8e7067afc 100644 --- a/src/custom-element.js +++ b/src/custom-element.js @@ -1608,14 +1608,14 @@ function createBaseCustomElementClass(win) { isLoadingEnabled_() { // No loading indicator will be shown if either one of these conditions // true: - // 1. `noloading` attribute is specified; - // 2. The element has not been whitelisted; - // 3. The element is too small or has not yet been measured; - // 4. The element has already been laid out (include having loading + // 1. The document is A4A. + // 2. `noloading` attribute is specified; + // 3. The element has already been laid out, and does not support reshowing the indicator (include having loading // error); - // 5. The element is a `placeholder` or a `fallback`; - // 6. The element's layout is not a size-defining layout. - // 7. The document is A4A. + // 4. The element is too small or has not yet been measured; + // 5. The element has not been whitelisted; + // 6. The element is an internal node (e.g. `placeholder` or `fallback`); + // 7. The element's layout is not a size-defining layout. if (this.isInA4A()) { return false; } @@ -1623,10 +1623,12 @@ function createBaseCustomElementClass(win) { this.loadingDisabled_ = this.hasAttribute('noloading'); } + const laidOut = + this.layoutCount_ > 0 || this.signals_.get(CommonSignals.RENDER_START); if ( - this.layoutCount_ > 0 || - this.layoutWidth_ <= 0 || // Layout is not ready or invisible this.loadingDisabled_ || + (laidOut && !this.implementation_.isLoadingReused()) || + this.layoutWidth_ <= 0 || // Layout is not ready or invisible !isLoadingAllowed(this) || isInternalOrServiceNode(this) || !isLayoutSizeDefined(this.layout_) @@ -1686,22 +1688,14 @@ function createBaseCustomElementClass(win) { /** * Turns the loading indicator on or off. * @param {boolean} state - * @param {{cleanup:(boolean|undefined), force:(boolean|undefined), startTime:(number|undefined)}=} opt_options + * @param {{cleanup:(boolean|undefined), startTime:(number|undefined)}=} opt_options * @public @final */ toggleLoading(state, opt_options) { const cleanup = opt_options && opt_options.cleanup; - const force = opt_options && opt_options.force; const startTime = opt_options && opt_options.startTime; assertNotTemplate(this); - if ( - state && - !this.implementation_.isLoadingReused() && - (this.layoutCount_ > 0 || this.signals_.get(CommonSignals.RENDER_START)) - ) { - // Loading has already been canceled. Ignore. - return; - } + if (state === this.loadingState_ && !opt_options) { // Loading state is the same. return; @@ -1712,7 +1706,7 @@ function createBaseCustomElementClass(win) { } // Check if loading should be shown. - if (state && !force && !this.isLoadingEnabled_()) { + if (state && !this.isLoadingEnabled_()) { this.loadingState_ = false; return; } @@ -1722,7 +1716,7 @@ function createBaseCustomElementClass(win) { let state = this.loadingState_; // Repeat "loading enabled" check because it could have changed while // waiting for vsync. - if (state && !force && !this.isLoadingEnabled_()) { + if (state && !this.isLoadingEnabled_()) { state = false; } if (state) {