Skip to content

Commit

Permalink
Prevent styles from being removed when in Customizer preview with Sta…
Browse files Browse the repository at this point in the history 
…ndard mode (#4553)

* Prevent styles from being removed when in Customizer preview with Standard mode enabled

* Revert "Prevent styles from being removed when in Customizer preview with Standard mode enabled"

This reverts commit 9d0e2ad.

* Re-work AmpBoilerplate to only remove boilerplate-specific styles

* Remove now-unused AmpProject\DevMode

* Improve phpdoc explaining refined behavior

* Remove trailing comma from function args

* Simplify logic in removeStyleAndNoscriptTags

* Remove all boilerplates not just the identified one

* Rename method to isBoilerplateStyle and improve docs

Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>

Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>
  • Loading branch information
@pierlon @westonruter @schlessera
3 people authored Apr 9, 2020
1 parent f6710a0 commit dba6055
Show file tree
Hide file tree
Showing 3 changed files with 89 additions and 70 deletions.
2 changes: 2 additions & 0 deletions lib/common/src/Attribute.php
View file
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
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
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

0 comments on commit dba6055

Please sign in to comment.