Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Prevent styles from being removed when in Customizer preview with Standard mode #4553

Merged
merged 10 commits into from
Apr 9, 2020
Merged

Prevent styles from being removed when in Customizer preview with Standard mode #4553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9d0e2ad
Prevent styles from being removed when in Customizer preview with Sta…
pierlon Apr 8, 2020
f4c635e
Merge branch 'develop' of github.com:ampproject/amp-wp into fix/stand…
westonruter Apr 9, 2020
b7d273a
Revert "Prevent styles from being removed when in Customizer preview …
westonruter Apr 9, 2020
00dc3ce
Re-work AmpBoilerplate to only remove boilerplate-specific styles
westonruter Apr 9, 2020
b3fb078
Remove now-unused AmpProject\DevMode
westonruter Apr 9, 2020
d8fc048
Improve phpdoc explaining refined behavior
westonruter Apr 9, 2020
ba90f19
Remove trailing comma from function args
westonruter Apr 9, 2020
4f81f56
Simplify logic in removeStyleAndNoscriptTags
westonruter Apr 9, 2020
6762054
Remove all boilerplates not just the identified one
westonruter Apr 9, 2020
89105c8
Rename method to isBoilerplateStyle and improve docs
westonruter Apr 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/common/src/Attribute.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ interface Attribute
const ALL_AMP4ADS = [self::AMP4ADS, self::AMP4ADS_EMOJI, self::AMP4ADS_EMOJI_ALT];
const ALL_AMP4EMAIL = [self::AMP4EMAIL, self::AMP4EMAIL_EMOJI, self::AMP4EMAIL_EMOJI_ALT];

const ALL_BOILERPLATES = [self::AMP_BOILERPLATE, self::AMP4ADS_BOILERPLATE, self::AMP4EMAIL_BOILERPLATE];

const TYPE_HTML = 'text/html';
const TYPE_JSON = 'application/json';
const TYPE_LD_JSON = 'application/ld+json';
Expand Down
52 changes: 32 additions & 20 deletions lib/optimizer/src/Transformer/AmpBoilerplate.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,15 @@

use AmpProject\Amp;
use AmpProject\Attribute;
use AmpProject\DevMode;
use AmpProject\Dom\Document;
use AmpProject\Optimizer\ErrorCollection;
use AmpProject\Optimizer\Transformer;
use AmpProject\Tag;
use DOMElement;

