Skip to content

Deferred script loading v2 #3696

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.

Sign up for GitHub

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

Closed
michaelletzgus wants to merge 1 commit into from
Closed

Deferred script loading v2 #3696

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 5 additions & 5 deletions core/templates/layout.base.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,15 @@
<?php foreach($_['printcssfiles'] as $cssfile): ?>
<link rel="stylesheet" href="<?php print_unescaped($cssfile); ?>" media="print">
<?php endforeach; ?>
<?php foreach ($_['jsfiles'] as $jsfile): ?>
Copy link
Member

@ChristophWurst ChristophWurst Mar 6, 2017

Choose a reason for hiding this comment

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

do we really have to move those lines up? This could potentially change/break the loading order with the scripts below (inline_ocjs)

Copy link
Contributor Author

@michaelletzgus michaelletzgus Mar 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Good point, it changed the order. inline_ocjs will be executed first (while parsing), the deferred scripts after parsing but before the DOMContentLoaded event comes up.

The standard states: "The defer attribute is only for external scripts (should only be used if the src attribute is present)."

I have found no code in the inline_ocjs which depends on other.
If the old order of execution (first inline, then extern) is desired the inline script code must become external. So why do we have internal code here at all? I think all these variables could be loaded form extern as well.

Copy link
Contributor Author

@michaelletzgus michaelletzgus Mar 6, 2017

Choose a reason for hiding this comment

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

@ChristophWurst
In fact this does NOT change the order of execution when using deferfor the external script, it only allows Firefox to download css and js in parallel.
With the inline_js block placed between css and js downloading of css and js happens synchronously. This seems to be a "feature" of at least Firefox...

Copy link
Member

@ChristophWurst ChristophWurst Mar 7, 2017

Choose a reason for hiding this comment

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

I see, thanks for the explanation :-)

<script defer nonce="<?php p(\OC::$server->getContentSecurityPolicyNonceManager()->getNonce()) ?>" src="<?php print_unescaped($jsfile); ?>"></script>
<?php endforeach; ?>
<?php print_unescaped($_['headers']); ?>
<?php if (isset($_['inline_ocjs'])): ?>
<script nonce="<?php p(\OC::$server->getContentSecurityPolicyNonceManager()->getNonce()) ?>" type="text/javascript">
<script nonce="<?php p(\OC::$server->getContentSecurityPolicyNonceManager()->getNonce()) ?>">
Copy link
Member

@nickvergessen nickvergessen Mar 6, 2017

Choose a reason for hiding this comment

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

is this change required?

Copy link
Contributor Author

@michaelletzgus michaelletzgus Mar 6, 2017

Choose a reason for hiding this comment

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

I don't know, for me both version work faster.
But: HTML5 does not require the type attribute. My intention was to prevent current or future browsers triggering any kind of legacy or compatibility mode which might interrupt the concurrent resource loading.

In short: The change is possibly not required for defeating the performance problem...

<?php print_unescaped($_['inline_ocjs']); ?>
</script>
<?php endif; ?>
<?php foreach ($_['jsfiles'] as $jsfile): ?>
<script nonce="<?php p(\OC::$server->getContentSecurityPolicyNonceManager()->getNonce()) ?>" src="<?php print_unescaped($jsfile); ?>"></script>
<?php endforeach; ?>
<?php print_unescaped($_['headers']); ?>
</head>
<body id="body-public">
<?php include('layout.noscript.warning.php'); ?>
Expand Down
10 changes: 5 additions & 5 deletions core/templates/layout.guest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,15 @@
<?php foreach($_['printcssfiles'] as $cssfile): ?>
<link rel="stylesheet" href="<?php print_unescaped($cssfile); ?>" media="print">
<?php endforeach; ?>
<?php foreach($_['jsfiles'] as $jsfile): ?>
Copy link
Member

@ChristophWurst ChristophWurst Mar 6, 2017

Choose a reason for hiding this comment

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

same problem/question here

<script defer nonce="<?php p(\OC::$server->getContentSecurityPolicyNonceManager()->getNonce()) ?>" src="<?php print_unescaped($jsfile); ?>"></script>
<?php endforeach; ?>
<?php print_unescaped($_['headers']); ?>
<?php if (isset($_['inline_ocjs'])): ?>
<script nonce="<?php p(\OC::$server->getContentSecurityPolicyNonceManager()->getNonce()) ?>" type="text/javascript">
<script nonce="<?php p(\OC::$server->getContentSecurityPolicyNonceManager()->getNonce()) ?>">
<?php print_unescaped($_['inline_ocjs']); ?>
</script>
<?php endif; ?>
<?php foreach($_['jsfiles'] as $jsfile): ?>
<script nonce="<?php p(\OC::$server->getContentSecurityPolicyNonceManager()->getNonce()) ?>" src="<?php print_unescaped($jsfile); ?>"></script>
<?php endforeach; ?>
<?php print_unescaped($_['headers']); ?>
</head>
<body id="<?php p($_['bodyid']);?>">
<?php include('layout.noscript.warning.php'); ?>
Expand Down
10 changes: 5 additions & 5 deletions core/templates/layout.user.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,15 @@
<?php foreach($_['printcssfiles'] as $cssfile): ?>
<link rel="stylesheet" href="<?php print_unescaped($cssfile); ?>" media="print">
<?php endforeach; ?>
<?php foreach($_['jsfiles'] as $jsfile): ?>
Copy link
Member

@ChristophWurst ChristophWurst Mar 6, 2017

Choose a reason for hiding this comment

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

same

<script defer nonce="<?php p(\OC::$server->getContentSecurityPolicyNonceManager()->getNonce()) ?>" src="<?php print_unescaped($jsfile); ?>"></script>
<?php endforeach; ?>
<?php print_unescaped($_['headers']); ?>
<?php if (isset($_['inline_ocjs'])): ?>
<script nonce="<?php p(\OC::$server->getContentSecurityPolicyNonceManager()->getNonce()) ?>" type="text/javascript">
<script nonce="<?php p(\OC::$server->getContentSecurityPolicyNonceManager()->getNonce()) ?>">
<?php print_unescaped($_['inline_ocjs']); ?>
</script>
<?php endif; ?>
<?php foreach($_['jsfiles'] as $jsfile): ?>
<script nonce="<?php p(\OC::$server->getContentSecurityPolicyNonceManager()->getNonce()) ?>" src="<?php print_unescaped($jsfile); ?>"></script>
<?php endforeach; ?>
<?php print_unescaped($_['headers']); ?>
</head>
<body id="<?php p($_['bodyid']);?>">
<?php include('layout.noscript.warning.php'); ?>
Expand Down
3 changes: 3 additions & 0 deletions lib/private/legacy/template.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,9 @@ public function fetchPage($additionalParams = null) {
$headers = '';
foreach(OC_Util::$headers as $header) {
$headers .= '<'.\OCP\Util::sanitizeHTML($header['tag']);
if ( strcasecmp($header['tag'], 'script') === 0 && in_array('src', array_map('strtolower', array_keys($header['attributes']))) ) {
$headers .= ' defer';
}
foreach($header['attributes'] as $name=>$value) {
$headers .= ' '.\OCP\Util::sanitizeHTML($name).'="'.\OCP\Util::sanitizeHTML($value).'"';
}
Expand Down