🚀 [Page Attachments] Ensure that the page attachment container is not inadvertently rendered by the AMP Resources manager

[coreymasanto]

• Fixes [Story performance] Not load all page attachment images when loading story #35534

---

The existing setOwner() call ensures that the page attachment manages its own resources instead of them being handled by the AMP Resources manager. However, it's likely that this call is running after AMP has already loaded the contents of the attachment. This PR hides the attachment container by default, which ensures that its content is not inadvertently rendered/loaded too early. The end result is that page attachment images remain unloaded until the scheduleLayout() call when the attachment fully opens.

[amp-owners-bot]

Hey @gmajoulet, @newmuis, @mszylkowski! These files were changed:

extensions/amp-story-page-attachment/0.1/amp-story-draggable-drawer.js
extensions/amp-story-page-attachment/0.1/amp-story-page-attachment.js

[gmajoulet]

amphtml/extensions/amp-story-page-attachment/0.1/amp-story-draggable-drawer.js

Lines 167 to 179 in 12ba200

	const walker = this.win.document.createTreeWalker(
	this.element,
	NodeFilter.SHOW_ELEMENT,
	null /** filter */,
	false /** entityReferenceExpansion */
	);
	while (walker.nextNode()) {
	const el = dev().assertElement(walker.currentNode);
	if (isAmpElement(el)) {
	this.ampComponents_.push(el);
	Services.ownersForDoc(this.element).setOwner(el, this.element);
	}
	}

This code claims ownership over the lifecycle of all AMP elements within it. My guess is that the code runs AFTER AMP has already built images, which is why they load.

A few fixes are possible:

1. Use CSS to hide the attachment content till layoutCallback ran. If the content is hidden, AMP will not build it
2. Same but use JS to do so (hidden by default with CSS, made visible AFTER we claim ownership)

[coreymasanto]

A few fixes are possible:

1. Use CSS to hide the attachment content till layoutCallback ran. If the content is hidden, AMP will not build it
2. Same but use JS to do so (hidden by default with CSS, made visible AFTER we claim ownership)

I've taken the 2nd approach and I've updated the code, description, and title to reflect this

[gmajoulet]

cc @processprocess to help test all draggable-drawer uses

[coreymasanto]

cc processprocess to help test all draggable-drawer uses

I've tested attachments that contain a mix of images and text, on both desktop and mobile. I'll also check any other cases that we deem necessary

[processprocess]

cc processprocess to help test all draggable-drawer uses

I've tested attachments that contain a mix of images and text, on both desktop and mobile. I'll also check any other cases that we deem necessary

Thanks for manually testing these @coreymasanto :)

@gmajoulet Our drawer visual diff tests open the attachment, which should cover this but agree it's good to manually test as well:
https://github.com/ampproject/amphtml/blob/main/examples/visual-tests/amp-story/amp-story-shopping.js#L26
https://github.com/ampproject/amphtml/blob/main/examples/visual-tests/amp-story/amp-story-page-attachment.js#L238

[mszylkowski]

Looks good, make sure the network tab shows the images loading later as expected and 🚢