ENGCOM-7678: Fix duplicate css when enable critical path #28480

 - Merge Pull Request #28480 from mrtuvn/magento2:fix-duplicate-css-when-enable-critical-path
 - Merged commits:
   1. bacb817

Author: magento-engcom-team

app/code/Magento/Theme/Controller/Result/AsyncCssPlugin.php

@@ -10,6 +10,9 @@
 use Magento\Framework\App\Config\ScopeConfigInterface;
 use Magento\Store\Model\ScopeInterface;
 use Magento\Framework\App\Response\Http;
+use Magento\Framework\App\Response\HttpInterface as HttpResponseInterface;
+use Magento\Framework\App\ResponseInterface;
+use Magento\Framework\View\Result\Layout;
 
 /**
  * Plugin for asynchronous CSS loading.
@@ -32,48 +35,94 @@ public function __construct(ScopeConfigInterface $scopeConfig)
     }
 
     /**
-     * Load CSS asynchronously if it is enabled in configuration.
+     * Extracts styles to head after critical css if critical path feature is enabled.
      *
-     * @param Http $subject
-     * @return void
+     * @param Layout $subject
+     * @param Layout $result
+     * @param HttpResponseInterface|ResponseInterface $httpResponse
+     * @return Layout (That should be void, actually)
+     * @SuppressWarnings(PHPMD.UnusedFormalParameter)
      */
