Skip to content

Commit

Permalink
amp-list: Support diffable with single-item (ampproject#33249)
Browse files Browse the repository at this point in the history 
* Revert "Revert "amp-list: Fix Bind.rescan vs. diffing race condition (ampproject#32650)" (ampproject#33232)"

This reverts commit 60adca7.

* Support diffable with single-item.

* Fix test.

* Fix lint.
  • Loading branch information
@rochapablo
William Chou authored and rochapablo committed Aug 30, 2021
1 parent 2e4d47a commit ec6aa63
Show file tree
Hide file tree
Showing 2 changed files with 111 additions and 32 deletions.
94 changes: 64 additions & 30 deletions extensions/amp-list/0.1/amp-list.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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_) {
Expand Down Expand Up @@ -326,20 +320,28 @@ export class AmpList extends AMP.BaseElement {
}

/**
* A "diffable placeholder" is just a div[role=list] child without the
* [placeholder] attribute.
* A "diffable placeholder" is the child container <div> (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.
*
* It's used to display pre-fetch (stale) list content that can be
* DOM diffed with the fetched (fresh) content later.
* For example:
* <amp-list>
* <div placeholder>I'm displayed before render.</div>
* </amp-list>
*
* <amp-list diffable>
* <div role=list>I'm displayed before render.</div>
* </amp-list>
*
* @return {?Element}
* @private
*/
queryDiffablePlaceholder_() {
return scopedQuerySelector(
this.element,
'> div[role=list]:not([placeholder]):not([fallback]):not([fetch-error])'
);
let selector = this.element.hasAttribute('single-item')
? '> div'
: '> div[role=list]';
// Don't select other special <div> children used for placeholders/fallback/etc.
selector += ':not([placeholder]):not([fallback]):not([fetch-error])';
return scopedQuerySelector(this.element, selector);
}

/**
Expand Down Expand Up @@ -1066,21 +1068,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);
}
}
Expand All @@ -1095,6 +1095,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<!Element>} addedElements
* @param {!Array<!Element>} 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<!Element>} elements
* @param {boolean=} opt_append
Expand All @@ -1107,10 +1133,18 @@ 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);

// 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);
Expand Down
49 changes: 47 additions & 2 deletions extensions/amp-list/0.1/test/test-amp-list.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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();
});
Expand Down Expand Up @@ -1416,6 +1417,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"', () => {
Expand Down

0 comments on commit ec6aa63

Please sign in to comment.