We should be able to use rel="preload" on a CSS asset.

dist/main.bundle.css: { attributes: { rel: preload, as: style } }

This doesn't work ; we can't override the default rel="stylesheet" because of the way the attributes are merged in Drupal\Core\Asset\CssCollectionRenderer

    // Defaults for LINK and STYLE elements.
    $link_element_defaults = [
      '#type' => 'html_tag',
      '#tag' => 'link',
      '#attributes' => [
        'rel' => 'stylesheet',
      ],
    ];

    foreach ($css_assets as $css_asset) {
      $element = $link_element_defaults;

      // .....

      // Merge any additional attributes.
      if (!empty($css_asset['attributes'])) {
        $element['#attributes'] += $css_asset['attributes'];
      }

Since $array1 + $array2 is not equal to $array2 + $array1 when those arrays have common keys (https://stackoverflow.com/a/13087814/3493566),
$element['#attributes'] += $css_asset['attributes']; should be
$element['#attributes'] = $css_asset['attributes'] + $element['#attributes']; or
$element['#attributes'] = NestedArray::mergeDeep($element['#attributes'], $css_asset['attributes']);

There's the exact same logic on Drupal\Core\Asset\JsCollectionRenderer but, since there's no default rel attribute, we already can use rel="preload" on JS assets.

CommentFileSizeAuthor
#14 3328221-14.patch817 bytesanas_maw
#11 3328221-11.patch658 bytesAjeet Tiwari

Issue fork drupal-3328221

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

MacSim created an issue. See original summary.

macsim’s picture

macsim commented
Issue summary: View changes
macsim’s picture

macsim commented
Title: Allow rel="preload" on CSS » Allow rel="preload" on assets
Issue summary: View changes
Related issues: +#2716115: Allow attributes passed with CSS in libraries (SRI)

Juanjol made their first commit to this issue’s fork.

Juanjol opened merge request !3267

juanjol’s picture

juanjol
Primary language Spanish
commented

I have created a merge request with a possible solution from the one proposed by MacSim, taking into account that we should not overwrite the 'stylesheet value of rel so that the styles continue working (the rel attribute allows multiple values separated by spaces). We still have to solve the CSS grouping, because with this solution we are applying the attributes to all groups, but not all the CSS had to have the modified attributes in the libraries.

juanjol’s picture

juanjol
Primary language Spanish
commented
Assigned: Unassigned » juanjol
socialnicheguru’s picture

socialnicheguru commented
Status: Active » Needs review

MR available for testing.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs tests

MR has a failure.

And the fact it is breaking something makes me think this will need it's own test coverage

b-prod’s picture

b-prod commented

The provided solution does not works well, it leads to merge the "stylesheet" and "preload" values, so we get:
rel="stylesheet preload"

Ajeet Tiwari’s picture

Ajeet Tiwari commented
Status: Needs work » Needs review
StatusFileSize
new 3328221-11.patch658 bytes

Kindly review the patch if it works fine.

cilefen’s picture

cilefen commented
Version: 9.4.x-dev » 11.x-dev
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Isuse was started in MR and should continue there please.

Also still needs tests.

anas_maw’s picture

anas_maw commented
StatusFileSize
new 3328221-14.patch817 bytes

Patch in 11 worked for me but it's missing the use statement, here is a new patch, I will update the MR also

juanjol’s picture

juanjol
Primary language Spanish
commented

Please propose changes in MR, don't provide more patches as MR is open.

nlisgo made their first commit to this issue’s fork.

nlisgo opened merge request !4509

nlisgo’s picture

nlisgo commented

I've created a test which expresses my understanding of the expected behaviour of CssCollectionRenderer.

I will look at the AssetResolver and CssCollectionGroup a little closer to understand what the expected behaviour of those should be.

nlisgo opened merge request !4510

nlisgo’s picture

nlisgo commented
Status: Needs work » Needs review
Issue tags: -Needs tests

The tests added cover my understanding of the expected behaviour. I have made the minimum changes to fix the failing tests.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Postponed (maintainer needs more info)
Related issues: +#3166831: Css aggregation should exclude assets with attributes from preprocessing

I think this might be a duplicate of #3166831: Css aggregation should exclude assets with attributes from preprocessing - there's no way to reliably merge different attributes for different css files into one aggregate, the only thing we can do is exclude those files from aggregation.

rikki_iki’s picture

rikki_iki commented

It's not a duplicate, I just don't think it's the right approach.

Preloading wouldn't just be for CSS assets, one also might preload a font file. I think it would be clearer if `preload` has it's own entry in the library system that was separate to css and js. Similarly a preconnect entry could be added at the same time.

Potential library definition;

font:
  css:
    base:
      https://fonts.example.com/font.css: { type: external }
  preconnect:
    https://fonts.example.com: {}
  preload:
    https://fonts.example.com/font-regular.woff2: { as: font, attributes: { type: font/woff2} }
    https://fonts.example.com/font-bold.woff2: { as: font, attributes: { type: font/woff2 } }

Then we don't need to worry about aggregation or deep merging arrays. It's very clear to anyone reading the library file that these are separate entries to the actual assets.

edit: simplified code example

catch’s picture

catch
he/him
Primary language English
commented
Status: Postponed (maintainer needs more info) » Closed (duplicate)

For fonts there is #3366561: Support preloading of fonts in the libraries API, but that's not allowing rel="preload"> on any arbitrary asset, it would be much closer to what you suggest in #22. I'm going to close this as a duplicate of that issue. Would be great if you could copy your comment over.

catch’s picture

catch
he/him
Primary language English
commented
Status: Closed (duplicate) » Needs work

After looking at both of these issues more, I don't think this is a duplicate after all, apologies for the diversion. However I do have questions:

Why is preload being added to a CSS file here? CSS is already linked from the header, so preloading won't load it any earlier than it is already.

Where there could be benefit to preload/preconnect is the following:

1. JavaScript added in the footer (when we know this ahead of time)

2. 103 early hint preloading of CSS and preconnect for external domains where we know assets will be loaded from them.

I think this is mostly going to end up blocked on #3565258: Support library-specific aggregates though unless it's only applied to files with preprocess: false.

quietone’s picture

quietone commented
Version: 11.x-dev » main
Assigned: juanjol » Unassigned

Un-assigning per Assigning ownership of a Drupal core issue.

Hi, Issues for Drupal core should be targeted to the 'main' branch, our primary development branch. Changes are made on the main branch first, and are then back ported as needed according to the Core change policies. The version the problem was discovered on should be stated in the issue summary Problem/Motivation section. Thanks.

macsim’s picture

macsim commented

To be honest I don't remember why I opened this issue 3 years ago.
I guess a web integrator on my team requested it for some reason and I wasn't able to fulfill their request.