Amphtml: amp-apester-media: Element mutation outside mutation phase

[jridgewell]

amp-apester-media inserts its iframe inside #layoutCallback, but because it's inside a Promise's then chain, it's outside of a mutation phase.

layoutCallback() {
  this.element.classList.add('amp-apester-container');
  // This is inside the mutation phase

  return this.queryMedia_().then(response => {
    // We're no longer inside mutation phase, due to promise's
    // async resolution.
    this.element.appendChild(iframe);
  });
}

[ampprojectbot]

This issue doesn't have a category which makes it harder for us to keep track of it. Please add an appropriate category.

[aghassemi]

/cc @mrsufgi @OmriKeret

[OmriKeret]

Hi guys,
I will gladly fix it with our next PR (ETA - end of next week).

@aghassemi Is it soon enough?

[jridgewell]

That's fine.

[OmriKeret]

Hi @aghassemi @jridgewell,
I’ve encountered a dead end, wondered if you could suggest the AMP way of solving it.
In order to construct the unit we need to know what media to display and in order to figure it out we do an api call (“queryMedia_()“).
I could initiate the call earlier but I won’t be sure that it’s finished before the layout callback.
WDYT?

[aghassemi]

@OmriKeret

Would something like this work? (pseudo-code)

layoutCallback() {


var media;
return queryMedia(response => {
 media = ...;
 return vsync.mutatePromise( () => {
    this.element.appendChild(iframe);
    return this.loadPromise(iframe);
  });
}).then( () => {
  return vsync.mutatePromise( () => {
    state.element.classList.add('i-amphtml-apester-iframe-ready');
    this.togglePlaceholder(false);
    ...
    changeHeight
    ... etc..
 });
});


}

[OmriKeret]

Thanks @aghassemi looks perfect.
fix is included in the following PR: #13741

[ampprojectbot]

This issue hasn't been updated in awhile. @aghassemi Do you have any updates?

[aghassemi]

fixed by #13741