Skip to content

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

Merged
coreymasanto merged 13 commits intoampproject:mainfrom
coreymasanto:attachmentLoad
Apr 1, 2022
Merged

🚀 [Page Attachments] Ensure that the page attachment container is not inadvertently rendered by the AMP Resources manager#37961
coreymasanto merged 13 commits intoampproject:mainfrom
coreymasanto:attachmentLoad

Conversation

@coreymasanto
Copy link
Copy Markdown
Contributor

@coreymasanto coreymasanto commented Mar 25, 2022 •
edited
Loading

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.

@coreymasanto
Make page attachment container element hidden by default, and then un…
3e16477 
…hide the element the first time the user starts dragging the attachment open
@coreymasanto coreymasanto requested a review from gmajoulet March 25, 2022 20:20
@coreymasanto coreymasanto marked this pull request as ready for review March 25, 2022 20:20
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Mar 25, 2022

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

mszylkowski
mszylkowski reviewed Mar 25, 2022
View reviewed changes
gmajoulet
gmajoulet reviewed Mar 25, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 coreymasanto changed the title 🚀 [Page Attachments] Only load page attachment images upon attachment open instead of upon story load 🚀 [Page Attachments] Ensure that the page attachment container is not inadvertently rendered by the AMP Resources manager Mar 29, 2022
@coreymasanto
Copy link
Copy Markdown
Contributor Author

coreymasanto commented Mar 29, 2022

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
Copy link
Copy Markdown
Contributor

gmajoulet commented Mar 29, 2022

cc @processprocess to help test all draggable-drawer uses

@coreymasanto coreymasanto requested a review from gmajoulet March 30, 2022 07:34
@coreymasanto
Copy link
Copy Markdown
Contributor Author

coreymasanto commented Mar 30, 2022

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
Copy link
Copy Markdown
Contributor

processprocess commented Mar 30, 2022 •
edited
Loading

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
mszylkowski approved these changes Mar 30, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@mszylkowski mszylkowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@coreymasanto coreymasanto enabled auto-merge (squash) March 31, 2022 17:56
@coreymasanto coreymasanto merged commit aa8e969 into ampproject:main Apr 1, 2022
@coreymasanto coreymasanto deleted the attachmentLoad branch April 1, 2022 16:42
@jshamble jshamble mentioned this pull request Apr 8, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmajoulet gmajoulet gmajoulet approved these changes

@processprocess processprocess processprocess approved these changes

@mszylkowski mszylkowski mszylkowski approved these changes

Assignees

No one assigned

Labels

PR use: In Beta / Experimental PR use: In LTS PR use: In Stable

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Story performance] Not load all page attachment images when loading story

5 participants

@coreymasanto @gmajoulet @processprocess @mszylkowski @ampprojectbot