CSS file is deferred when CSS element has defer attribute in default_head_blocks.xml. #37814

nurullah · 2023-07-28T15:44:12Z

Description (*)

While I searching for ways to defer non-critical CSS files, I found that magento2 has a defer attribute in the head.xsd file. This means I can write the following code in the default_head_blocks.xml without any errors.

<css src="css/intl-tel-input.css" defer="true" />

But I realized that this defer attribute has no effect on the CSS element. Because HTML link element doesn't have defer attribute. It only works with the <script> element. This pull request aims to provide defer feature for CSS files with defer non-critical render-blocking CSS hacky method.

Related Pull Requests

Fixes [View] Removed extra space in link and script tag #32919

Fixed Issues (if relevant)

Fixes What is the purpose of the defer attribute in the linkType elements (css, link, font)? #37657

Manual testing scenarios (*)

Create/Open default_head_blocks.xml file and add a CSS element like below;

<css src="css/intl-tel-input.css" defer="true" />

Flush cache (bin/magento cache:flush)
Open your Magento2 site in the browser and press ctrl + u combination.
See that the CSS file is defined as deferred like below.

<link rel="preload" type="text/css" as="style" onload="this.onload=null;this.rel=&#039;stylesheet&#039;" href="https://magento.test/static/version1690544175/frontend/ExampleVendor/base/tr_TR/css/intl-tel-input.css" />
<noscript><link rel="stylesheet" href="https://magento.test/static/version1690544175/frontend/ExampleVendor/base/tr_TR/css/intl-tel-input.css"></noscript>

Disable Javascript in site settings on the browser. Be sure that the css file loaded properly.

Questions or comments

I am not entirely sure that these changes are correct and I don't know if it will affect other code. I tested it on my browser and it works fine. Please review my code and let me know if I made any mistakes.

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
All automated tests passed successfully (all builds are green)

…ement in default_head_blocks.xml.

…sabled.

m2-assistant · 2023-07-28T15:44:15Z

Hi @nurullah. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.

Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests
Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

mrtuvn · 2023-07-29T02:16:24Z

Seem good fix inmy opinion. Can you able to add some automate test cover new changes

mrtuvn · 2023-07-30T01:53:11Z

lib/internal/Magento/Framework/View/Test/Unit/Page/Config/RendererTest.php
Unit test stay in here

hostep · 2023-08-03T20:42:23Z

