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.

Sign up for GitHub

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

amp-list: [src] mutation must be in mutate context #16513

Merged
merged 5 commits into from
Jul 3, 2018
Merged

amp-list: [src] mutation must be in mutate context #16513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b61bbd7
Make sure amp-list renders via amp-bind happen in mutate context.
Jun 30, 2018
cbd2b7a
Add unit test.
Jul 2, 2018
6f916a9
Remove unnecessary promise chain in tests.
Jul 2, 2018
748d340
Always mutate instead.
Jul 2, 2018
de2d6f1
Fix lint.
Jul 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 32 additions & 32 deletions extensions/amp-list/0.1/amp-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,35 +125,34 @@ export class AmpList extends AMP.BaseElement {
/** @override */
layoutCallback() {
this.layoutCompleted_ = true;

return this.fetchList_();
}

/** @override */
mutatedAttributesCallback(mutations) {
const src = mutations['src'];
const state = mutations['state'];

if (src !== undefined) {
const typeOfSrc = typeof src;
if (typeOfSrc === 'string') {
// Defer to fetch in layoutCallback() before first layout.
if (this.layoutCompleted_) {
this.fetchList_();
return this.fetchList_();
}
} else if (typeOfSrc === 'object') {
const items = isArray(src) ? src : [src];
this.scheduleRender_(items);
// Remove the 'src' now that local data is used to render the list.
this.element.setAttribute('src', '');
const items = isArray(src) ? src : [src];
return this.scheduleRender_(items);
} else {
this.user().error(TAG, 'Unexpected "src" type: ' + src);
}
} else if (state !== undefined) {
const items = isArray(state) ? state : [state];
this.scheduleRender_(items);
user().error(TAG, '[state] is deprecated, please use [src] instead.');
const items = isArray(state) ? state : [state];
return this.scheduleRender_(items);
}
return Promise.resolve();
}

/**
Expand Down Expand Up @@ -277,15 +276,16 @@ export class AmpList extends AMP.BaseElement {
this.renderItems_ = null;
}
};
this.templates_.findAndRenderTemplateArray(this.element, current.items)
const {items, resolver, rejecter} = current;
this.templates_.findAndRenderTemplateArray(this.element, items)
.then(elements => this.updateBindingsForElements_(elements))
.then(elements => this.rendered_(elements))
.then(elements => this.render_(elements))
.then(/* onFulfilled */ () => {
scheduleNextPass();
current.resolver();
resolver();
}, /* onRejected */ () => {
scheduleNextPass();
current.rejecter();
rejecter();
});
}

