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.

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 PreloadHeroImage Optimizer transformer #5350

Merged
merged 98 commits into from
Nov 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
98 commits
Select commit Hold shift + click to select a range
9706741
Add initial scaffold for preload hero image transformer
schlessera Aug 27, 2020
53dc56a
Merge branch 'develop' into add/5055-hero-images
schlessera Aug 28, 2020
437fa8d
Add HeroImage value object
schlessera Aug 29, 2020
b268cd6
Add missing attributes
schlessera Aug 29, 2020
72a01b5
Add Amp::isTemplate check
schlessera Aug 29, 2020
99eec9b
Complete first iteration of the code
schlessera Aug 29, 2020
4d462af
Add tests for Amp::isTemplate()
schlessera Aug 29, 2020
3a98ad0
Use Extension class constants
schlessera Aug 29, 2020
d8b2015
Hook up spec test suite for PreloadHeroImage
schlessera Aug 29, 2020
0b78bd7
Add image URL validation
schlessera Aug 29, 2020
4c4bc56
Fix PHPStan issues
schlessera Aug 29, 2020
9703ebd
Fix code style issues
schlessera Aug 29, 2020
4f1426b
Check for existing preloads
schlessera Aug 29, 2020
96f88ab
Merge branch 'develop' into add/5055-hero-images
schlessera Sep 1, 2020
fcbcc4a
Extract isValidImageSrc into separate Url helper
schlessera Sep 1, 2020
16dbf2a
Add test for Amp::iAmpStory()
schlessera Sep 1, 2020
8795fde
Extract isAmpIframe() into Amp helper class
schlessera Sep 1, 2020
d46c44e
Add preloadHeroImage configuration key
schlessera Sep 1, 2020
d97d49c
Add TooManyHeroImages error
schlessera Sep 1, 2020
e850726
Add CannotPreloadImage error
schlessera Sep 1, 2020
8cfadb1
Video posters should not have SSRed images
schlessera Sep 8, 2020
51da2cd
Fix srcset assignment
schlessera Sep 8, 2020
111e497
Fix negated comparisons
schlessera Sep 8, 2020
a0dc9cc
Add documentation for PreloadHeroImage transformer
schlessera Sep 8, 2020
1acb66f
Add test for throwing error at max data-hero
schlessera Sep 8, 2020
7c32636
Add tests for throwing srcset error
schlessera Sep 8, 2020
7af1c5b
Remove <noscript> fallback image when adding SSR'ed image
schlessera Sep 9, 2020
cbb01a1
Only continue to next parent when both dimensions are missing
schlessera Sep 9, 2020
ae4b1bd
Limit amount of parent traversal to be done when looking for dimensions
schlessera Sep 9, 2020
86f34ef
Skip "auto" dimensions
schlessera Sep 9, 2020
a4f2ec4
Add relative URL tests
schlessera Sep 9, 2020
361efde
Merge branch 'develop' into add/5055-hero-images
schlessera Sep 13, 2020
71d5170
Extract ImageDimensions into separate class
schlessera Sep 13, 2020
e98e684
Fix typehint for threshold
schlessera Sep 13, 2020
56804ea
Add DIV to Tag constants
schlessera Sep 13, 2020
d890f2e
Merge branch 'develop' into add/5055-hero-images
schlessera Sep 14, 2020
034e724
Let ImageDimensions store floats and strings
schlessera Sep 14, 2020
0a07090
Use auto width for fixed height in tests
schlessera Sep 15, 2020
b05b035
Merge branch 'develop' into add/5055-hero-images
schlessera Sep 23, 2020
0ab477e
AMP Stories can accept SSR images as well now
schlessera Sep 23, 2020
3850c25
Change viewport caching todo into a comment
schlessera Sep 23, 2020
b71afe8
Use attribute constant for viewport
schlessera Sep 23, 2020
122a5a0
Use constants for checking viewport
schlessera Sep 23, 2020
392709c
Use attribute constant for viewport name
schlessera Sep 23, 2020
025d54b
Fix userpass and host order
schlessera Sep 23, 2020
e68f7ff
Fix typo in docblock
schlessera Sep 23, 2020
9b58169
Make CannotPreloadImage work without element
schlessera Sep 23, 2020
b0ed9ba
Add more tests for PreloadHeroImage transformer
schlessera Sep 23, 2020
b6033c2
Merge branch 'develop' into add/5055-hero-images
schlessera Oct 10, 2020
c251909
Remove unused check for amp-story
schlessera Oct 10, 2020
d35b8a8
Merge branch 'develop' into add/5055-hero-images
schlessera Oct 12, 2020
d1ba9c7
Update spec tests
schlessera Oct 12, 2020
fba400f
Check amp-anim for placeholders as well
schlessera Oct 12, 2020
7afa02b
Fix bug in placeholder detection
schlessera Oct 12, 2020
a6c1a89
Fix typos in tests
schlessera Oct 12, 2020
1169981
Fix order of preload link additions
schlessera Oct 12, 2020
8eb3250
Fix srcset preload test
schlessera Oct 12, 2020
ea637d5
Add 2nd image to test noscript in AMP_Theme_Support tests
schlessera Oct 12, 2020
6ae3e3e
Allow for more embeds to preload their placeholders
schlessera Oct 12, 2020
a981eeb
Add tests for Youtube placeholder
schlessera Oct 12, 2020
7309ee3
Use any descendant image of a placeholder for preloading
schlessera Oct 12, 2020
dda3065
Add test for descendant amp-img and img tags
schlessera Oct 12, 2020
6caab91
Make noscript fetching account for descendants
schlessera Oct 14, 2020
17380a7
Introduce constant for noscript>img query
schlessera Oct 14, 2020
607fa5d
Add DetermineHeroImages transformer in plugin
schlessera Oct 14, 2020
6343103
Add support for CSS background-image property
schlessera Oct 14, 2020
d4465a4
Support data-amp-original-style for fetching background-image URL
schlessera Oct 14, 2020
ec6daca
Improve transformer docblock
schlessera Oct 14, 2020
a56f7c1
Add missing trailing comma
schlessera Oct 14, 2020
29daf74
Revert priority change again
schlessera Oct 14, 2020
1555c72
Add site icon detection
schlessera Oct 15, 2020
07689ad
Use constants for response destinations
schlessera Oct 15, 2020
27d3be6
Add experimental srcset support
schlessera Oct 16, 2020
30973c6
Add featured image detection
schlessera Oct 16, 2020
e47cb8f
Add featured image detection
schlessera Oct 16, 2020
2bd673f
Switch off srcset preloading
schlessera Oct 16, 2020
a92b8c1
Copy ErrorComparison over into the plugin tests
schlessera Oct 16, 2020
9addb67
Add tests for DetermineHeroImages
schlessera Oct 16, 2020
60abe7a
Make isTiny depend on height for layout::FIXED_HEIGHT
schlessera Oct 16, 2020
c249d69
Merge branch 'develop' into add/5055-hero-images
schlessera Oct 16, 2020
a146319
Extract xpath queries intro constants
schlessera Oct 17, 2020
7ba627e
Take pre-existing data-hero elements into account
schlessera Oct 17, 2020
01b33e0
Add tests for pre-existing hero images
schlessera Oct 17, 2020
cdb8818
Only look for hero image candidates that are not already tagged as he…
schlessera Oct 17, 2020
a856eb1
Allow for the embed element itself to have a background image
schlessera Oct 31, 2020
1eed7f1
Allow regular image in anticipation of AMP change
schlessera Oct 31, 2020
332ad51
Grammatify correctlier
schlessera Oct 31, 2020
d41fab7
Append pass instead of replacing entire user string
schlessera Oct 31, 2020
ce95088
Rename ResponseDestination into RequestDestination
schlessera Oct 31, 2020
a7ac118
Rename isValidImageUrl into isValidNonDataUrl
schlessera Oct 31, 2020
3320b6f
Rename expected_errors into expectedErrors
schlessera Oct 31, 2020
6ee3b0a
Remove useless TODO
schlessera Oct 31, 2020
910342c
Correct equal sign alignment
schlessera Oct 31, 2020
b2f67a6
Rename site icon int ocustom logo
schlessera Nov 3, 2020
94f4259
Add configuration key for srcset preloading
schlessera Nov 3, 2020
5270af3
Adapt isValidImageSrc tests to isValidNonDataUrl
schlessera Nov 3, 2020
ec5584f
Ensure src is valid non-data URL
schlessera Nov 3, 2020
981e960
Fix missing renames for ResponseDestination -> RequestDestination
schlessera Nov 3, 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
14 changes: 11 additions & 3 deletions includes/class-amp-theme-support.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
use AmpProject\RemoteRequest\FallbackRemoteGetRequest;
use AmpProject\RemoteRequest\FilesystemRemoteGetRequest;
use AmpProject\AmpWP\RemoteRequest\WpHttpRemoteGetRequest;
use AmpProject\RequestDestination;
use AmpProject\Tag;