-    public function beforeSendResponse(Http $subject): void
+    public function afterRenderResult(Layout $subject, Layout $result, ResponseInterface $httpResponse)
     {
-        $content = $subject->getContent();
+        if (!$this->isCssCriticalEnabled()) {
+            return $result;
+        }
 
-        if (\is_string($content) && strpos($content, '</body') !== false && $this->scopeConfig->isSetFlag(
-            self::XML_PATH_USE_CSS_CRITICAL_PATH,
-            ScopeInterface::SCOPE_STORE
-        )) {
-            $cssMatches = [];
-            // add link rel preload to style sheets
-            $content = preg_replace_callback(
-                '@<link\b.*?rel=("|\')stylesheet\1.*?/>@',
-                function ($matches) use (&$cssMatches) {
-                    $cssMatches[] = $matches[0];
-                    preg_match('@href=("|\')(.*?)\1@', $matches[0], $hrefAttribute);
-                    $href = $hrefAttribute[2];
-                    if (preg_match('@media=("|\')(.*?)\1@', $matches[0], $mediaAttribute)) {
-                        $media = $mediaAttribute[2];
-                    }
-                    $media = $media ?? 'all';
-                    $loadCssAsync = sprintf(
-                        '<link rel="preload" as="style" media="%s"' .
-                         ' onload="this.onload=null;this.rel=\'stylesheet\'"' .
-                        ' href="%s" />',
-                        $media,
-                        $href
-                    );
-
-                    return $loadCssAsync;
-                },
-                $content
-            );
+        $content = (string)$httpResponse->getContent();
+        $headCloseTag = '</head>';
 
-            if (!empty($cssMatches)) {
-                $content = str_replace('</body', implode("\n", $cssMatches) . "\n</body", $content);
-                $subject->setContent($content);
+        $headEndTagFound = strpos($content, $headCloseTag) !== false;
+
+        if ($headEndTagFound) {
+            $styles = $this->extractLinkTags($content);
+            if ($styles) {
+                $newHeadEndTagPosition = strrpos($content, $headCloseTag);
+                $content = substr_replace($content, $styles . "\n", $newHeadEndTagPosition, 0);
+                $httpResponse->setContent($content);
             }
         }
+
+        return $result;
+    }
+
+    /**
+     * Extracts link tags found in given content.
+     *
+     * @param string $content
+     */
+    private function extractLinkTags(string &$content): string
+    {
+        $styles = '';
+        $styleOpen = '<link';
+        $styleClose = '>';
+        $styleOpenPos = strpos($content, $styleOpen);
+
+        while ($styleOpenPos !== false) {
+            $styleClosePos = strpos($content, $styleClose, $styleOpenPos);
+            $style = substr($content, $styleOpenPos, $styleClosePos - $styleOpenPos + strlen($styleClose));
+
+            if (!preg_match('@rel=["\']stylesheet["\']@', $style)) {
+                // Link is not a stylesheet, search for another one after it.
+                $styleOpenPos = strpos($content, $styleOpen, $styleClosePos);
+                continue;
+            }
+            // Remove the link from HTML to add it before </head> tag later.
+            $content = str_replace($style, '', $content);
+
+            if (!preg_match('@href=("|\')(.*?)\1@', $style, $hrefAttribute)) {
+                throw new \RuntimeException("Invalid link {$style} syntax provided");
+            }
+            $href = $hrefAttribute[2];
+
+            if (preg_match('@media=("|\')(.*?)\1@', $style, $mediaAttribute)) {
+                $media = $mediaAttribute[2];
+            }
+            $media = $media ?? 'all';
+
+            $style = sprintf(
+                '<link rel="stylesheet" media="print" onload="this.onload=null;this.media=\'%s\'" href="%s">',
+                $media,
+                $href
+            );
+            $styles .= "\n" . $style;
+            // Link was cut out, search for the next one at its former position.
+            $styleOpenPos = strpos($content, $styleOpen, $styleOpenPos);
+        }
+
+        return $styles;
+    }
+
+    /**
+     * Returns information whether css critical path is enabled
+     *
+     * @return bool
+     */
+    private function isCssCriticalEnabled(): bool
+    {
+        return $this->scopeConfig->isSetFlag(
+            self::XML_PATH_USE_CSS_CRITICAL_PATH,
+            ScopeInterface::SCOPE_STORE
+        );
     }
 }

app/code/Magento/Theme/Test/Unit/Controller/Result/AsyncCssPluginTest.php

@@ -7,13 +7,14 @@
 
 namespace Magento\Theme\Test\Unit\Controller\Result;
 
-use Magento\Framework\App\Config\ScopeConfigInterface;
-use Magento\Framework\App\Response\Http;
-use Magento\Framework\TestFramework\Unit\Helper\ObjectManager as ObjectManagerHelper;
-use Magento\Store\Model\ScopeInterface;
 use Magento\Theme\Controller\Result\AsyncCssPlugin;
-use PHPUnit\Framework\MockObject\MockObject;
+use Magento\Framework\App\Response\Http;
 use PHPUnit\Framework\TestCase;
+use PHPUnit\Framework\MockObject\MockObject;
+use Magento\Framework\App\Config\ScopeConfigInterface;
+use Magento\Store\Model\ScopeInterface;
+use Magento\Framework\View\Result\Layout;
+use Magento\Framework\TestFramework\Unit\Helper\ObjectManager as ObjectManagerHelper;
 
 /**
  * Unit test for Magento\Theme\Test\Unit\Controller\Result\AsyncCssPlugin.
@@ -37,6 +38,9 @@ class AsyncCssPluginTest extends TestCase
      */
     private $httpMock;
 
+    /** @var Layout|MockObject */
+    private $layoutMock;
+
     /**
      * @inheritdoc
      */
@@ -48,6 +52,7 @@ protected function setUp(): void
             ->getMockForAbstractClass();
 
         $this->httpMock = $this->createMock(Http::class);
+        $this->layoutMock = $this->createMock(Layout::class);
 
         $objectManager = new ObjectManagerHelper($this);
         $this->plugin = $objectManager->getObject(
@@ -59,87 +64,134 @@ protected function setUp(): void
     }
 
     /**
-     * Data Provider for before send response
+     * Data Provider for testAfterRenderResult
      *
      * @return array
      */
-    public function sendResponseDataProvider(): array
+    public function renderResultDataProvider(): array
     {
         return [
             [
-                "content" => "<body><h1>Test Title</h1>" .
-                    "<link rel=\"stylesheet\" href=\"css/critical.css\" />" .
-                    "<p>Test Content</p></body>",
+                "content" => "<head><link rel=\"stylesheet\" href=\"css/async.css\">" .
+                    "<style>.critical-css{}</style>" .
+                    "</head>",
+                "flag" => true,
+                "result" => "<head><style>.critical-css{}</style>\n" .
+                    "<link " .
+                        "rel=\"stylesheet\" media=\"print\" onload=\"this.onload=null;this.media='all'\" " .
+                        "href=\"css/async.css\">\n" .
+                    "</head>",
+            ],
+            [
+                "content" => "<head><link rel=\"stylesheet\" href=\"css/async.css\">" .
+                    "<link rel=\"preload\" href=\"other-file.html\">" .
+                    "</head>",
                 "flag" => true,
-                "result" => "<body><h1>Test Title</h1>" .
-                    "<link rel=\"preload\" as=\"style\" media=\"all\"" .
-                    " onload=\"this.onload=null;this.rel='stylesheet'\" href=\"css/critical.css\" />" .
-                    "<p>Test Content</p>" .
-                    "<link rel=\"stylesheet\" href=\"css/critical.css\" />" .
-                    "\n</body>"
+                "result" => "<head><link rel=\"preload\" href=\"other-file.html\">\n" .
+                    "<link " .
+                        "rel=\"stylesheet\" media=\"print\" onload=\"this.onload=null;this.media='all'\" " .
+                        "href=\"css/async.css\">\n" .
+                    "</head>",
             ],
             [
-                "content" => "<body><p>Test Content</p></body>",
+                "content" => "<head><link rel=\"stylesheet\" href=\"css/async.css\">" .
+                    "<link rel=\"preload\" href=\"other-file.html\">" .
+                    "</head>",
                 "flag" => false,
-                "result" => "<body><p>Test Content</p></body>"
+                "result" => "<head><link rel=\"stylesheet\" href=\"css/async.css\">" .
+                    "<link rel=\"preload\" href=\"other-file.html\">" .
+                    "</head>",
             ],
             [
-                "content" => "<body><p>Test Content</p></body>",
+                "content" => "<head><link rel=\"stylesheet\" href=\"css/first.css\">" .
+                    "<link rel=\"stylesheet\" href=\"css/second.css\">" .
+                    "<style>.critical-css{}</style>" .
+                    "</head>",
                 "flag" => true,
-                "result" => "<body><p>Test Content</p></body>"
+                "result" => "<head><style>.critical-css{}</style>\n" .
+                    "<link " .
+                        "rel=\"stylesheet\" media=\"print\" onload=\"this.onload=null;this.media='all'\" " .
+                        "href=\"css/first.css\">\n" .
+                    "<link " .
+                        "rel=\"stylesheet\" media=\"print\" onload=\"this.onload=null;this.media='all'\" " .
+                        "href=\"css/second.css\">\n" .
+                    "</head>",
+            ],
+            [
+                "content" => "<head><style>.critical-css{}</style></head>",
+                "flag" => false,
+                "result" => "<head><style>.critical-css{}</style></head>"
+            ],
+            [
+                "content" => "<head><style>.critical-css{}</style></head>",
+                "flag" => true,
+                "result" => "<head><style>.critical-css{}</style></head>"
             ]
         ];
     }
 
     /**
-     * Test beforeSendResponse
+     * Test after render result response
      *
      * @param string $content
      * @param bool $isSetFlag
      * @param string $result
      * @return void
-     * @dataProvider sendResponseDataProvider
+     * @dataProvider renderResultDataProvider
      */
-    public function testBeforeSendResponse($content, $isSetFlag, $result): void
+    public function testAfterRenderResult(string $content, bool $isSetFlag, string $result): void
     {
-        $this->httpMock->expects($this->once())
-            ->method('getContent')
+        // Given (context)
+        $this->httpMock->method('getContent')
             ->willReturn($content);
 
-        $this->scopeConfigMock->expects($this->once())
-            ->method('isSetFlag')
-            ->with(
-                self::STUB_XML_PATH_USE_CSS_CRITICAL_PATH,
-                ScopeInterface::SCOPE_STORE
-            )
+        $this->scopeConfigMock->method('isSetFlag')
+            ->with(self::STUB_XML_PATH_USE_CSS_CRITICAL_PATH, ScopeInterface::SCOPE_STORE)
             ->willReturn($isSetFlag);
 
+        // Expects
         $this->httpMock->expects($this->any())
             ->method('setContent')
             ->with($result);
 
-        $this->plugin->beforeSendResponse($this->httpMock);
+        // When
+        $this->plugin->afterRenderResult($this->layoutMock, $this->layoutMock, $this->httpMock);
     }
 
     /**
-     * Test BeforeSendResponse if content is not a string
+     * Data Provider for testAfterRenderResultIfGetContentIsNotAString()
      *
+     * @return array
+     */
+    public function ifGetContentIsNotAStringDataProvider(): array
+    {
+        return [
+            [
+                'content' => null
+            ]
+        ];
+    }
+
+    /**
+     * Test AfterRenderResult if content is not a string
+     *
+     * @param $content
      * @return void
+     * @dataProvider ifGetContentIsNotAStringDataProvider
      */
-    public function testIfGetContentIsNotAString(): void
+    public function testAfterRenderResultIfGetContentIsNotAString($content): void
     {
+        $this->scopeConfigMock->method('isSetFlag')
+            ->with(self::STUB_XML_PATH_USE_CSS_CRITICAL_PATH, ScopeInterface::SCOPE_STORE)
+            ->willReturn(true);
+
         $this->httpMock->expects($this->once())
             ->method('getContent')
-            ->willReturn([]);
+            ->willReturn($content);
 
-        $this->scopeConfigMock->expects($this->any())
-            ->method('isSetFlag')
-            ->with(
-                self::STUB_XML_PATH_USE_CSS_CRITICAL_PATH,
-                ScopeInterface::SCOPE_STORE
-            )
-            ->willReturn(false);
+        $this->httpMock->expects($this->never())
+            ->method('setContent');
 
-        $this->plugin->beforeSendResponse($this->httpMock);
+        $this->plugin->afterRenderResult($this->layoutMock, $this->layoutMock, $this->httpMock);
     }
 }

