dnnsoftware
[Bug]: CDF - Two Small Issues Rendering DnnXxIncludes -- v10.02.00 RC2
Is there an existing issue for this?
- [x] I have searched the existing issues
What happened?
See code below for details and to reproduce using Aperture default.aspx
Issue 1A: double forward slash after SkinPath
Issue 3A: multiple rel attributes are not supported, only the last one is used by the browser... given "preconnect" (or "preload"), don't add rel="stylesheet".
Steps to reproduce?
In a new deploy or DNN v10.02.00 RC2, Insert everything below into ~/Portals/_default/Skins/Aperture/default.aspx at line 4.
<%-- copy+paste this into Aperture default.ascx at line 4 to test --%>
<%-- Test 1
- Issue 1A: double forward slash after SkinPath
- fantastic that the unnecessary type="text/css" is removed
--%>
<dnn:DnnCssInclude
FilePath="public/YOUR_FILE_HERE"
PathNameAlias="SkinPath"
Priority="16"
runat="server"
/>
<%--
Results in DNN v10.02.00 RC2:
<link href="/Portals/_default/Skins/Aperture//public/YOUR_FILE_HERE?cdv=50" rel="stylesheet">
^^
Results in DNN v10.01.02:
<link href="/Portals/_default/skins/Aperture/public/YOUR_FILE_HERE?cdv=223" type="text/css" rel="stylesheet">
--%>
<%-- Test 2
- async and defer attributes render correctly, without =""
--%>
<dnn:DnnJsInclude
FilePath="~/Portals/0/notapath/ae44c967ae.js"
ForceProvider="DnnFormBottomProvider"
HtmlAttributesAsString="async defer crossorigin:anonymous"
Priority="99"
runat="server"
/>
<%--
Results in DNN v10.02.00 RC2:
<script src="/Portals/0/notapath/ae44c967ae.js?cdv=50" type="text/javascript" async defer crossorigin="anonymous"></script>
Results in DNN v10.01.02:
<script src="/Portals/0/notapath/ae44c967ae.js?cdv=50" type="text/javascript" async="" defer="" crossorigin="anonymous"></script>
--%>
<%-- 3. How close can we get to Google Fonts docs?
- Issue 3A: multiple rel attributes are not supported, only the last one is rendered... given "preconnect" (or "preload"), don't add "stylesheet"
- ampersands in href are okay converted or not
- in HTML5, end brackets with "/>" or ">" are both valid
--%>
<%-- Copied contents from Google for [font-name]
https://fonts.google.com/specimen/MuseoModerno
https://web.dev/learn/performance/optimize-web-fonts
--%>
<dnn:DnnCssInclude
FilePath="https://fonts.googleapis.com"
Priority="2"
ForceProvider="DnnPageHeaderProvider"
HtmlAttributesAsString="rel:preconnect"
runat="server"
/>
<dnn:DnnCssInclude
FilePath="https://fonts.gstatic.com"
Priority="2"
ForceProvider="DnnPageHeaderProvider"
HtmlAttributesAsString="crossorigin rel:preconnect"
runat="server"
/>
<dnn:DnnCssInclude
FilePath="https://fonts.googleapis.com/css2?family=MuseoModerno:ital,wght@0,100..900;1,100..900&display=swap"
Priority="2"
ForceProvider="DnnPageHeaderProvider"
runat="server"
/>
<%--
Expected/desired:
<link rel="preconnect" href="https://fonts.googleapis.com">
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin>
<link href="https://fonts.googleapis.com/css2?family=MuseoModerno:ital,wght@0,100..900;1,100..900&display=swap" rel="stylesheet">
Results in DNN v10.02.00 RC2:
<link href="https://fonts.googleapis.com" rel="stylesheet" rel="preconnect" />
<link href="https://fonts.gstatic.com" rel="stylesheet" crossorigin rel="preconnect" />
<link href="https://fonts.googleapis.com/css2?family=MuseoModerno:ital,wght@0,100..900;1,100..900&display=swap" rel="stylesheet" />
Results in DNN v10.01.02 (corrected):
<link href="https://fonts.googleapis.com?cdv=223" rel="preconnect" type="text/css"/>
<link href="https://fonts.gstatic.com?cdv=223" crossorigin rel="preconnect" type="text/css" rel="stylesheet"/>
<link href="https://fonts.googleapis.com/css2?family=MuseoModerno:ital,wght@0,100..900;1,100..900&display=swap&cdv=224" type="text/css" rel="stylesheet"/>
--%>
<%-- end of CDF tests, Aperture Skin below --%>
Current Behavior
Covered above, see output comparisons.
Expected Behavior
As described above.
Relevant log output
n/a
Anything else?
I plan to do more tests, main goal is to ensure compat with existing code. I plan to test the new Fluent stuff more soon-ish. :)
Affected Versions
10.02.00 RC2 or current development branch (unreleased)
What browsers are you seeing the problem on?
Chrome
Code of Conduct
- [x] I agree to follow this project's Code of Conduct
Thanks for the testing Jeremy. It looks to me like there are not any major new issues, do any of these feel like showstoppers to you?
I feel like the rel and type** attributes are very important. But if they fell to 10.02.01 I would be okay. Both can cause problems, but the scenarios are pretty rare. Would many developers debug and check to see if their esm was "actually" loading as a module or not? I believe the code works either way. Also, I believe the last listed attribute (rel or type) is the one the browser will use, so again doesn't feel critical or breaking.
** #6851
Yes, it would be nice to avoid duplicate attributes (I need to verify that there aren't any attributes that are valid to include multiple times), but it looks like that's not new in 10.2.0.
Is your final example (MuseoModerno) missing something? It's rendering as preload, but it doesn't look like there's anything special in the DnnCssInclude markup.
Sorry, let me correct that. Split it in to two tests at one point for consistency and then mixed the diff results. Check back in 30-60 mins.
UPDATE: corrected and noted. Also, there was nothing of note in the alternate version, so I am leaving it out. I originally was trying to get the (unset) crossorigin in there as well as the modern as="font" by adding HtmlAttributesAsString="crossorigin as:font,rel:preload" - which gives you this result in 10.02.00 RC2 (adding nothing new):
<link href="https://fonts.googleapis.com/css2?family=MuseoModerno:ital,wght@0,100..900;1,100..900&display=swap" rel="stylesheet" crossorigin as="font" rel="preload" />
The extra / one seems like that could be a big change and depending on server configuration could have an actual user impact
Just leaving this here because its so close to Aperture's result loading local fonts, though they do it entirely with code, (CDefault)page, .Header.Controls.Add(HtmlLink()) - see partials/_includes.ascx.
<%-- copy+paste this into Aperture default.ascx at line 4 to test --%>
<%-- Test 7 - Preload Local Font
- note: both issues already covered in 6850 - double slash and double rel attribute
Reference: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/link
--%>
<dnn:DnnCssInclude
FilePath="pathto/fonty-boldo.woff2"
PathNameAlias="SkinPath"
ForceProvider="DnnPageHeaderProvider"
HtmlAttributesAsString="rel:preload,as:font,type:font/woff2,crossorigin:anonymous"
Priority="7"
runat="server"
/>
<%--
Desired outcome in <head>:
<link href="/Portals/_default/skins/Aperture/pathto/fonty-boldo.woff2"
rel="preload" as="font" type="font/woff2" crossorigin="anonymous" />
Results in DNN v10.02.00 RC2:
<link href="/Portals/_default/Skins/Aperture//pathto/fonty-boldo.woff2?cdv=50"
rel="stylesheet" rel="preload" as="font" type="font/woff2" crossorigin="anonymous" />
Results in DNN v10.01.02:
<link href="/Portals/_default/skins/Aperture/pathto/fonty-boldo.woff2?cdv=50"
rel="preload" as="font" type="font/woff2" crossorigin="anonymous"/>
Aperture doing it better than DNN can:
<link rel="preload" as="font" href="/Portals/_default/Skins/Aperture/fonts/Ubuntu-Regular.woff" type="font/woff" crossorigin="anonymous" />
--%>
<%-- end of CDF tests, Aperture Skin below --%>
@bdukes and @donker... UPDATE: after reading responses, suggestion withdrawn.
I am wondering if we need an explicit way to specify that we should not add the "?cdv=nn". Using the local font example and knowing it would very likely NEVER change... should there be an option for force opting out? Maybe:
<dnn:DnnCssInclude
FilePath="pathto/fonty-boldo.woff2"
PathNameAlias="SkinPath"
ForceProvider="DnnPageHeaderProvider"
ForceNoCdv="true"
HtmlAttributesAsString="rel:preload,as:font,type:font/woff2,crossorigin:anonymous"
Priority="7"
runat="server"
/>
Someone else can come up with the better name for it. :)
This should give us (assuming fixes in 10.02.01):
<link href="/Portals/_default/Skins/Aperture/pathto/fonty-boldo.woff2" rel="preload" as="font" type="font/woff2" crossorigin="anonymous" />
Can you explain the use case? Why would someone specifically NOT want a cdn nr? It doesn't hurt to have the cdn nr there, right? And what if you decided to update that woff? Why add more code to opt out of the cdn nr? Especially since there is already a workaround: just include an html link like Aperture does it.
Yeah, I kind of agree here, the number does not hurt and helps CDNs know to re-fetch the file as it may have changed and they need to bust their cache
The url writing logic is concentrated in this method:
https://github.com/dnnsoftware/Dnn.Platform/blob/develop/DNN%20Platform/DotNetNuke.Web.Client.ResourceManager/Models/ResourceBase.cs#L82
So recapping that method:
- If you've added a CDN and CDN is enabled then that is output without modification
- If the link starts with "http" then no cdf nr is added (i.e. this is considered an external link)
- In all other cases a crm gets added
Where I see a potential bug is if the internal link already has a querystring. In that case the "?" should not be re-added but a "&" should be used. Other than that it looks legit. If there are issues of erroneous output it must be a bug in the layers above (i.e. the controls).
Now, to come to the above: is there a use case where a font served internally should NOT get a crm version attached? It runs counter to our current practice and I am unsure this creates some kind of error. The crm version is there so all clients are forced to update all resources from the local DNN installation. Why would we allow a developer to opt out of this?
Sounds good. And if a situation comes up where this is necessary, we can always do what David P did in Aperture (_includes.ascx). Thanks for the consideration.
I am glad Peter mentioned the pre-existing "?param=value" causing two "?" instead of one and an ampersand, I've run into that more than once.