-
Notifications
You must be signed in to change notification settings - Fork 382
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.
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
Add AMP_Script_Sanitizer to replace noscript elements with their contents #1226
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
Hi @westonruter and @amedina,
This PR looks really good, and has very thorough PHPUnit tests. As you mentioned, this should help the user experience when scripts are removed.
When you have a chance, it'd be good to address the Travis build issues. I could help with that if you'd like.
while ( $noscript->firstChild ) { | ||
$fragment->appendChild( $noscript->firstChild ); | ||
} | ||
$fragment->appendChild( $this->dom->createComment( '/noscript' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nice how this outputs the comment <!--noscript-->
. This should help with debugging if needed, like on WordPress.org support topics.
/** | ||
* Test style[amp-boilerplate] preservation. | ||
*/ | ||
public function test_boilerplate_preservation() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice thorough testing of the boilerplate not being removed.
@kienstra thank you. I'm very confused about why the tests are failing. They all pass for me locally. |
@westonruter, that's strange, the tests pass locally for me also. |
cf4751d
to
22a1457
Compare
Force-pushed a tweak. Maybe there was a caching problem. The test is failing as if these lines are not present: --- a/tests/test-amp-script-sanitizer.php
+++ b/tests/test-amp-script-sanitizer.php
@@ -38,8 +38,6 @@ class AMP_Script_Sanitizer_Test extends WP_UnitTestCase {
*/
public function test_noscript_promotion( $source, $expected = null ) {
$dom = AMP_DOM_Utils::get_dom_from_content( $source );
- $sanitizer = new AMP_Script_Sanitizer( $dom );
- $sanitizer->sanitize();
$whitelist_sanitizer = new AMP_Tag_And_Attribute_Sanitizer( $dom );
$whitelist_sanitizer->sanitize();
$content = AMP_DOM_Utils::get_content_from_dom( $dom ); |
22a1457
to
b0a08e1
Compare
I'm pretty sure the problem is due to an old version of libxml and this bit of logic: |
66e04d1
to
c44b954
Compare
c44b954
to
fb55f08
Compare
As noted in Implementing Interactivity:
Nevertheless, this is not totally true at present: the plugin's postprocessor does not currently do any transformation of
noscript
elements meaning that the no-JS fallbacks will be not be successfully used in AMP pages.So this PR ensures that that a non-AMP document like:
Will get served in AMP as:
Thus it encourages the use of no-JS fallbacks for the baseline AMP experience as well as for visitors who have JS turned off in their browsers.
This will be further built upon in #1213 and #1032 with more intelligence about whether to promote the
noscript
element's contents or rather to convert a companionscript
to an AMP equivalent.The base of this PR comes from #1127