app/code/Magento/Theme/etc/frontend/di.xml

@@ -26,11 +26,9 @@
     <type name="Magento\Framework\Controller\ResultInterface">
         <plugin name="result-messages" type="Magento\Theme\Controller\Result\MessagePlugin"/>
     </type>
-    <type name="Magento\Framework\App\Response\Http">
-        <plugin name="asyncCssLoad" type="Magento\Theme\Controller\Result\AsyncCssPlugin"/>
-    </type>
     <type name="Magento\Framework\View\Result\Layout">
-        <plugin name="deferJsToFooter" type="Magento\Theme\Controller\Result\JsFooterPlugin" sortOrder="-10"/>
+        <plugin name="asyncCssLoad" type="Magento\Theme\Controller\Result\AsyncCssPlugin" />
+        <plugin name="deferJsToFooter" type="Magento\Theme\Controller\Result\JsFooterPlugin" sortOrder="-10" />
     </type>
     <type name="Magento\Theme\Block\Html\Header\CriticalCss">
         <arguments>

app/code/Magento/Theme/view/frontend/layout/default_head_blocks.xml

@@ -18,6 +18,7 @@
                     <argument name="criticalCssViewModel" xsi:type="object">Magento\Theme\Block\Html\Header\CriticalCss</argument>
                 </arguments>
             </block>
