diff --git a/includes/sanitizers/class-amp-core-theme-sanitizer.php b/includes/sanitizers/class-amp-core-theme-sanitizer.php index 0cc32de0ec0..89eea469975 100644 --- a/includes/sanitizers/class-amp-core-theme-sanitizer.php +++ b/includes/sanitizers/class-amp-core-theme-sanitizer.php @@ -1638,7 +1638,7 @@ public function wrap_modal_in_lightbox( $args = [] ) { return; } - $body_id = AMP_DOM_Utils::get_element_id( $this->dom->body, 'body' ); + $body_id = $this->dom->getElementId( $this->dom->body, 'body' ); $open_xpaths = isset( $args['open_button_xpath'] ) ? $args['open_button_xpath'] : []; $close_xpaths = isset( $args['close_button_xpath'] ) ? $args['close_button_xpath'] : []; @@ -1776,7 +1776,7 @@ public function add_twentytwenty_modals() { } } - $modal_id = AMP_DOM_Utils::get_element_id( $modal ); + $modal_id = $this->dom->getElementId( $modal ); // Add the lightbox itself as a close button xpath as well. // With twentytwenty compat, the lightbox fills the entire screen, and only an inner wrapper will contain @@ -1808,7 +1808,7 @@ public function add_twentytwenty_modals() { */ public function add_twentytwenty_toggles() { $toggles = $this->dom->xpath->query( '//*[ @data-toggle-target ]' ); - $body_id = AMP_DOM_Utils::get_element_id( $this->dom->body, 'body' ); + $body_id = $this->dom->getElementId( $this->dom->body, 'body' ); if ( false === $toggles || 0 === $toggles->length ) { return; @@ -1821,7 +1821,7 @@ public function add_twentytwenty_toggles() { */ foreach ( $toggles as $toggle ) { $toggle_target = $toggle->getAttribute( 'data-toggle-target' ); - $toggle_id = AMP_DOM_Utils::get_element_id( $toggle ); + $toggle_id = $this->dom->getElementId( $toggle ); if ( 'next' === $toggle_target ) { $target_node = $toggle->nextSibling; @@ -1847,7 +1847,7 @@ public function add_twentytwenty_toggles() { $is_sub_menu = AMP_DOM_Utils::has_class( $target_node, 'sub-menu' ); $new_target_node = $is_sub_menu ? $this->get_closest_submenu( $toggle ) : $target_node; - $new_target_id = AMP_DOM_Utils::get_element_id( $new_target_node ); + $new_target_id = $this->dom->getElementId( $new_target_node ); $state_string = str_replace( '-', '_', $new_target_id ); @@ -1869,7 +1869,7 @@ public function add_twentytwenty_toggles() { // Skip adding the 'active' class on the "Close" button in the primary nav menu. continue; } - $target_toggle_id = AMP_DOM_Utils::get_element_id( $target_toggle ); + $target_toggle_id = $this->dom->getElementId( $target_toggle ); AMP_DOM_Utils::add_amp_action( $toggle, 'tap', "{$target_toggle_id}.toggleClass(class='active')" ); } } @@ -1888,7 +1888,7 @@ public function add_twentytwenty_toggles() { $focus_element = $this->dom->xpath->query( $focus_xpath )->item( 0 ); if ( $focus_element instanceof DOMElement ) { - $focus_element_id = AMP_DOM_Utils::get_element_id( $focus_element ); + $focus_element_id = $this->dom->getElementId( $focus_element ); AMP_DOM_Utils::add_amp_action( $toggle, 'tap', "{$focus_element_id}.focus" ); } } diff --git a/includes/utils/class-amp-dom-utils.php b/includes/utils/class-amp-dom-utils.php index ffa2aff6085..c7eadeaf234 100644 --- a/includes/utils/class-amp-dom-utils.php +++ b/includes/utils/class-amp-dom-utils.php @@ -365,31 +365,22 @@ public static function has_class( DOMElement $element, $class ) { * If the element does not have an ID, create one first. * * @since 1.4.0 + * @since 1.5.1 Deprecated for AmpProject\Dom\Document::getElementId() + * + * @deprecated Use AmpProject\Dom\Document::getElementId() instead. * * @param DOMElement $element Element to get the ID for. - * @param string $prefix Optional. Defaults to '_amp_wp_id_'. + * @param string $prefix Optional. Defaults to 'amp-wp-id'. * @return string ID to use. */ public static function get_element_id( $element, $prefix = 'amp-wp-id' ) { - static $index_counter = []; - - if ( $element->hasAttribute( 'id' ) ) { - return $element->getAttribute( 'id' ); - } - - if ( ! array_key_exists( $prefix, $index_counter ) ) { - $index_counter[ $prefix ] = 2; - $element->setAttribute( 'id', $prefix ); - - return $prefix; - } - - $id = "{$prefix}-{$index_counter[ $prefix ]}"; - $index_counter[ $prefix ] ++; - - $element->setAttribute( 'id', $id ); + _deprecated_function( + 'AMP_DOM_Utils::get_element_id', + '1.5.1', + 'Use AmpProject\Amp\Dom\Document::getElementId() instead' + ); - return $id; + return Document::fromNode( $element )->getElementId( $element, $prefix ); } /** diff --git a/lib/common/composer.json b/lib/common/composer.json index 5509862caa9..d92993142eb 100644 --- a/lib/common/composer.json +++ b/lib/common/composer.json @@ -8,11 +8,11 @@ "ext-dom": "*", "ext-iconv": "*", "ext-json": "*", - "ext-libxml": "*" + "ext-libxml": "*", + "php-parallel-lint/php-parallel-lint": "^1.2" }, "require-dev": { "civicrm/composer-downloads-plugin": "^2.1", - "jakub-onderka/php-parallel-lint": "^1.0", "roave/security-advisories": "dev-master", "squizlabs/php_codesniffer": "^3" }, diff --git a/lib/common/src/Attribute.php b/lib/common/src/Attribute.php index 03c6f57a54d..9555b5d7ed6 100644 --- a/lib/common/src/Attribute.php +++ b/lib/common/src/Attribute.php @@ -58,6 +58,7 @@ interface Attribute const PROFILE = 'profile'; const REL = 'rel'; const ROLE = 'role'; + const SRCSET = 'srcset'; const SIZES = 'sizes'; const STYLE = 'style'; const SRC = 'src'; diff --git a/lib/common/src/Dom/CssByteCountCalculator.php b/lib/common/src/Dom/CssByteCountCalculator.php new file mode 100644 index 00000000000..89accc786f8 --- /dev/null +++ b/lib/common/src/Dom/CssByteCountCalculator.php @@ -0,0 +1,79 @@ + tag, relative to the node. + * + * @var string + */ + const AMP_CUSTOM_STYLE_TAG_XPATH = './/style[@amp-custom]'; + + /** + * XPath query to fetch the inline style attributes, relative to the node. + * + * @var string + */ + const INLINE_STYLE_ATTRIBUTES_XPATH = './/@style'; + + /** + * Document to calculate the total byte count for. + * + * @var Document + */ + private $document; + + /** + * Instantiate a CssByteCountCalculator object. + * + * @param Document $document Document to calculate the total byte count for. + */ + public function __construct(Document $document) + { + $this->document = $document; + } + + /** + * Calculate the byte count for the provided document. + * + * @return int Total byte count of CSS styles in the document. + */ + public function calculate() + { + $ampCustomStyle = $this->document->xpath->query(self::AMP_CUSTOM_STYLE_TAG_XPATH, $this->document->head); + $inlineStyles = $this->document->xpath->query(self::INLINE_STYLE_ATTRIBUTES_XPATH, $this->document->body); + + return $this->calculateForNodeList($ampCustomStyle) + $this->calculateForNodeList($inlineStyles); + } + + /** + * Calculate the byte count of CSS styles for a given node list. + * + * @param DOMNodeList $nodeList Node list to calculate the byte count of CSS styles for. + * @return int Byte count of CSS styles for the given node list. + */ + private function calculateForNodeList(DOMNodeList $nodeList) + { + $byteCount = 0; + + foreach ($nodeList as $node) { + $byteCount += strlen($node->textContent); + } + + return $byteCount; + } +} diff --git a/lib/common/src/Dom/Document.php b/lib/common/src/Dom/Document.php index 3e0a64a2526..428e9c6f472 100644 --- a/lib/common/src/Dom/Document.php +++ b/lib/common/src/Dom/Document.php @@ -236,6 +236,15 @@ final class Document extends DOMDocument */ private $usedAmpEmoji; + /** + * Store the current index by prefix. + * + * This is used to generate unique-per-prefix IDs. + * + * @var int[] + */ + private $indexCounter = []; + /** * Creates a new AmpProject\Dom\Document object * @@ -314,7 +323,7 @@ public static function fromNode(DOMNode &$node) // We replace the $node by reference, to make sure the next lines of code will // work as expected with the new document. // Otherwise $dom and $node would refer to two different DOMDocuments. - $node = $dom->importNode($root->documentElement ?: $root, true); + $node = $dom->importNode($node, true); $dom->appendChild($node); $dom->hasInitialAmpDevMode = $dom->documentElement->hasAttribute(DevMode::DEV_MODE_ATTRIBUTE); @@ -1474,6 +1483,39 @@ public function isValidHeadNode(DOMNode $node) ); } + /** + * Get the ID for an element. + * + * If the element does not have an ID, create one first. + * + * @param DOMElement $element Element to get the ID for. + * @param string $prefix Optional. The prefix to use (should not have a trailing dash). Defaults to 'i-amp-id'. + * @return string ID to use. + */ + public function getElementId(DOMElement $element, $prefix = 'i-amp-id') + { + if ($element->hasAttribute('id')) { + return $element->getAttribute('id'); + } + + if (array_key_exists($prefix, $this->indexCounter)) { + ++$this->indexCounter[$prefix]; + $id = "{$prefix}-{$this->indexCounter[ $prefix ]}"; + } else { + $id = $prefix; + $this->indexCounter[$prefix] = 1; + } + + while ($this->getElementById($id) instanceof DOMElement) { + ++$this->indexCounter[$prefix]; + $id = "{$prefix}-{$this->indexCounter[ $prefix ]}"; + } + + $element->setAttribute('id', $id); + + return $id; + } + /** * Determine whether `data-ampdevmode` was initially set on the document element. * diff --git a/lib/common/tests/Dom/CssByteCountCalculatorTest.php b/lib/common/tests/Dom/CssByteCountCalculatorTest.php new file mode 100644 index 00000000000..a2b2e672de7 --- /dev/null +++ b/lib/common/tests/Dom/CssByteCountCalculatorTest.php @@ -0,0 +1,58 @@ + [ + '', 5, + ], + 'one_inline_style_attribute' => [ + '
', 5, + ], + 'multiple_inline_style_attributes' => [ + '
', 9, + ], + 'amp_custom_style_tag_and_multiple_inline_style_attributes' => [ + '
', 14, + ], + 'amp_custom_style_tag_outside_head' => [ + '', 5, + ], + 'multibyte_chars_are_counted_in_bytes_not_chars' => [ + '
', 54, + ], + ]; + } + + /** + * Test the calculate() method. + * + * @dataProvider dataCalculate + * @covers CssByteCountCalculator::calculate() + */ + public function testCalculate($html, $expected) + { + $document = Document::fromHtml($html); + $this->assertEquals($expected, (new CssByteCountCalculator($document))->calculate()); + } +} diff --git a/lib/common/tests/Dom/DocumentTest.php b/lib/common/tests/Dom/DocumentTest.php index c8738dc1a7e..d367b6e42fd 100644 --- a/lib/common/tests/Dom/DocumentTest.php +++ b/lib/common/tests/Dom/DocumentTest.php @@ -705,4 +705,105 @@ public function testHasInitialAmpDevMode($document, $hasInitialDevMode) { $this->assertEquals($hasInitialDevMode, $document->hasInitialAmpDevMode()); } + + /** + * Data provider for Dom\Document::getElementId() tests. + * + * @return array + */ + public function getGetElementIdData() + { + $elementFactory = static function ($dom, $id = null) { + $element = $dom->createElement('div'); + + if ($id) { + $element->setAttribute('id', $id); + } + + $dom->body->appendChild($element); + + return $element; + }; + + return [ + 'single check with existing ID' => [ + [ + [ $elementFactory, 'my-id', 'some-prefix', 'my-id' ], + ], + ], + + 'single check without existing ID' => [ + [ + [ $elementFactory, null, 'some-prefix', 'some-prefix' ], + ], + ], + + 'consecutive checks count upwards' => [ + [ + [ $elementFactory, null, 'some-prefix', 'some-prefix' ], + [ $elementFactory, null, 'some-prefix', 'some-prefix-2' ], + ], + ], + + 'consecutive checks for same element return same ID' => [ + [ + [ $elementFactory, null, 'some-prefix', 'some-prefix' ], + [ null, null, 'some-prefix', 'some-prefix' ], + ], + ], + + 'mixing prefixes keeps counts separate' => [ + [ + [ $elementFactory, 'my-id', 'some-prefix', 'my-id' ], + [ $elementFactory, null, 'some-prefix', 'some-prefix' ], + [ $elementFactory, null, 'some-prefix', 'some-prefix-2' ], + [ $elementFactory, null, 'other-prefix', 'other-prefix' ], + [ $elementFactory, null, 'other-prefix', 'other-prefix-2' ], + [ $elementFactory, null, 'some-prefix', 'some-prefix-3' ], + [ $elementFactory, 'another-id', 'some-prefix', 'another-id' ], + [ $elementFactory, null, 'some-prefix', 'some-prefix-4' ], + [ null, null, 'some-prefix', 'some-prefix-4' ], + ], + ], + ]; + } + + /** + * Test Document::getElementId(). + * + * @dataProvider getGetElementIdData + * @covers Document::getElementId() + * + * @param array $checks Checks to perform. Each check is an array containing an element, a prefix and an expected ID. + */ + public function testGetElementId($checks) + { + $dom = new Document(); + foreach ($checks as list($elementFactory, $id, $prefix, $expected)) { + // If no element factory was passed, just reuse the previous element. + if ($elementFactory) { + $element = $elementFactory($dom, $id); + } + + $actual = $dom->getElementId($element, $prefix); + $this->assertEquals($expected, $actual); + } + } + + /** + * Test whether existing element IDs are taken into account, even if the index counter is off. + * + * @covers Document::getElementId() + */ + public function testGetElementIdOnPreexistingIds() + { + $dom = Document::fromHtml( + '
' + ); + + $element = $dom->createElement('div'); + $dom->body->appendChild($element); + + $this->assertEquals('some-prefix-4', $dom->getElementId($element, 'some-prefix')); + } } diff --git a/lib/optimizer/composer.json b/lib/optimizer/composer.json index 30b1e8bcad9..ce095175d7a 100644 --- a/lib/optimizer/composer.json +++ b/lib/optimizer/composer.json @@ -8,12 +8,12 @@ "ext-dom": "*", "ext-iconv": "*", "ext-libxml": "*", - "ampproject/common": "^1" + "ampproject/common": "^1", + "php-parallel-lint/php-parallel-lint": "^1.2" }, "require-dev": { "ext-zip": "*", "civicrm/composer-downloads-plugin": "^2.1", - "jakub-onderka/php-parallel-lint": "^1.0", "roave/security-advisories": "dev-master", "squizlabs/php_codesniffer": "^3" }, diff --git a/lib/optimizer/resources/local_fallback/rtv/metadata b/lib/optimizer/resources/local_fallback/rtv/metadata index 031b783c4a1..027f378ccb3 100644 --- a/lib/optimizer/resources/local_fallback/rtv/metadata +++ b/lib/optimizer/resources/local_fallback/rtv/metadata @@ -1 +1 @@ -{"ampRuntimeVersion":"012004252135000","ampCssUrl":"https://cdn.ampproject.org/rtv/012004252135000/v0.css","canaryPercentage":"0.005","diversions":["002004252135000","002005050322000","022004252135000","032004252135000","032005050322000","042005051629000","052004252135000","102004252135000"],"ltsRuntimeVersion":"012004030010070","ltsCssUrl":"https://cdn.ampproject.org/rtv/012004030010070/v0.css"} \ No newline at end of file +{"ampRuntimeVersion":"012004252135000","ampCssUrl":"https://cdn.ampproject.org/rtv/012004252135000/v0.css","canaryPercentage":"0.005","diversions":["002005050322000","022004252135000","032005050322000","042005062312000","052004252135000","102005050322000"],"ltsRuntimeVersion":"012004030010070","ltsCssUrl":"https://cdn.ampproject.org/rtv/012004030010070/v0.css"} \ No newline at end of file diff --git a/lib/optimizer/src/Error/CannotRemoveBoilerplate.php b/lib/optimizer/src/Error/CannotRemoveBoilerplate.php index 94473487636..4f9f5c2e791 100644 --- a/lib/optimizer/src/Error/CannotRemoveBoilerplate.php +++ b/lib/optimizer/src/Error/CannotRemoveBoilerplate.php @@ -4,12 +4,14 @@ use AmpProject\Optimizer\Error; use DOMElement; +use Exception; final class CannotRemoveBoilerplate implements Error { use ErrorProperties; - const ATTRIBUTES_STRING = 'Cannot remove boilerplate as either heights, media or sizes attribute is set: '; + const ATTRIBUTES_STRING = 'Cannot remove boilerplate as either heights, media or sizes attribute is set and cannot be adapted: '; + const ATTRIBUTES_EXCEPTION_STRING = 'Cannot remove boilerplate as the removal of either heights, media or sizes attribute produced an error: '; const RENDER_DELAYING_SCRIPT_STRING = 'Cannot remove boilerplate because the document contains a render-delaying extension: '; const AMP_AUDIO_STRING = 'Cannot remove boilerplate because the document contains an extension that needs to know the dimensions of the browser: '; const UNSUPPORTED_LAYOUT_STRING = 'Cannot remove boilerplate because of an unsupported layout: '; @@ -25,6 +27,17 @@ public static function fromAttributesRequiringBoilerplate(DOMElement $element) return new self(self::ATTRIBUTES_STRING . new ElementDump($element)); } + /** + * Instantiate a CannotRemoveBoilerplate object for attributes that require the boilerplate to be around. + * + * @param Exception $exception Exception being thrown. + * @return self + */ + public static function fromAttributeThrowingException($exception) + { + return new self(self::ATTRIBUTES_EXCEPTION_STRING . $exception->getMessage()); + } + /** * Instantiate a CannotRemoveBoilerplate object for an amp-experiment element. * diff --git a/lib/optimizer/src/Exception/InvalidHtmlAttribute.php b/lib/optimizer/src/Exception/InvalidHtmlAttribute.php new file mode 100644 index 00000000000..5e306835071 --- /dev/null +++ b/lib/optimizer/src/Exception/InvalidHtmlAttribute.php @@ -0,0 +1,30 @@ + tag, relative to the node. + * + * @var string + */ + const STYLE_AMP_CUSTOM_XPATH = './/style[@amp-custom]'; + + /** + * Regex pattern to match a CSS Dimension with an associated media condition. + * + * @var string + */ + const CSS_DIMENSION_WITH_MEDIA_CONDITION_REGEX_PATTERN = '/(?.*)\s(?.*)/m'; + + /** + * Characters to use for trimming CSS values. + * + * @var string + */ + const CSS_TRIM_CHARACTERS = " \t\n\r\0\x0B;"; + + /** + * Maximum size of the CSS styles in bytes. + * + * @todo Max size is hard-coded for now until we ported over the generated spec into a reusable package. + * + * @var int + */ + const MAX_CSS_BYTE_COUNT = 75000; + + /** + * The ' + ), + [], ], - 'media attribute' => [ - $input(''), - $expectWithBoilerplate(''), - [Error\CannotRemoveBoilerplate::class], + 'sizes attribute with amp-custom' => [ + $input( + '', + '' + ), + $expectWithoutBoilerplate( + '', + '' + ), + [], ], - 'sizes attribute' => [ + // According to the Mozilla docs, a sizes attribute without a valid srcset attribute should have no effect. + // Therefore, it should simply be stripped, without producing media queries. + // @see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img#attr-sizes + 'sizes attribute without srcset' => [ $input(''), - $expectWithBoilerplate(''), - [Error\CannotRemoveBoilerplate::class], + $expectWithoutBoilerplate( + '' + ), + [], + ], + + 'sizes attribute empty srcset' => [ + $input(''), + $expectWithoutBoilerplate( + '' + ), + [], + ], + + 'sizes attribute with disable-inline-width' => [ + $input(''), + $expectWithoutBoilerplate( + '' + ), + [], + ], + + 'bad sizes attribute' => [ + $input(''), + $expectWithBoilerplate(''), + [ + Error\CannotRemoveBoilerplate::fromAttributeThrowingException( + InvalidHtmlAttribute::fromAttribute( + 'sizes', + Document::fromHtmlFragment( + '' + )->body->firstChild + ) + ), + ], + ], + + 'heights attribute without amp-custom' => [ + $input(''), + $expectWithoutBoilerplate( + '', + '' + ), + [], + ], + + 'heights attribute with amp-custom' => [ + $input( + '', + '' + ), + $expectWithoutBoilerplate( + '', + '' + ), + [], + ], + + 'bad heights attribute' => [ + $input(''), + // This adds an ID as it stores the CSS to inline before the actual error is detected. + $expectWithBoilerplate(''), + [ + Error\CannotRemoveBoilerplate::fromAttributeThrowingException( + InvalidHtmlAttribute::fromAttribute( + 'heights', + Document::fromHtmlFragment( + '' + )->body->firstChild + ) + ), + ], ], // @todo Remove floor when ampproject/amphtml#27528 is resolved. @@ -173,6 +280,36 @@ public function dataTransform() $input(''), $expectWithoutBoilerplate(''), ], + + 'media attribute without amp-custom' => [ + $input(''), + $expectWithoutBoilerplate( + '', + '' + ), + [], + ], + + 'media attribute with amp-custom' => [ + $input( + '', + '' + ), + $expectWithoutBoilerplate( + '', + '' + ), + [], + ], + + 'media attribute with type condition' => [ + $input(''), + $expectWithoutBoilerplate( + '', + '' + ), + [], + ], ]; } @@ -194,7 +331,7 @@ public function testTransform($source, $expectedHtml, $expectedErrors = []) $transformer->transform($document, $errors); - $this->assertEqualMarkup($expectedHtml, $document->saveHTML()); + $this->assertSimilarMarkup($expectedHtml, $document->saveHTML()); $this->assertSameErrors($expectedErrors, $errors); } } diff --git a/tests/php/test-class-amp-dom-utils.php b/tests/php/test-class-amp-dom-utils.php index ca57c58d1cc..d6d3ff74aa8 100644 --- a/tests/php/test-class-amp-dom-utils.php +++ b/tests/php/test-class-amp-dom-utils.php @@ -349,29 +349,58 @@ public function test_has_class( DOMElement $element, $class, $expected ) { } public function get_get_element_id_data() { - $dom = new Document(); + $element_factory = static function ( $dom, $id = null ) { + $element = $dom->createElement( 'div' ); + + if ( $id ) { + $element->setAttribute( 'id', $id ); + } - $same_element = AMP_DOM_Utils::create_node( $dom, 'div', [] ); + $dom->body->appendChild( $element ); + + return $element; + }; return [ - // Element with existing ID - [ AMP_DOM_Utils::create_node( $dom, 'div', [ 'id' => 'my-id' ] ), 'some-prefix', 'my-id' ], - // Element without ID - [ AMP_DOM_Utils::create_node( $dom, 'div', [] ), 'some-prefix', 'some-prefix' ], - // Another element without ID with same prefix - [ AMP_DOM_Utils::create_node( $dom, 'div', [] ), 'some-prefix', 'some-prefix-2' ], - // Another element without ID with different prefix - [ AMP_DOM_Utils::create_node( $dom, 'div', [] ), 'other-prefix', 'other-prefix' ], - // Another element without ID with different prefix again - [ AMP_DOM_Utils::create_node( $dom, 'div', [] ), 'other-prefix', 'other-prefix-2' ], - // Another element without ID with first prefix again - [ AMP_DOM_Utils::create_node( $dom, 'div', [] ), 'some-prefix', 'some-prefix-3' ], - // Element with existing prefix again - [ AMP_DOM_Utils::create_node( $dom, 'div', [ 'id' => 'another-id' ] ), 'some-prefix', 'another-id' ], - // Same element first time - [ $same_element, 'some-prefix', 'some-prefix-4' ], - // Same element second time - [ $same_element, 'some-prefix', 'some-prefix-4' ], + 'single check with existing ID' => [ + [ + [ $element_factory, 'my-id', 'some-prefix', 'my-id' ], + ], + ], + + 'single check without existing ID' => [ + [ + [ $element_factory, null, 'some-prefix', 'some-prefix' ], + ], + ], + + 'consecutive checks count upwards' => [ + [ + [ $element_factory, null, 'some-prefix', 'some-prefix' ], + [ $element_factory, null, 'some-prefix', 'some-prefix-2' ], + ], + ], + + 'consecutive checks for same element return same ID' => [ + [ + [ $element_factory, null, 'some-prefix', 'some-prefix' ], + [ null, null, 'some-prefix', 'some-prefix' ], + ], + ], + + 'mixing prefixes keeps counts separate' => [ + [ + [ $element_factory, 'my-id', 'some-prefix', 'my-id' ], + [ $element_factory, null, 'some-prefix', 'some-prefix' ], + [ $element_factory, null, 'some-prefix', 'some-prefix-2' ], + [ $element_factory, null, 'other-prefix', 'other-prefix' ], + [ $element_factory, null, 'other-prefix', 'other-prefix-2' ], + [ $element_factory, null, 'some-prefix', 'some-prefix-3' ], + [ $element_factory, 'another-id', 'some-prefix', 'another-id' ], + [ $element_factory, null, 'some-prefix', 'some-prefix-4' ], + [ null, null, 'some-prefix', 'some-prefix-4' ], + ], + ], ]; } @@ -381,13 +410,21 @@ public function get_get_element_id_data() { * @dataProvider get_get_element_id_data * @covers \AMP_DOM_Utils::get_element_id() * - * @param DOMElement $element Element. - * @param string $prefix Prefix. - * @param string $expected Expected element ID. + * @expectedDeprecated AMP_DOM_Utils::get_element_id + * + * @param array $checks Checks to perform. Each check is an array containing an element, a prefix and an expected ID. */ - public function test_get_element_id( DOMElement $element, $prefix, $expected ) { - $actual = AMP_DOM_Utils::get_element_id( $element, $prefix ); - $this->assertEquals( $expected, $actual ); + public function test_get_element_id( $checks ) { + $dom = new Document(); + foreach ( $checks as list( $element_factory, $id, $prefix, $expected ) ) { + // If no element factory was passed, just reuse the previous element. + if ( $element_factory ) { + $element = $element_factory( $dom, $id ); + } + + $actual = AMP_DOM_Utils::get_element_id( $element, $prefix ); + $this->assertEquals( $expected, $actual ); + } } public function get_add_amp_action_data() {