lib/internal/Magento/Framework/View/Page/Config/Renderer.php

     * @return string
     */
    protected function getAssetTemplate($contentType, $attributes)
    {
+        $attributesString = '';
+        foreach ($attributes as $name => $value) {
+            $attributesString .= ' ' . $name . '="' . $this->escaper->escapeHtml($value) . '"';


Should be escapeHtmlAttr instead of escapeHtml I think?

And in the case of the onload one, it should be escapeJs inside a escapeHtmlAttr according to the docs:

Case: All JavaScript inside attributes must be escaped by escapeJs before escapeHtmlAttr:

The escapeHtmlAttr function escapes slash character and it looks like the below;

It's pretty useful for body element attributes but not for head element attributes. Also, before this pull request, the same escapeHtml function was used for the attribute value.

magento2/lib/internal/Magento/Framework/View/Page/Config/Renderer.php

Lines 332 to 334 in 0a834cc

foreach ($attributes as $name => $value) {

$attributesString .= ' ' . $name . '="' . $this->escaper->escapeHtml($value) . '"';

}

Thanks for the feedback! I know too little about this, would be great if somebody with a little bit of security knowledge could double check this here then.

@nathanjosiah: if you have a little bit of time, do you have some input here?

If the main issue is that escapeHtmlAttr escaped all chars then that is correct behavior even for head.

For example:

<html> <head> <style id="foo/bar"></style> </head> <body> Test succeeds if this is green <script>document.getElementById('foo/bar').innerHTML = 'body { color: green; }';</script> </body> </html>

The text would be green.

Here is a small demo that shows this off.

escapeHtmlAttr is appropriate for all attributes including script attributes. You would only need to combine escapeJs with escapeHtmlAttr if there is additional content being injected into the JS. For example from our devdocs

<div onclick="<?= $escaper->escapeHtmlAttr('handler("' . $escaper->escapeJs($aString) . '", ' . $escaper->escapeJs($aVar) .')') ?>"> My DIV </div>

As far as I can see, this PR is using the escaper methods correctly.

Looks like this PR is still waiting for your review @hostep. If there are no other problems, can you take this to the next step? Thanks.

I don't have a lot of expert knowledge on the subject but since somebody forced me to review this (I wasn't able to unassign myself), I approved it anyways.

Please be advised that because of too much escaping we end up with this in page source:

@sky-hub: and how is that a problem? A browser should be able to decode those without an issue. It's like writing > or < in the html source to display > or <. In this case / translates to / for example. It's redundant and will increase the payload size a bit sure, but it shouldn't break anything.

nurullah · 2023-08-04T18:03:33Z

Seem good fix inmy opinion. Can you able to add some automate test cover new changes

I added unit test for defer attribute.

@magento run Unit Tests

magento-automated-testing · 2023-08-04T18:03:39Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

nurullah · 2023-08-04T19:39:00Z

I opened the unit tests report but I didn't see any error. All 3 reports detail shows RendererTest.php passed the test. But unit tests failed surprisingly. So, I restart the unit test.

@magento run Unit Tests

nurullah · 2023-08-24T08:08:57Z

@magento run all tests

magento-automated-testing · 2023-08-24T08:09:09Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

nurullah · 2023-08-24T09:57:42Z

@magento run all Unit Tests, Functional Tests

magento-automated-testing · 2023-08-24T09:57:48Z

Failed to run the builds. Please try to re-run them later.

nurullah · 2023-08-24T09:58:23Z

@magento run all Unit Tests, Static Tests

magento-automated-testing · 2023-08-24T09:58:29Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

nurullah · 2023-08-24T10:40:18Z

@magento run all Unit Tests

magento-automated-testing · 2023-08-24T10:40:25Z

Failed to run the builds. Please try to re-run them later.

nurullah · 2023-08-24T10:41:02Z

@magento run all tests

magento-automated-testing · 2023-08-24T10:41:07Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

nurullah · 2023-08-27T07:54:18Z

@magento run Functional Tests CE, Functional Tests EE, Functional Tests B2B

magento-automated-testing · 2023-08-27T07:54:29Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

nurullah · 2024-01-26T12:11:02Z

@magento run all tests

magento-automated-testing · 2024-01-26T12:11:08Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

nurullah added 2 commits July 28, 2023 16:29

Added non-critical CSS attributes when defer parameter pass on css el…

597fd24

…ement in default_head_blocks.xml.

Deferred css file added in noscript tag to work when javascript is di…

7b65739

…sabled.

m2-community-project bot added the Progress: pending review label Jul 28, 2023

mrtuvn added Area: Perf/Frontend All tickets related with improving frontend performance. Area: Framework Priority: P2 A defect with this priority could have functionality issues which are not to expectations. labels Jul 29, 2023

mrtuvn added the Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests label Jul 29, 2023

type=preload mistake fixed.

c9a52f3

hostep reviewed Aug 3, 2023

View reviewed changes

m2-community-project bot assigned hostep Aug 3, 2023

m2-community-project bot added Progress: review and removed Progress: pending review labels Aug 3, 2023

hostep removed their assignment Aug 3, 2023

m2-community-project bot added Progress: pending review and removed Progress: review labels Aug 3, 2023

m2-community-project bot assigned hostep Aug 4, 2023

m2-community-project bot added Progress: review and removed Progress: pending review labels Aug 4, 2023

hostep removed their assignment Aug 4, 2023

m2-community-project bot added Progress: pending review and removed Progress: review labels Aug 4, 2023

nurullah added 3 commits August 4, 2023 18:02

Updated RendererTest inputs.

35a7260

Fixed double space problem between tagname and attributes.

ad4e4cf

Unit test added for defer attribute.

86ed4e6

hostep removed their assignment Aug 23, 2023

escapeHtmlAttr is used in getGroupAttributes value.

90c50ef

Merge branch '2.4-develop' into 2.4-develop

8f4ff6a

m2-community-project bot assigned hostep Sep 26, 2023

hostep removed their assignment Sep 26, 2023

hostep approved these changes Sep 27, 2023

View reviewed changes

m2-community-project bot assigned hostep Sep 27, 2023

m2-community-project bot added Progress: ready for testing and removed Progress: review labels Sep 27, 2023

hostep removed their assignment Sep 27, 2023

m2-community-project bot added Progress: review and removed Progress: ready for testing labels Sep 27, 2023

m2-community-project bot assigned sky-hub Dec 5, 2023

m2-community-project bot assigned hostep Dec 26, 2023

hostep removed their assignment Dec 26, 2023

Merge branch '2.4-develop' into 2.4-develop

3cb0c63

	foreach ($attributes as $name => $value) {
	$attributesString .= ' ' . $name . '="' . $this->escaper->escapeHtml($value) . '"';
	}

CSS file is deferred when CSS element has defer attribute in default_head_blocks.xml. #37814

Are you sure you want to change the base?

CSS file is deferred when CSS element has defer attribute in default_head_blocks.xml. #37814

Uh oh!

Conversation

nurullah commented Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

Uh oh!

m2-assistant bot commented Jul 28, 2023

Uh oh!

mrtuvn commented Jul 29, 2023

Uh oh!

mrtuvn commented Jul 30, 2023

Uh oh!

hostep Aug 3, 2023

Choose a reason for hiding this comment

Uh oh!

nurullah Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hostep Aug 4, 2023

Choose a reason for hiding this comment

Uh oh!

hostep Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

nathanjosiah Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanjosiah Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nurullah Sep 27, 2023

Choose a reason for hiding this comment

Uh oh!

hostep Sep 27, 2023

Choose a reason for hiding this comment

Uh oh!

sky-hub Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hostep Dec 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nurullah commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magento-automated-testing bot commented Aug 4, 2023

Uh oh!

nurullah commented Aug 4, 2023

Uh oh!

nurullah commented Aug 24, 2023

Uh oh!

magento-automated-testing bot commented Aug 24, 2023

Uh oh!

nurullah commented Aug 24, 2023

Uh oh!

magento-automated-testing bot commented Aug 24, 2023

Uh oh!

nurullah commented Aug 24, 2023

Uh oh!

magento-automated-testing bot commented Aug 24, 2023

Uh oh!

nurullah commented Aug 24, 2023

Uh oh!

magento-automated-testing bot commented Aug 24, 2023

Uh oh!

nurullah commented Aug 24, 2023

Uh oh!

nurullah commented Jul 28, 2023 •

edited

Loading

nurullah Aug 4, 2023 •

edited

Loading

nathanjosiah Aug 7, 2023 •

edited

Loading

nathanjosiah Sep 26, 2023 •

edited

Loading

sky-hub Dec 5, 2023 •

edited

Loading

hostep Dec 26, 2023 •

edited

Loading

nurullah commented Aug 4, 2023 •

edited

Loading