/**
Expand Down Expand Up @@ -1667,7 +1668,7 @@ public static function ensure_required_markup( Document $dom, $script_handles =
Tag::LINK,
[
Attribute::REL => Attribute::REL_PRELOAD,
'as' => Tag::SCRIPT,
Attribute::AS_ => RequestDestination::SCRIPT,
Attribute::HREF => $runtime_src,
]
);
Expand All @@ -1686,7 +1687,7 @@ public static function ensure_required_markup( Document $dom, $script_handles =
Tag::LINK,
[
Attribute::REL => Attribute::REL_PRELOAD,
'as' => Tag::SCRIPT,
Attribute::AS_ => RequestDestination::SCRIPT,
Attribute::HREF => $amp_scripts[ $script_handle ]->getAttribute( Attribute::SRC ),
]
);
Expand Down Expand Up @@ -2267,6 +2268,8 @@ private static function get_optimizer_configuration( $args ) {
Optimizer\Transformer\TransformedIdentifier::class,
]
);
} else {
array_unshift( $transformers, Transformer\DetermineHeroImages::class );
schlessera marked this conversation as resolved.
Show resolved Hide resolved
}

array_unshift( $transformers, Transformer\AmpSchemaOrgMetadata::class );
Expand All @@ -2281,7 +2284,12 @@ private static function get_optimizer_configuration( $args ) {
$configuration = apply_filters(
'amp_optimizer_config',
array_merge(
[ Optimizer\Configuration::KEY_TRANSFORMERS => $transformers ],
[
Optimizer\Configuration::KEY_TRANSFORMERS => $transformers,
Optimizer\Transformer\PreloadHeroImage::class => [
Optimizer\Configuration\PreloadHeroImageConfiguration::INLINE_STYLE_BACKUP_ATTRIBUTE => 'data-amp-original-style',
],
],
$args
)
);
Expand Down
67 changes: 67 additions & 0 deletions lib/common/src/Amp.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace AmpProject;

use AmpProject\Dom\Document;
use DOMElement;
use DOMNode;

Expand Down Expand Up @@ -249,6 +250,56 @@ public static function isCustomElement(DOMNode $node)
return $node instanceof DOMElement && strpos($node->tagName, Extension::PREFIX) === 0;
}

/**
* Check whether the given document is an AMP story.
*
* @param Document $document Document of the page to check within.
* @return bool Whether the provided document is an AMP story.
*/
public static function isAmpStory(Document $document)
{
foreach ($document->head->childNodes as $node) {
if (
$node instanceof DOMElement
&&
$node->tagName === Tag::SCRIPT
&&
$node->getAttribute(Attribute::CUSTOM_ELEMENT) === Extension::STORY
) {
return true;
}
}

return false;
}

