Skip to content

Commit bacb817

Browse files
author
Tu Nguyen
committed
Fix css shows twice when css critical path enabled
update fix permissions Fix static check update fix fail tests update add extra condition check update update update update update update Fix integration test update Fix static test fail
1 parent 5daadae commit bacb817

File tree

7 files changed

+203
-94
lines changed

7 files changed

+203
-94
lines changed

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

Lines changed: 85 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@
1010
use Magento\Framework\App\Config\ScopeConfigInterface;
1111
use Magento\Store\Model\ScopeInterface;
1212
use Magento\Framework\App\Response\Http;
13+
use Magento\Framework\App\Response\HttpInterface as HttpResponseInterface;
14+
use Magento\Framework\App\ResponseInterface;
15+
use Magento\Framework\View\Result\Layout;
1316

1417
/**
1518
* Plugin for asynchronous CSS loading.
@@ -32,48 +35,94 @@ public function __construct(ScopeConfigInterface $scopeConfig)
3235
}
3336

3437
/**
35-
* Load CSS asynchronously if it is enabled in configuration.
38+
* Extracts styles to head after critical css if critical path feature is enabled.
3639
*
37-
* @param Http $subject
38-
* @return void
40+
* @param Layout $subject
41+
* @param Layout $result
42+
* @param HttpResponseInterface|ResponseInterface $httpResponse
43+
* @return Layout (That should be void, actually)
44+
* @SuppressWarnings(PHPMD.UnusedFormalParameter)
3945
*/
40-
public function beforeSendResponse(Http $subject): void
46+
public function afterRenderResult(Layout $subject, Layout $result, ResponseInterface $httpResponse)
4147
{
42-
$content = $subject->getContent();
48+
if (!$this->isCssCriticalEnabled()) {
49+
return $result;
50+
}
4351

44-
if (\is_string($content) && strpos($content, '</body') !== false && $this->scopeConfig->isSetFlag(
45-
self::XML_PATH_USE_CSS_CRITICAL_PATH,
46-
ScopeInterface::SCOPE_STORE
47-
)) {
48-
$cssMatches = [];
49-
// add link rel preload to style sheets
50-
$content = preg_replace_callback(
51-
'@<link\b.*?rel=("|\')stylesheet\1.*?/>@',
52-
function ($matches) use (&$cssMatches) {
53-
$cssMatches[] = $matches[0];
54-
preg_match('@href=("|\')(.*?)\1@', $matches[0], $hrefAttribute);
55-
$href = $hrefAttribute[2];
56-
if (preg_match('@media=("|\')(.*?)\1@', $matches[0], $mediaAttribute)) {
57-
$media = $mediaAttribute[2];
58-
}
59-
$media = $media ?? 'all';
60-
$loadCssAsync = sprintf(
61-
'<link rel="preload" as="style" media="%s"' .
62-
' onload="this.onload=null;this.rel=\'stylesheet\'"' .
63-
' href="%s" />',
64-
$media,
65-
$href
66-
);
67-
68-
return $loadCssAsync;
69-
},
70-
$content
71-
);
52+
$content = (string)$httpResponse->getContent();
53+
$headCloseTag = '</head>';
7254

73-
if (!empty($cssMatches)) {
74-
$content = str_replace('</body', implode("\n", $cssMatches) . "\n</body", $content);
75-
$subject->setContent($content);
55+
$headEndTagFound = strpos($content, $headCloseTag) !== false;
56+
57+
if ($headEndTagFound) {
58+
$styles = $this->extractLinkTags($content);
59+
if ($styles) {
60+
$newHeadEndTagPosition = strrpos($content, $headCloseTag);
61+
$content = substr_replace($content, $styles . "\n", $newHeadEndTagPosition, 0);
62+
$httpResponse->setContent($content);
7663
}
7764
}
65+
66+
return $result;
67+
}
68+
69+
/**
70+
* Extracts link tags found in given content.
71+
*
72+
* @param string $content
73+
*/
74+
private function extractLinkTags(string &$content): string
75+
{
76+
$styles = '';
77+
$styleOpen = '<link';
78+
$styleClose = '>';
79+
$styleOpenPos = strpos($content, $styleOpen);
80+
81+
while ($styleOpenPos !== false) {
82+
$styleClosePos = strpos($content, $styleClose, $styleOpenPos);
83+
$style = substr($content, $styleOpenPos, $styleClosePos - $styleOpenPos + strlen($styleClose));
84+
85+
if (!preg_match('@rel=["\']stylesheet["\']@', $style)) {
86+
// Link is not a stylesheet, search for another one after it.
87+
$styleOpenPos = strpos($content, $styleOpen, $styleClosePos);
88+
continue;
89+
}
90+
// Remove the link from HTML to add it before </head> tag later.
91+
$content = str_replace($style, '', $content);
92+
93+
if (!preg_match('@href=("|\')(.*?)\1@', $style, $hrefAttribute)) {
94+
throw new \RuntimeException("Invalid link {$style} syntax provided");
95+
}
96+
$href = $hrefAttribute[2];
97+
98+
if (preg_match('@media=("|\')(.*?)\1@', $style, $mediaAttribute)) {
99+
$media = $mediaAttribute[2];
100+
}
101+
$media = $media ?? 'all';
102+
103+
$style = sprintf(
104+
'<link rel="stylesheet" media="print" onload="this.onload=null;this.media=\'%s\'" href="%s">',
105+
$media,
106+
$href
107+
);
108+
$styles .= "\n" . $style;
109+
// Link was cut out, search for the next one at its former position.
110+
$styleOpenPos = strpos($content, $styleOpen, $styleOpenPos);
111+
}
112+
113+
return $styles;
114+
}
115+
116+
/**
117+
* Returns information whether css critical path is enabled
118+
*
119+
* @return bool
120+
*/
121+
private function isCssCriticalEnabled(): bool
122+
{
123+
return $this->scopeConfig->isSetFlag(
124+
self::XML_PATH_USE_CSS_CRITICAL_PATH,
125+
ScopeInterface::SCOPE_STORE
126+
);
78127
}
79128
}

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

Lines changed: 95 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,14 @@
77

88
namespace Magento\Theme\Test\Unit\Controller\Result;
99

10-
use Magento\Framework\App\Config\ScopeConfigInterface;
11-
use Magento\Framework\App\Response\Http;
12-
use Magento\Framework\TestFramework\Unit\Helper\ObjectManager as ObjectManagerHelper;
13-
use Magento\Store\Model\ScopeInterface;
1410
use Magento\Theme\Controller\Result\AsyncCssPlugin;
15-
use PHPUnit\Framework\MockObject\MockObject;
11+
use Magento\Framework\App\Response\Http;
1612
use PHPUnit\Framework\TestCase;
13+
use PHPUnit\Framework\MockObject\MockObject;
14+
use Magento\Framework\App\Config\ScopeConfigInterface;
15+
use Magento\Store\Model\ScopeInterface;
16+
use Magento\Framework\View\Result\Layout;
17+
use Magento\Framework\TestFramework\Unit\Helper\ObjectManager as ObjectManagerHelper;
1718

1819
/**
1920
* Unit test for Magento\Theme\Test\Unit\Controller\Result\AsyncCssPlugin.
@@ -37,6 +38,9 @@ class AsyncCssPluginTest extends TestCase
3738
*/
3839
private $httpMock;
3940

41+
/** @var Layout|MockObject */
42+
private $layoutMock;
43+
4044
/**
4145
* @inheritdoc
4246
*/
@@ -48,6 +52,7 @@ protected function setUp(): void
4852
->getMockForAbstractClass();
4953

5054
$this->httpMock = $this->createMock(Http::class);
55+
$this->layoutMock = $this->createMock(Layout::class);
5156

5257
$objectManager = new ObjectManagerHelper($this);
5358
$this->plugin = $objectManager->getObject(
@@ -59,87 +64,134 @@ protected function setUp(): void
5964
}
6065

6166
/**
62-
* Data Provider for before send response
67+
* Data Provider for testAfterRenderResult
6368
*
6469
* @return array
6570
*/
66-
public function sendResponseDataProvider(): array
71+
public function renderResultDataProvider(): array
6772
{
6873
return [
6974
[
70-
"content" => "<body><h1>Test Title</h1>" .
71-
"<link rel=\"stylesheet\" href=\"css/critical.css\" />" .
72-
"<p>Test Content</p></body>",
75+
"content" => "<head><link rel=\"stylesheet\" href=\"css/async.css\">" .
76+
"<style>.critical-css{}</style>" .
77+
"</head>",
78+
"flag" => true,
79+
"result" => "<head><style>.critical-css{}</style>\n" .
80+
"<link " .
81+
"rel=\"stylesheet\" media=\"print\" onload=\"this.onload=null;this.media='all'\" " .
82+
"href=\"css/async.css\">\n" .
83+
"</head>",
84+
],
85+
[
86+
"content" => "<head><link rel=\"stylesheet\" href=\"css/async.css\">" .
87+
"<link rel=\"preload\" href=\"other-file.html\">" .
88+
"</head>",
7389
"flag" => true,
74-
"result" => "<body><h1>Test Title</h1>" .
75-
"<link rel=\"preload\" as=\"style\" media=\"all\"" .
76-
" onload=\"this.onload=null;this.rel='stylesheet'\" href=\"css/critical.css\" />" .
77-
"<p>Test Content</p>" .
78-
"<link rel=\"stylesheet\" href=\"css/critical.css\" />" .
79-
"\n</body>"
90+
"result" => "<head><link rel=\"preload\" href=\"other-file.html\">\n" .
91+
"<link " .
92+
"rel=\"stylesheet\" media=\"print\" onload=\"this.onload=null;this.media='all'\" " .
93+
"href=\"css/async.css\">\n" .
94+
"</head>",
8095
],
8196
[
82-
"content" => "<body><p>Test Content</p></body>",
97+
"content" => "<head><link rel=\"stylesheet\" href=\"css/async.css\">" .
98+
"<link rel=\"preload\" href=\"other-file.html\">" .
99+
"</head>",
83100
"flag" => false,
84-
"result" => "<body><p>Test Content</p></body>"
101+
"result" => "<head><link rel=\"stylesheet\" href=\"css/async.css\">" .
102+
"<link rel=\"preload\" href=\"other-file.html\">" .
103+
"</head>",
85104
],
86105
[
87-
"content" => "<body><p>Test Content</p></body>",
106+
"content" => "<head><link rel=\"stylesheet\" href=\"css/first.css\">" .
107+
"<link rel=\"stylesheet\" href=\"css/second.css\">" .
108+
"<style>.critical-css{}</style>" .
109+
"</head>",
88110
"flag" => true,
89-
"result" => "<body><p>Test Content</p></body>"
111+
"result" => "<head><style>.critical-css{}</style>\n" .
112+
"<link " .
113+
"rel=\"stylesheet\" media=\"print\" onload=\"this.onload=null;this.media='all'\" " .
114+
"href=\"css/first.css\">\n" .
115+
"<link " .
116+
"rel=\"stylesheet\" media=\"print\" onload=\"this.onload=null;this.media='all'\" " .
117+
"href=\"css/second.css\">\n" .
118+
"</head>",
119+
],
120+
[
121+
"content" => "<head><style>.critical-css{}</style></head>",
122+
"flag" => false,
123+
"result" => "<head><style>.critical-css{}</style></head>"
124+
],
125+
[
126+
"content" => "<head><style>.critical-css{}</style></head>",
127+
"flag" => true,
128+
"result" => "<head><style>.critical-css{}</style></head>"
90129
]
91130
];
92131
}
93132

94133
/**
95-
* Test beforeSendResponse
134+
* Test after render result response
96135
*
97136
* @param string $content
98137
* @param bool $isSetFlag
99138
* @param string $result
100139
* @return void
101-
* @dataProvider sendResponseDataProvider
140+
* @dataProvider renderResultDataProvider
102141
*/
103-
public function testBeforeSendResponse($content, $isSetFlag, $result): void
142+
public function testAfterRenderResult(string $content, bool $isSetFlag, string $result): void
104143
{
105-
$this->httpMock->expects($this->once())
106-
->method('getContent')
144+
// Given (context)
145+
$this->httpMock->method('getContent')
107146
->willReturn($content);
108147

109-
$this->scopeConfigMock->expects($this->once())
110-
->method('isSetFlag')
111-
->with(
112-
self::STUB_XML_PATH_USE_CSS_CRITICAL_PATH,
113-
ScopeInterface::SCOPE_STORE
114-
)
148+
$this->scopeConfigMock->method('isSetFlag')
149+
->with(self::STUB_XML_PATH_USE_CSS_CRITICAL_PATH, ScopeInterface::SCOPE_STORE)
115150
->willReturn($isSetFlag);
116151

152+
// Expects
117153
$this->httpMock->expects($this->any())
118154
->method('setContent')
119155
->with($result);
120156

121-
$this->plugin->beforeSendResponse($this->httpMock);
157+
// When
158+
$this->plugin->afterRenderResult($this->layoutMock, $this->layoutMock, $this->httpMock);
122159
}
123160

124161
/**
125-
* Test BeforeSendResponse if content is not a string
162+
* Data Provider for testAfterRenderResultIfGetContentIsNotAString()
126163
*
164+
* @return array
165+
*/
166+
public function ifGetContentIsNotAStringDataProvider(): array
167+
{
168+
return [
169+
[
170+
'content' => null
171+
]
172+
];
173+
}
174+
175+
/**
176+
* Test AfterRenderResult if content is not a string
177+
*
178+
* @param $content
127179
* @return void
180+
* @dataProvider ifGetContentIsNotAStringDataProvider
128181
*/
129-
public function testIfGetContentIsNotAString(): void
182+
public function testAfterRenderResultIfGetContentIsNotAString($content): void
130183
{
184+
$this->scopeConfigMock->method('isSetFlag')
185+
->with(self::STUB_XML_PATH_USE_CSS_CRITICAL_PATH, ScopeInterface::SCOPE_STORE)
186+
->willReturn(true);
187+
131188
$this->httpMock->expects($this->once())
132189
->method('getContent')
133-
->willReturn([]);
190+
->willReturn($content);
134191

135-
$this->scopeConfigMock->expects($this->any())
136-
->method('isSetFlag')
137-
->with(
138-
self::STUB_XML_PATH_USE_CSS_CRITICAL_PATH,
139-
ScopeInterface::SCOPE_STORE
140-
)
141-
->willReturn(false);
192+
$this->httpMock->expects($this->never())
193+
->method('setContent');
142194

143-
$this->plugin->beforeSendResponse($this->httpMock);
195+
$this->plugin->afterRenderResult($this->layoutMock, $this->layoutMock, $this->httpMock);
144196
}
145197
}

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,9 @@
2626
<type name="Magento\Framework\Controller\ResultInterface">
2727
<plugin name="result-messages" type="Magento\Theme\Controller\Result\MessagePlugin"/>
2828
</type>
29-
<type name="Magento\Framework\App\Response\Http">
30-
<plugin name="asyncCssLoad" type="Magento\Theme\Controller\Result\AsyncCssPlugin"/>
31-
</type>
3229
<type name="Magento\Framework\View\Result\Layout">
33-
<plugin name="deferJsToFooter" type="Magento\Theme\Controller\Result\JsFooterPlugin" sortOrder="-10"/>
30+
<plugin name="asyncCssLoad" type="Magento\Theme\Controller\Result\AsyncCssPlugin" />
31+
<plugin name="deferJsToFooter" type="Magento\Theme\Controller\Result\JsFooterPlugin" sortOrder="-10" />
3432
</type>
3533
<type name="Magento\Theme\Block\Html\Header\CriticalCss">
3634
<arguments>

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
<argument name="criticalCssViewModel" xsi:type="object">Magento\Theme\Block\Html\Header\CriticalCss</argument>
1919
</arguments>
2020
</block>
21+
<!-- Todo: Block css_rel_preload_script will be removed in next release as polyfill isn't used anymore -->
2122
<block name="css_rel_preload_script" ifconfig="dev/css/use_css_critical_path" template="Magento_Theme::js/css_rel_preload.phtml"/>
2223
</referenceBlock>
2324
<referenceContainer name="after.body.start">

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

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,17 @@
44
* See COPYING.txt for license details.
55
*/
66

7-
/** @var \Magento\Framework\View\Helper\SecureHtmlRenderer $secureRenderer */
7+
/**
8+
* @var \Magento\Framework\View\Element\Template $block
9+
* @var \Magento\Framework\Escaper $escaper
10+
* @var \Magento\Framework\View\Helper\SecureHtmlRenderer $secureRenderer
11+
*/
12+
813
?>
914
<div data-role="main-css-loader" class="loading-mask">
1015
<div class="loader">
11-
<img src="<?= $block->escapeUrl($block->getViewFileUrl('images/loader-1.gif')); ?>"
12-
alt="<?= $block->escapeHtml(__('Loading...')); ?>">
16+
<img src="<?= $escaper->escapeUrl($block->getViewFileUrl('images/loader-1.gif')); ?>"
17+
alt="<?= $escaper->escapeHtml(__('Loading...')); ?>">
1318
</div>
1419
<?= /* @noEscape */ $secureRenderer->renderStyleAsTag(
1520
"position: absolute;",

0 commit comments

Comments
 (0)