SPA with amp shadow DOM extras #4055
Conversation
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
You can now rebase against |
|
Thank you for the feedback, glad to hear one of the items were already fixed. I'll only include the others. |
Update amp-wp-app-shell.js **Rationale** Due to caching restrictions with WPengine the use of a query-string parameter to denote an inner shell request is problematic as it prevents the request from being cached independently. The request needs to be distinctly different from the outer shell in-order for it to be treated as a seperate cache item. **Changes** - Query string parameter is now appended to the path instead of - Fixed bug where code was always expecting a .site-branding element to exist
**Rationale** A prior change introduced an early return for `isHeaderVisible` should the `site-branding` element not be found in the document. However this early return caused code to trigger elsewhere related to scrolling. **Changes** - Ensure `.site-content-contain` document lookup is also guarded to prevent calling scrollIntoView on a non-existing dom element.
mehigh
force-pushed
the
add/spa-amp-shadow-dom-extras
branch
from
c5b6ee9 to
bb371b1
Compare
|
This was rebased, only the remaining 4 commits are present now. |
|
I consent to this being merged, and Jonathan Barnett gave it in a different PR where I originally added this in, so we should be good with the consent needs. |
|
@indieisaconcept sorry, I need you to do another |
includes/class-amp-theme-support.php
Outdated
| @@ -2033,7 +2033,7 @@ public static function is_output_buffering() { | |||
| */ | |||
| public static function finish_output_buffering( $response ) { | |||
| self::$is_output_buffering = false; | |||
| return self::prepare_response( $response ); | |||
| return apply_filters( 'amp_document_output', self::prepare_response( $response ) ); | |||
There was a problem hiding this comment.
This is a very powerful filter that has the possibility of overriding AMP validity. Any such mutations to the document should rather be done through sanitizers, which the AMP plugin will then guarantee to be valid AMP since the tag-and-attribute sanitizer runs last. So I think bb371b1 should be reverted.
There was a problem hiding this comment.
@westonruter we found this feature to be necessary in-order to integrate non AMP compatible code into our documents. This was a conscious decision on our part & we were well aware of the implications of such a change in terms of AMP validation. As a publisher this level of flexibility is really important as unfortunately AMP compatibility is not a priority for some third-party services.
As an aside I think AMP validation in general needs to be overhauled to provide a broader degree of opt-in/opt-out flexibility but I see that as a larger discussion outside of the scope of this specific PR & better suited to ampproject/amphtml.
There was a problem hiding this comment.
@indieisaconcept What specific changes did you need to make? There is now a way to do this officially via AMP dev mode, depending on your use case.
There was a problem hiding this comment.
I've reverted this for now in f59a9c9.
| @@ -124,7 +127,8 @@ | |||
| */ | |||
| function fetchShadowDocResponse( url ) { | |||
| const ampUrl = new URL( url ); | |||
| ampUrl.searchParams.set( ampAppShell.componentQueryVar, 'inner' ); | |||
| const pathSuffix = '_' + ampAppShell.componentQueryVar + '_inner'; | |||
| ampUrl.pathname = ampUrl.pathname + pathSuffix; | |||
There was a problem hiding this comment.
Does this not require the addition of new rewrite rules?
|
@googlebot I consent. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
googlebot
added
cla: yes
Summary
Issued it against the branch part of this PR: #1519
This contains some improvements to amp-wp-app-shell.js contributed by Jonathan Barnett ( @indieisaconcept ). Consent was already provided.
Ensure .site-content-contain element exists
Rationale
A prior change introduced an early return for isHeaderVisible should the
site-branding element not be found in the document. However this early
return caused code to trigger elsewhere related to scrolling.
Changes
Ensure .site-content-contain document lookup is also guarded to prevent
calling scrollIntoView on a non-existing dom element.
Rationale
Due to caching restrictions with WPengine the use of a query-string parameter to
denote an inner shell request is problematic as it prevents the request from
being cached independently. The request needs to be distinctly different from
the outer shell in-order for it to be treated as a seperate cache item.
Changes
Query string parameter is now appended to the path instead of
Fixed bug where code was always expecting a .site-branding element to exist
Checklist