/**
* Check whether a given node is an AMP template.
*
* @param DOMNode $node Node to check.
* @return bool Whether the node is an AMP template.
*/
public static function isTemplate(DOMNode $node)
{
if (! $node instanceof DOMElement) {
return false;
}

if ($node->tagName === Tag::TEMPLATE) {
return true;
}

if (
$node->tagName === Tag::SCRIPT
&& $node->hasAttribute(Attribute::TEMPLATE)
&& $node->getAttribute(Attribute::TEMPLATE) === Extension::MUSTACHE
) {
return true;
}

return false;
}

/**
* Check whether a given node is an async <script> element.
*
Expand All @@ -273,4 +324,20 @@ private static function isAsyncScript(DOMNode $node)

return true;
}

/**
* Check whether a given node is an AMP iframe.
*
* @param DOMNode $node Node to check.
* @return bool Whether the node is an AMP iframe.
*/
public static function isAmpIframe(DOMNode $node)
{
if (! $node instanceof DOMElement) {
return false;
}

return $node->tagName === Extension::IFRAME
|| $node->tagName === Extension::VIDEO_IFRAME;
}
}
11 changes: 11 additions & 0 deletions lib/common/src/Attribute.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ interface Attribute
const AMP_RUNTIME = 'amp-runtime';
const AMP_SCRIPT_SRC = 'amp-script-src';
const ARIA_HIDDEN = 'aria-hidden';
const AS_ = 'as'; // Underscore needed because 'as' is a PHP keyword.
const ASYNC = 'async';
const ATTRIBUTION = 'attribution';
const AUTOPLAY = 'autoplay';
const CHARSET = 'charset';
const CLASS_ = 'class'; // Underscore needed because 'class' is a PHP keyword.
Expand All @@ -43,8 +45,11 @@ interface Attribute
const HOST_SERVICE = 'host-service';
const HREF = 'href';
const HTTP_EQUIV = 'http-equiv';
const I_AMPHTML_SSR = 'i-amphtml-ssr';
const I_AMPHTML_VERSION = 'i-amphtml-version';
const ID = 'id';
const IMAGESRCSET = 'imagesrcset';
const IMAGESIZES = 'imagesizes';
const IMPORTANCE = 'importance';
const INTRINSICSIZE = 'intrinsicsize';
const LAYOUT = 'layout';
Expand All @@ -55,8 +60,12 @@ interface Attribute
const NAME = 'name';
const NOLOADING = 'noloading';
const OBJECT_FIT = 'object-fit';
const OBJECT_POSITION = 'object-position';
const ON = 'on';
const PLACEHOLDER = 'placeholder';
const POSTER = 'poster';
const PROFILE = 'profile';
const REFERRERPOLICY = 'referrerpolicy';
const REL = 'rel';
const ROLE = 'role';
const SRCSET = 'srcset';
Expand Down Expand Up @@ -93,4 +102,6 @@ interface Attribute
const REL_PRELOAD = 'preload';
const REL_PRERENDER = 'prerender';
const REL_STYLESHEET = 'stylesheet';