/**
* Transformer that removes <style> and <noscript> tags in <head>, keeping only the amp-custom style tag. It then
* inserts the amp-boilerplate.
* Transformer that removes AMP boilerplate <style> and <noscript> tags in <head>, keeping only the amp-custom style tag.
* It then (re-)inserts the amp-boilerplate unless the document is marked with the i-amphtml-no-boilerplate attribute.
*
* This is ported from the Go optimizer.
*
Expand Down Expand Up @@ -68,31 +67,44 @@ public function transform(Document $document, ErrorCollection $errors)
}

/**
* Remove all <style> and <noscript> tags except for the <style amp-custom> tag.
* Remove all <style> and <noscript> tags which are for the boilerplate.
*
* @param Document $document Document to remove the tags from.
*/
private function removeStyleAndNoscriptTags(Document $document)
{
$headNode = $document->head->firstChild;
$devMode = DevMode::isActiveForDocument($document);
while ($headNode) {
$nextSibling = $headNode->nextSibling;
if ($headNode instanceof DOMElement && (! $devMode || ! DevMode::hasExemptionForNode($headNode))) {
switch ($headNode->tagName) {
case Tag::STYLE:
if (! $headNode->hasAttribute(Attribute::AMP_CUSTOM)) {
$document->head->removeChild($headNode);
}
break;
case Tag::NOSCRIPT:
$document->head->removeChild($headNode);
break;
}
/**
* Style element.
*
* @var DOMElement $style
*/
foreach (iterator_to_array($document->head->getElementsByTagName('style')) as $style) {
if (! $this->isBoilerplateStyle($style)) {
continue;
}
if (Tag::NOSCRIPT === $style->parentNode->nodeName) {
$style->parentNode->parentNode->removeChild($style->parentNode);
} else {
$style->parentNode->removeChild($style);
}
}
}

$headNode = $nextSibling;
/**
* Check whether an element is a boilerplate style.
*
* @param DOMElement $element Element to check.
* @return bool Whether the element is a boilerplate style.
*/
private function isBoilerplateStyle(DOMElement $element)
{
foreach (Attribute::ALL_BOILERPLATES as $boilerplate) {
if ($element->hasAttribute($boilerplate)) {
return true;
}
}

return false;
}

/**
Expand Down
105 changes: 55 additions & 50 deletions lib/optimizer/tests/Transformer/AmpBoilerplateTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,27 +26,16 @@ final class AmpBoilerplateTest extends TestCase
*/
public function dataTransform()
{
$inputWithBoilerplate = static function ($html, $boilerplate) {
$htmlDocument = static function ($html, $headEnd = '') {
return TestMarkup::DOCTYPE . $html . '<head>' .
TestMarkup::META_CHARSET . TestMarkup::META_VIEWPORT . TestMarkup::SCRIPT_AMPRUNTIME .
TestMarkup::LINK_FAVICON . TestMarkup::LINK_CANONICAL .
$boilerplate .
$headEnd .
'</head><body></body></html>';
};

$inputWithoutBoilerplate = static function ($html) {
return TestMarkup::DOCTYPE . $html . '<head>' .
TestMarkup::META_CHARSET . TestMarkup::META_VIEWPORT . TestMarkup::SCRIPT_AMPRUNTIME .
TestMarkup::LINK_FAVICON . TestMarkup::LINK_CANONICAL .
'</head><body></body></html>';
};

$expected = static function ($html, $boilerplate) {
return TestMarkup::DOCTYPE . $html . '<head>' .
TestMarkup::META_CHARSET . TestMarkup::META_VIEWPORT . TestMarkup::SCRIPT_AMPRUNTIME .
TestMarkup::LINK_FAVICON . TestMarkup::LINK_CANONICAL .
$boilerplate .
'</head><body></body></html>';
$repeatTwice = static function ($value) {
return array_fill(0, 2, $value);
};

$ampBoilerplate = '<style ' . Attribute::AMP_BOILERPLATE . '>' . Amp::BOILERPLATE_CSS . '</style>' .
Expand All @@ -57,65 +46,81 @@ public function dataTransform()
$amp4EmailBoilerplate = '<style ' . Attribute::AMP4EMAIL_BOILERPLATE . '>' . Amp::AMP4ADS_AND_AMP4EMAIL_BOILERPLATE_CSS . '</style>';

return [
'keeps boilerplate' => [
$inputWithBoilerplate('<html ⚡>', $ampBoilerplate),
$expected('<html ⚡>', $ampBoilerplate),
],
'keeps boilerplate' => $repeatTwice(
$htmlDocument('<html ⚡>', $ampBoilerplate)
),

'keeps boilerplate for amp4ads' => [
$inputWithBoilerplate('<html amp4ads>', $amp4AdsBoilerplate),
$expected('<html amp4ads>', $amp4AdsBoilerplate),
],
'keeps boilerplate again' => $repeatTwice(
$htmlDocument('<html amp>', $ampBoilerplate)
),

'keeps boilerplate for ⚡4ads' => [
$inputWithBoilerplate('<html ⚡4ads>', $amp4AdsBoilerplate),
$expected('<html ⚡4ads>', $amp4AdsBoilerplate),
'removes incorrect boilerplates' => [
$htmlDocument('<html amp>', $ampBoilerplate . $amp4AdsBoilerplate . $amp4EmailBoilerplate),
$htmlDocument('<html amp>', $ampBoilerplate),
],

'keeps boilerplate for amp4email' => [
$inputWithBoilerplate('<html amp4email>', $amp4EmailBoilerplate),
$expected('<html amp4email>', $amp4EmailBoilerplate),
],
'leaves out boilerplate' => $repeatTwice(
$htmlDocument('<html amp i-amphtml-no-boilerplate>')
),

'keeps boilerplate for ⚡4email' => [
$inputWithBoilerplate('<html ⚡4email>', $amp4EmailBoilerplate),
$expected('<html ⚡4email>', $amp4EmailBoilerplate),
'removes boilerplate' => [
$htmlDocument('<html amp i-amphtml-no-boilerplate>', $ampBoilerplate),
$htmlDocument('<html amp i-amphtml-no-boilerplate>'),
],

'keeps boilerplate for amp4ads' => $repeatTwice(
$htmlDocument('<html amp4ads>', $amp4AdsBoilerplate)
),

'keeps boilerplate for ⚡4ads' => $repeatTwice(
$htmlDocument('<html ⚡4ads>', $amp4AdsBoilerplate)
),

'keeps boilerplate for amp4email' => $repeatTwice(
$htmlDocument('<html amp4email>', $amp4EmailBoilerplate)
),

'keeps boilerplate for ⚡4email' => $repeatTwice(
$htmlDocument('<html ⚡4email>', $amp4EmailBoilerplate)
),

'adds boilerplate if missing' => [
$inputWithoutBoilerplate('<html ⚡>'),
$expected('<html ⚡>', $ampBoilerplate),
$htmlDocument('<html ⚡>'),
$htmlDocument('<html ⚡>', $ampBoilerplate),
],

'adds boilerplate if missing for amp4ads' => [
$inputWithoutBoilerplate('<html amp4ads>'),
$expected('<html amp4ads>', $amp4AdsBoilerplate),
$htmlDocument('<html amp4ads>'),
$htmlDocument('<html amp4ads>', $amp4AdsBoilerplate),
],

'adds boilerplate if missing for ⚡4ads' => [
$inputWithoutBoilerplate('<html ⚡4ads>'),
$expected('<html ⚡4ads>', $amp4AdsBoilerplate),
$htmlDocument('<html ⚡4ads>'),
$htmlDocument('<html ⚡4ads>', $amp4AdsBoilerplate),
],

'adds boilerplate if missing for amp4email' => [
$inputWithoutBoilerplate('<html amp4email>'),
$expected('<html amp4email>', $amp4EmailBoilerplate),
$htmlDocument('<html amp4email>'),
$htmlDocument('<html amp4email>', $amp4EmailBoilerplate),
],

'adds boilerplate if missing for ⚡4email' => [
$inputWithoutBoilerplate('<html ⚡4email>'),
$expected('<html ⚡4email>', $amp4EmailBoilerplate),
$htmlDocument('<html ⚡4email>'),
$htmlDocument('<html ⚡4email>', $amp4EmailBoilerplate),
],

'keeps devmode nodes when in devmode' => [
$inputWithBoilerplate('<html ⚡ data-ampdevmode>', '<style data-ampdevmode>h1: red;</style>' . $ampBoilerplate),
$expected('<html ⚡ data-ampdevmode>', '<style data-ampdevmode>h1: red;</style>' . $ampBoilerplate),
],
'leaves styles that lack boilerplate attribute' => $repeatTwice(
$htmlDocument('<html ⚡>', '<style>h1{color:red}</style><noscript><style>h2{color:blue}</style></noscript>' . $ampBoilerplate)
),

'removes devmode nodes when not in devmode' => [
$inputWithBoilerplate('<html ⚡>', '<style data-ampdevmode>h1: red;</style>' . $ampBoilerplate),
$expected('<html ⚡>', $ampBoilerplate),
'leaves styles that lack boilerplate attribute and adds boilerplate' => [
$htmlDocument('<html ⚡>', '<style>h1{color:red}</style><noscript><style>h2{color:blue}</style></noscript>'),
$htmlDocument('<html ⚡>', '<style>h1{color:red}</style><noscript><style>h2{color:blue}</style></noscript>' . $ampBoilerplate),
],

'leaves styles that lack boilerplate attribute and leaves out boilerplate' => $repeatTwice(
$htmlDocument('<html amp i-amphtml-no-boilerplate>', '<style>h1{color:red}</style><noscript><style>h2{color:blue}</style></noscript>')
),
];
}

Expand Down