Expand Down Expand Up @@ -333,27 +333,27 @@ export class AmpList extends AMP.BaseElement {
* @param {!Array<!Element>} elements
* @private
*/
rendered_(elements) {
removeChildren(dev().assertElement(this.container_));
elements.forEach(element => {
if (!element.hasAttribute('role')) {
element.setAttribute('role', 'listitem');
}
this.container_.appendChild(element);
});

const event = createCustomEvent(this.win,
AmpEvents.DOM_UPDATE, /* detail */ null, {bubbles: true});
this.container_.dispatchEvent(event);

// Change height if needed.
this.getVsync().measure(() => {
const scrollHeight = this.container_./*OK*/scrollHeight;
const height = this.element./*OK*/offsetHeight;
if (scrollHeight > height) {
this.attemptChangeHeight(scrollHeight).catch(() => {});
}
});
render_(elements) {
this.mutateElement(() => {
removeChildren(dev().assertElement(this.container_));
elements.forEach(element => {
if (!element.hasAttribute('role')) {
element.setAttribute('role', 'listitem');
}
this.container_.appendChild(element);
});
const event = createCustomEvent(this.win,
AmpEvents.DOM_UPDATE, /* detail */ null, {bubbles: true});
this.container_.dispatchEvent(event);
// Change height if needed.
this.getVsync().measure(() => {
const scrollHeight = this.container_./*OK*/scrollHeight;
const height = this.element./*OK*/offsetHeight;
if (scrollHeight > height) {
this.attemptChangeHeight(scrollHeight).catch(() => {});
}
});
}, this.container_);
}

/**
Expand Down
65 changes: 34 additions & 31 deletions extensions/amp-list/0.1/test/test-amp-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,13 @@ describes.realWin('amp-list component', {
*/
function expectFetchAndRender(fetched, rendered, opts = DEFAULT_LIST_OPTS) {
const fetch = Promise.resolve(fetched);
listMock.expects('fetch_')
.withExactArgs(opts.expr).returns(fetch).atLeast(1);

if (opts.resetOnRefresh) {
listMock.expects('togglePlaceholder').withExactArgs(true).once();
listMock.expects('toggleLoading').withExactArgs(true, true).once();
}
listMock.expects('fetch_')
.withExactArgs(opts.expr).returns(fetch).atLeast(1);
listMock.expects('toggleLoading').withExactArgs(false).once();
listMock.expects('togglePlaceholder').withExactArgs(false).once();

Expand All @@ -97,7 +98,9 @@ describes.realWin('amp-list component', {
templatesMock.expects('findAndRenderTemplateArray')
.withExactArgs(element, itemsToRender).returns(render).atLeast(1);

return Promise.all([fetch, render]);
listMock.expects('mutateElement')
.callsFake(mutator => mutator())
.atLeast(1);
}

describe('without amp-bind', () => {
Expand All @@ -110,8 +113,8 @@ describes.realWin('amp-list component', {
{title: 'Title1'},
];
const itemElement = doc.createElement('div');
const rendered = expectFetchAndRender(items, [itemElement]);
return list.layoutCallback().then(() => rendered).then(() => {
expectFetchAndRender(items, [itemElement]);
return list.layoutCallback().then(() => {
expect(list.container_.contains(itemElement)).to.be.true;
});
});
Expand All @@ -123,7 +126,7 @@ describes.realWin('amp-list component', {
const itemElement = doc.createElement('div');
itemElement.style.height = '1337px';

const rendered = expectFetchAndRender(items, [itemElement]);
expectFetchAndRender(items, [itemElement]);

let measureFunc;
listMock.expects('getVsync').returns({
Expand All @@ -136,7 +139,7 @@ describes.realWin('amp-list component', {
.withExactArgs(1337)
.returns(Promise.resolve());

return list.layoutCallback().then(() => rendered).then(() => {
return list.layoutCallback().then(() => {
expect(list.container_.contains(itemElement)).to.be.true;
expect(measureFunc).to.exist;
measureFunc();
Expand All @@ -148,10 +151,10 @@ describes.realWin('amp-list component', {
const itemElement = doc.createElement('div');
element.setAttribute('single-item', 'true');

const rendered = expectFetchAndRender(
expectFetchAndRender(
items, [itemElement], {expr: 'items', singleItem: true});

return list.layoutCallback().then(() => rendered).then(() => {
return list.layoutCallback().then(() => {
expect(list.container_.contains(itemElement)).to.be.true;
});
});
Expand All @@ -165,10 +168,10 @@ describes.realWin('amp-list component', {
const itemElement = doc.createElement('div');
element.setAttribute('max-items', '2');

const rendered = expectFetchAndRender(
expectFetchAndRender(
items, [itemElement], {expr: 'items', maxItems: 2});

return list.layoutCallback().then(() => rendered).then(() => {
return list.layoutCallback().then(() => {
expect(list.container_.contains(itemElement)).to.be.true;
});
});
Expand All @@ -178,9 +181,9 @@ describes.realWin('amp-list component', {

const items = [{title: 'Title1'}];
const itemElement = doc.createElement('div');
const rendered = expectFetchAndRender(items, [itemElement]);
expectFetchAndRender(items, [itemElement]);

return list.layoutCallback().then(() => rendered).then(() => {
return list.layoutCallback().then(() => {
expect(spy).to.have.been.calledOnce;
expect(spy).calledWithMatch({
type: AmpEvents.DOM_UPDATE,
Expand All @@ -196,7 +199,7 @@ describes.realWin('amp-list component', {

const items = [{title: 'foo'}];
const foo = doc.createElement('div');
const rendered = expectFetchAndRender(items, [foo]);
expectFetchAndRender(items, [foo]);
const layout = list.layoutCallback();

// Execute another fetch-triggering action immediately (actually on
Expand All @@ -210,7 +213,7 @@ describes.realWin('amp-list component', {
listMock.expects('togglePlaceholder').withExactArgs(false).once();


return layout.then(() => rendered).then(() => {
return layout.then(() => {
expect(list.container_.contains(foo)).to.be.true;

// Only one render pass should be invoked at a time.
Expand All @@ -224,9 +227,9 @@ describes.realWin('amp-list component', {
it('should refetch if refresh action is called', () => {
const items = [{title: 'foo'}];
const foo = doc.createElement('div');
const rendered = expectFetchAndRender(items, [foo]);
expectFetchAndRender(items, [foo]);

return list.layoutCallback().then(() => rendered).then(() => {
return list.layoutCallback().then(() => {
expect(list.container_.contains(foo)).to.be.true;

const renderedAgain = expectFetchAndRender(items, [foo]);
Expand All @@ -245,9 +248,9 @@ describes.realWin('amp-list component', {
const items = [{title: 'foo'}];
const foo = doc.createElement('div');
const opts = {expr: 'items', resetOnRefresh: true};
const rendered = expectFetchAndRender(items, [foo], opts);
expectFetchAndRender(items, [foo], opts);

return list.layoutCallback().then(() => rendered).then(() => {
return list.layoutCallback().then(() => {
expect(list.container_.contains(foo)).to.be.true;

const renderedAgain = expectFetchAndRender(items, [foo], opts);
Expand Down Expand Up @@ -300,9 +303,9 @@ describes.realWin('amp-list component', {
it('should set accessibility roles', () => {
const items = [{title: 'Title1'}];
const itemElement = doc.createElement('div');
const rendered = expectFetchAndRender(items, [itemElement]);
expectFetchAndRender(items, [itemElement]);

return list.layoutCallback().then(() => rendered).then(() => {
return list.layoutCallback().then(() => {
expect(list.container_.getAttribute('role')).to.equal('list');
expect(itemElement.getAttribute('role')).to.equal('listitem');
});
Expand All @@ -313,9 +316,9 @@ describes.realWin('amp-list component', {
element.setAttribute('role', 'list1');
const itemElement = doc.createElement('div');
itemElement.setAttribute('role', 'listitem1');
const rendered = expectFetchAndRender(items, [itemElement]);
expectFetchAndRender(items, [itemElement]);

return list.layoutCallback().then(() => rendered).then(() => {
return list.layoutCallback().then(() => {
expect(list.element.getAttribute('role')).to.equal('list1');
expect(itemElement.getAttribute('role')).to.equal('listitem1');
});
Expand Down Expand Up @@ -390,9 +393,9 @@ describes.realWin('amp-list component', {
it('should render and remove `src` if [src] points to local data', () => {
const items = [{title: 'foo'}];
const foo = doc.createElement('div');
const rendered = expectFetchAndRender(items, [foo]);
expectFetchAndRender(items, [foo]);

return list.layoutCallback().then(() => rendered).then(() => {
return list.layoutCallback().then(() => {
expect(list.container_.contains(foo)).to.be.true;

listMock.expects('fetchList_').never();
Expand All @@ -406,9 +409,9 @@ describes.realWin('amp-list component', {
it('should refetch if [src] attribute changes (after layout)', () => {
const items = [{title: 'foo'}];
const foo = doc.createElement('div');
const rendered = expectFetchAndRender(items, [foo]);
expectFetchAndRender(items, [foo]);

return list.layoutCallback().then(() => rendered).then(() => {
return list.layoutCallback().then(() => {
expect(list.container_.contains(foo)).to.be.true;

// Allowed post-layout.
Expand All @@ -427,8 +430,8 @@ describes.realWin('amp-list component', {
it('should not call scanAndApply() before FIRST_MUTATE', function*() {
const items = [{title: 'Title1'}];
const output = [doc.createElement('div')];
const rendered = expectFetchAndRender(items, output);
yield list.layoutCallback().then(() => rendered);
expectFetchAndRender(items, output);
yield list.layoutCallback();

expect(bind.scanAndApply).to.not.have.been.called;
});
Expand All @@ -440,8 +443,8 @@ describes.realWin('amp-list component', {

const items = [{title: 'Title1'}];
const output = [doc.createElement('div')];
const rendered = expectFetchAndRender(items, output);
yield list.layoutCallback().then(() => rendered);
expectFetchAndRender(items, output);
yield list.layoutCallback();

expect(bind.scanAndApply).to.have.been.calledOnce;
expect(bind.scanAndApply).calledWithExactly(output, [list.container_]);
Expand Down
10 changes: 4 additions & 6 deletions src/custom-element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1294,12 +1294,10 @@ function createBaseCustomElementClass(win) {

/**
* Called when one or more attributes are mutated.
* Note Must be called inside a mutate context.
* Note Boolean attributes have a value of `true` and `false` when
* present and missing, respectively.
* @param {
* !JsonObject<string, (null|boolean|string|number|Array|Object)>
* } mutations
* Note: Must be called inside a mutate context.
* Note: Boolean attributes have a value of `true` and `false` when
* present and missing, respectively.
* @param {!JsonObject<string, (null|boolean|string|number|Array|Object)>} mutations
*/
mutatedAttributesCallback(mutations) {
this.implementation_.mutatedAttributesCallback(mutations);
Expand Down