+            <!-- Todo: Block css_rel_preload_script will be removed in next release as polyfill isn't used anymore -->
             <block name="css_rel_preload_script" ifconfig="dev/css/use_css_critical_path" template="Magento_Theme::js/css_rel_preload.phtml"/>
         </referenceBlock>
         <referenceContainer name="after.body.start">

app/code/Magento/Theme/view/frontend/templates/html/main_css_preloader.phtml

@@ -4,12 +4,17 @@
  * See COPYING.txt for license details.
  */
 
-/** @var \Magento\Framework\View\Helper\SecureHtmlRenderer $secureRenderer */
+/**
+ * @var \Magento\Framework\View\Element\Template $block
+ * @var \Magento\Framework\Escaper $escaper
+ * @var \Magento\Framework\View\Helper\SecureHtmlRenderer $secureRenderer
+ */
+
 ?>
 <div data-role="main-css-loader" class="loading-mask">
     <div class="loader">
-        <img src="<?= $block->escapeUrl($block->getViewFileUrl('images/loader-1.gif')); ?>"
-             alt="<?= $block->escapeHtml(__('Loading...')); ?>">
+        <img src="<?= $escaper->escapeUrl($block->getViewFileUrl('images/loader-1.gif')); ?>"
+             alt="<?= $escaper->escapeHtml(__('Loading...')); ?>">
     </div>
     <?= /* @noEscape */ $secureRenderer->renderStyleAsTag(
         "position: absolute;",