const DATA_HERO = 'data-hero';
}
24 changes: 19 additions & 5 deletions lib/common/src/Dom/Document.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,12 @@
*
* Abstract away some of the difficulties of working with PHP's DOMDocument.
*
* @property DOMXPath $xpath XPath query object for this document.
* @property DOMElement $html The document's <html> element.
* @property DOMElement $head The document's <head> element.
* @property DOMElement $body The document's <body> element.
* @property DOMNodeList $ampElements The document's <amp-*> elements.
* @property DOMXPath $xpath XPath query object for this document.
* @property DOMElement $html The document's <html> element.
* @property DOMElement $head The document's <head> element.
* @property DOMElement $body The document's <body> element.
* @property DOMElement|null $viewport The document's viewport meta element.
* @property DOMNodeList $ampElements The document's <amp-*> elements.
*
* @package ampproject/common
*/
Expand Down Expand Up @@ -1614,6 +1615,19 @@ public function __get($name)
$this->body = $this->getElementsByTagName(Tag::BODY)->item(0);
}
return $this->body;
case Attribute::VIEWPORT:
// This is not cached as it could potentially be requested too early, before the viewport was added, and
// the cache would then store null without rechecking later on after the viewport has been added.
for ($node = $this->head->firstChild; $node !== null; $node = $node->nextSibling) {
if (
$node instanceof DOMElement
&& $node->tagName === Tag::META
&& $node->getAttribute(Attribute::NAME) === Attribute::VIEWPORT
) {
return $node;
}
}
return null;
case 'ampElements':
$this->ampElements = $this->xpath->query(".//*[ starts-with( name(), 'amp-' ) ]", $this->body)
?: new DOMNodeList();
Expand Down
17 changes: 17 additions & 0 deletions lib/common/src/Extension.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,35 @@
interface Extension
{

const AD = 'amp-ad';
const ANALYTICS = 'amp-analytics';
const ANIM = 'amp-anim';
const AUDIO = 'amp-audio';
const BIND = 'amp-bind';
const BRIGHTCOVE = 'amp-brightcove';
const DAILYMOTION = 'amp-dailymotion';
const DELIGHT_PLAYER = 'amp-delight-player';
const DYNAMIC_CSS_CLASSES = 'amp-dynamic-css-classes';
const EXPERIMENT = 'amp-experiment';
const FACEBOOK = 'amp-facebook';
const GEO = 'amp-geo';
const GFYCAT = 'amp-gfycat';
const IFRAME = 'amp-iframe';
const IMAGE = 'amp-img';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should stick with the extension name being the same as the constant name (other areas where this is being used would also need to be updated).

Suggested change
const IMAGE = 'amp-img';
const IMG = 'amp-img';

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will do this in a separate PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

const IMGUR = 'amp-imgur';
const INSTAGRAM = 'amp-instagram';
const MUSTACHE = 'amp-mustache';
const PINTEREST = 'amp-pinterest';
const PIXEL = 'amp-pixel';
const REDDIT = 'amp-reddit';
const SOCIAL_SHARE = 'amp-social-share';
const STORY = 'amp-story';
const TWITTER = 'amp-twitter';
const VIDEO = 'amp-video';
const VIDEO_IFRAME = 'amp-video-iframe';
const VIMEO = 'amp-vimeo';
const YOUTUBE = 'amp-youtube';
const WISTIA_PLAYER = 'amp-wistia-player';

/**
* Prefix of an AMP extension.
Expand Down
102 changes: 102 additions & 0 deletions lib/common/src/RequestDestination.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
<?php

namespace AmpProject;

/**
* Interface with constants for the different request destinations that are supported.
*
* For the purposes of the AMP implementation, we are only interested in the
* request destinations that are valid values for the 'as' attribute in preloads.
*
* Full list of request destinations:
* @see https://fetch.spec.whatwg.org/#concept-request-destination
*
* @package ampproject/common
*/
interface RequestDestination
{

/**
* Audio file, as typically used in <audio>.
*
* @var string
*/
const AUDIO = 'audio';

/**
* An HTML document intended to be embedded by a <frame> or <iframe>.
*
* @var string
*/
const DOCUMENT = 'document';

/**
* A resource to be embedded inside an <embed> element.
*
* @var string
*/
const EMBED = 'embed';

/**
* Resource to be accessed by a fetch or XHR request, such as an ArrayBuffer or JSON file.
*
* @var string
*/
const FETCH = 'fetch';

/**
* Font file.
*
* @var string
*/
const FONT = 'font';

/**
* Image file.
*
* @var string
*/
const IMAGE = 'image';

/**
* A resource to be embedded inside an <object> element.
*
* @var string
*/
const OBJECT = 'object';

/**
* JavaScript file.
*
* @var string
*/
const SCRIPT = 'script';

/**
* CSS stylesheet.
*
* @var string
*/
const STYLE = 'style';

/**
* WebVTT file.
*
* @var string
*/
const TRACK = 'track';

/**
* A JavaScript web worker or shared worker.
*
* @var string
*/
const WORKER = 'worker';

/**
* Video file, as typically used in <video>.
*
* @var string
*/
const VIDEO = 'video';
}
1 change: 1 addition & 0 deletions lib/common/src/Tag.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ interface Tag
const BODY = 'body';
const BR = 'br';
const COL = 'col';
const DIV = 'div';
const EMBED = 'embed';
const FIGCAPTION = 'figcaption';
const FIGURE = 'figure';
Expand Down
Loading