From a374a978297dfcd5f9e994587b32cb7a17e31c0b Mon Sep 17 00:00:00 2001 From: William Chou Date: Mon, 22 Mar 2021 15:29:40 -0400 Subject: [PATCH 1/4] Revert "Revert "amp-list: Fix Bind.rescan vs. diffing race condition (#32650)" (#33232)" This reverts commit 60adca7966ae76bff23dae8f71c7dbe626a059ca. --- extensions/amp-list/0.1/amp-list.js | 58 ++++++++++++++----- extensions/amp-list/0.1/test/test-amp-list.js | 44 ++++++++++++++ 2 files changed, 89 insertions(+), 13 deletions(-) diff --git a/extensions/amp-list/0.1/amp-list.js b/extensions/amp-list/0.1/amp-list.js index ac197b59c060..3316d4675b25 100644 --- a/extensions/amp-list/0.1/amp-list.js +++ b/extensions/amp-list/0.1/amp-list.js @@ -1066,21 +1066,19 @@ export class AmpList extends AMP.BaseElement { // binding=refresh: Only do render-blocking update after initial render. if (binding && binding.startsWith('refresh')) { - // Bind service must be available after first mutation, so don't - // wait on the async service getter. - if (this.bind_ && this.bind_.signals().get('FIRST_MUTATE')) { + // Don't bother using bindForDocOrNull() since the Bind service must be available after first mutate. + const afterFirstMutate = + this.bind_ && this.bind_.signals().get('FIRST_MUTATE'); + if (afterFirstMutate) { return updateWith(this.bind_); } else { - // On initial render, do a non-blocking scan and don't update. - Services.bindForDocOrNull(this.element).then((bind) => { - if (bind) { - const evaluate = binding == 'refresh-evaluate'; - bind.rescan(elements, [], { - 'fast': true, - 'update': evaluate ? 'evaluate' : false, - }); - } - }); + // This must be initial render, so do a non-blocking scan for bindings only. + // [diffable] is a special case that is handled later in render_(), see comment there. + if (!this.element.hasAttribute('diffable')) { + this.scanForBindings_(elements, []); + } + + // Don't block render and return synchronously. return Promise.resolve(elements); } } @@ -1095,6 +1093,32 @@ export class AmpList extends AMP.BaseElement { }); } + /** + * Scans for bindings in `addedElements` and removes bindings in `removedElements`. + * Unlike updateBindings(), does NOT apply bindings or update DOM. + * Should only be used for binding="refresh" or binding="refresh-evaluate". + * @param {!Array} addedElements + * @param {!Array} removedElements + * @private + */ + scanForBindings_(addedElements, removedElements) { + const binding = this.element.getAttribute('binding'); + if (!binding || !binding.startsWith('refresh')) { + return; + } + Services.bindForDocOrNull(this.element).then((bind) => { + if (bind) { + // For binding="refresh-evaluate", we scan for bindings, evaluate+cache expressions, but skip DOM update. + // For binding="refresh", we only scan for bindings. + const update = binding == 'refresh-evaluate' ? 'evaluate' : false; + bind.rescan(addedElements, removedElements, { + 'fast': true, + 'update': update, + }); + } + }); + } + /** * @param {!Array} elements * @param {boolean=} opt_append @@ -1111,6 +1135,14 @@ export class AmpList extends AMP.BaseElement { // TODO:(wg-performance)(#28781) Ensure owners_.scheduleUnlayout() is // called for diff elements that are removed this.diff_(container, elements); + + // For diffable amp-list, we have to wait until DOM diffing is done here to scan for new bindings + // (vs. asynchronously in updateBindings()), and scan the entire container (vs. just `elements`). + // + // This is because instead of replacing the entire DOM subtree, the diffing process removes + // select children from `elements` and inserts them into the container. This results in a race + // between diff_() and Bind.rescan(), which we avoid by delaying the latter until now. + this.scanForBindings_([container], [container]); } else { if (!opt_append) { this.owners_./*OK*/ scheduleUnlayout(this.element, container); 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 c8a7718f94b0..149d9a370fc1 100644 --- a/extensions/amp-list/0.1/test/test-amp-list.js +++ b/extensions/amp-list/0.1/test/test-amp-list.js @@ -1416,6 +1416,50 @@ describes.repeated( } ); }); + + describe('rescan vs. diff race', () => { + async function rescanVsDiffTest() { + env.sandbox.spy(list, 'diff_'); + env.sandbox.spy(list, 'render_'); + + // Diffing is skipped if there's no existing children to diff against. + const oldChild = doc.createElement('p'); + oldChild.textContent = 'foo'; + list.container_.appendChild(oldChild); + + const newChild = doc.createElement('p'); + newChild.textContent = 'bar'; + // New children must have at least one binding to trigger rescan. + newChild.setAttribute('i-amphtml-binding', ''); + const rendered = expectFetchAndRender(DEFAULT_FETCHED_DATA, [ + newChild, + ]); + await list.layoutCallback().then(() => rendered); + } + + it('without diffing, should rescan _before_ render', async () => { + await rescanVsDiffTest(); + + expect(list.diff_).to.not.have.been.called; + expect(bind.rescan).to.have.been.calledOnce; + + // Without diffable, rescan should happen before rendering the new children. + expect(bind.rescan).calledBefore(list.render_); + }); + + it('with diffing, should rescan _after_ render/diff', async () => { + element.setAttribute('diffable', ''); + + await rescanVsDiffTest(); + + expect(list.diff_).to.have.been.calledOnce; + expect(bind.rescan).to.have.been.calledOnce; + + // With diffable, rescanning must happen after rendering (diffing) the new children. + expect(bind.rescan).calledAfter(list.render_); + expect(bind.rescan).calledAfter(list.diff_); + }); + }); }); describe('binding="no"', () => { From 1690ceec2746d2b45dc7784ac6876dc18ddbc004 Mon Sep 17 00:00:00 2001 From: William Chou Date: Fri, 12 Mar 2021 18:45:28 -0500 Subject: [PATCH 2/4] Support diffable with single-item. --- extensions/amp-list/0.1/amp-list.js | 32 +++++++++++++++-------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/extensions/amp-list/0.1/amp-list.js b/extensions/amp-list/0.1/amp-list.js index 3316d4675b25..56ad9803db04 100644 --- a/extensions/amp-list/0.1/amp-list.js +++ b/extensions/amp-list/0.1/amp-list.js @@ -195,9 +195,7 @@ export class AmpList extends AMP.BaseElement { const doc = this.element.ownerDocument; const isEmail = doc && isAmp4Email(doc); const hasPlaceholder = - this.getPlaceholder() || - (this.element.hasAttribute('diffable') && - this.queryDiffablePlaceholder_()); + this.getPlaceholder() || this.element.hasAttribute('diffable'); if (isEmail) { if (!hasPlaceholder) { user().warn( @@ -264,11 +262,7 @@ export class AmpList extends AMP.BaseElement { if (this.diffablePlaceholder_) { this.container_ = this.diffablePlaceholder_; } else { - user().warn( - TAG, - 'Could not find child div[role=list] for diffing.', - this.element - ); + user().warn(TAG, 'Could not find child div for diffing.', this.element); } } if (!this.container_) { @@ -326,19 +320,27 @@ export class AmpList extends AMP.BaseElement { } /** - * A "diffable placeholder" is just a div[role=list] child without the - * [placeholder] attribute. - * - * It's used to display pre-fetch (stale) list content that can be - * DOM diffed with the fetched (fresh) content later. + * A "diffable placeholder" is the child container
(which is usually created by the amp-list + * to hold the rendered children). It serves the same purpose as a placeholder, except it can be diffed. + * + * For example: + * + *
I'm displayed before render.
+ * + * + * + *
I'm displayed before render.
+ * * * @return {?Element} * @private */ queryDiffablePlaceholder_() { + const roleSelector = this.element.hasAttribute('single-item') ? '' : '[role=list]' return scopedQuerySelector( this.element, - '> div[role=list]:not([placeholder]):not([fallback]):not([fetch-error])' + // Don't select other special
children used for placeholders/fallback/etc. + `> div${roleSelector}:not([placeholder]):not([fallback]):not([fetch-error])` ); } @@ -1131,7 +1133,7 @@ export class AmpList extends AMP.BaseElement { const renderAndResize = () => { this.hideFallbackAndPlaceholder_(); - if (this.element.hasAttribute('diffable') && container.hasChildNodes()) { + if (this.element.hasAttribute('diffable')) { // TODO:(wg-performance)(#28781) Ensure owners_.scheduleUnlayout() is // called for diff elements that are removed this.diff_(container, elements); From a608edec099f31d119662d6219f6eaf1ece20538 Mon Sep 17 00:00:00 2001 From: William Chou Date: Wed, 19 May 2021 14:42:31 -0400 Subject: [PATCH 3/4] Fix test. --- extensions/amp-list/0.1/test/test-amp-list.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 149d9a370fc1..d6eab44909ae 100644 --- a/extensions/amp-list/0.1/test/test-amp-list.js +++ b/extensions/amp-list/0.1/test/test-amp-list.js @@ -770,7 +770,7 @@ describes.repeated( it('should attemptChangeHeight initial content', async () => { const initialContent = doc.createElement('div'); initialContent.setAttribute('role', 'list'); - initialContent.style.height = '1337px'; + initialContent.setAttribute('style', 'height: 123px'); // Initial content must be set before buildCallback(), so use // a new test AmpList instance. @@ -785,11 +785,12 @@ describes.repeated( // content, once to resize to rendered contents. listMock .expects('attemptChangeHeight') - .withExactArgs(1337) + .withExactArgs(123) .returns(Promise.resolve()) .twice(); const itemElement = doc.createElement('div'); + itemElement.setAttribute('style', 'height: 123px'); expectFetchAndRender(DEFAULT_FETCHED_DATA, [itemElement]); await list.layoutCallback(); }); From 64637e0a3283a86fbce8860293b51c2d133aea82 Mon Sep 17 00:00:00 2001 From: William Chou Date: Thu, 20 May 2021 20:22:18 -0400 Subject: [PATCH 4/4] Fix lint. --- extensions/amp-list/0.1/amp-list.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/extensions/amp-list/0.1/amp-list.js b/extensions/amp-list/0.1/amp-list.js index 56ad9803db04..d50f431abc0d 100644 --- a/extensions/amp-list/0.1/amp-list.js +++ b/extensions/amp-list/0.1/amp-list.js @@ -322,12 +322,12 @@ export class AmpList extends AMP.BaseElement { /** * A "diffable placeholder" is the child container
(which is usually created by the amp-list * to hold the rendered children). It serves the same purpose as a placeholder, except it can be diffed. - * + * * For example: * *
I'm displayed before render.
* - * + * * *
I'm displayed before render.
* @@ -336,12 +336,12 @@ export class AmpList extends AMP.BaseElement { * @private */ queryDiffablePlaceholder_() { - const roleSelector = this.element.hasAttribute('single-item') ? '' : '[role=list]' - return scopedQuerySelector( - this.element, - // Don't select other special
children used for placeholders/fallback/etc. - `> div${roleSelector}:not([placeholder]):not([fallback]):not([fetch-error])` - ); + let selector = this.element.hasAttribute('single-item') + ? '> div' + : '> div[role=list]'; + // Don't select other special
children used for placeholders/fallback/etc. + selector += ':not([placeholder]):not([fallback]):not([fetch-error])'; + return scopedQuerySelector(this.element, selector); } /**