chore: remove rel shortcut from favicon
#2646
Conversation
🦋 Changeset detectedLatest commit: 662d775 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for astro-starlight ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| href: '/favicon.svg', | ||
| type: 'image/svg+xml', | ||
| 'data-favicon': 'default', |
There was a problem hiding this comment.
Thank you for the PR @techfg!
Could you explain why the data-favicon attribute was added in this PR? It’s not mentioned in the PR description and I don’t believe it’s a web standard.
There was a problem hiding this comment.
@delucis - The previous code detected shortcut icon as way to set the sort weight to 70 so I needed a way to maintain that behavior and adding a custom attribute seemed like the only way to detect the "starlight" favicon. I considered naming the attribute data-sl-favicon to clearly identify its a starlight custom attribute but in reviewing other custom attributes throughout the code base, it didn't appear that sl was applied so left it as data-favicon. Happy to modify this approach and/or name if you'd like, just need another option for maintaining the sort weight adjustment. Let me know, thanks!
There was a problem hiding this comment.
Ahhh, understood. OK.
Reading the comment in the head processing code, it says:
The default favicon should be below any extra icons that the user may have set because if several icons are equally appropriate, the last one is used and we want to use the SVG icon when supported.
The current heuristic of relying on shortcut icon honestly seems not a great idea (I’m looking at you past self 👀). I wonder if we can just short based on the extension? So instead of needing the extra data attribute, we make sure an SVG favicon is listed last?
There was a problem hiding this comment.
Agreed on the prior approach not being ideal (e.g., there could be multiple with shortcut icon to begin with), was going to mention but wasn't really relevant 😃.
The concern I'd have with detecting SVG directly is that there may not even be an SVG and even if there is, there may be multiple (e.g., config contains custom head attributes for different sizes).
I guess the question comes down to what the comment you referenced was really intending to do - I can think of three ways to interpret:
- It was attempting to ensure that the
favicondefined in the Starlight configuration property was always last (regardless of its type - e.g., svg, png, ico, jpg, etc.) - this is what thedefault faviconportion of the comment may have been referring to along with potentially assuming it would be an SVG. - It was attempting to ensure that the
favicondefined in the Starlight configuration property if its an SVG was always last - It was attempting to ensure that any SVG (regardless of whether it was the Starlight configuration property favicon or any other
headconfigurationlink) was last
It appears that head config was added to Starlight prior to favicon which provides some context to the comment, but it still is a little ambiguous in terms of what it was intending to do unless I'm just overthinking the comment 😄.
In doing some research on how user agents/browsers determine which browser to use, Wikipedia seemed to be provide some information for anything definitive. Unfortunately, every browser is a little different assuming the information there is accurate and at the end of the day, user agents/browsers are free to change how they choose a favicon in the future.
This article seems to be the de facto guide on favicons but there is no way at build/runtime for Starlight to know what was added to head configuration regarding favicon reliably (or at least without doing a lot of inspection of rel & type attributes) and it could really be anything with any number of meta tags.
All the above said, do you recall what you were ultimately intending to do with the favicon config property and sort weight? Depending on that, I can think of a few options:
- Remove the check entirely and just leave at
80 - Detect any
linkthat is for ansvgand weight at70 - Detect only the
faviconconfig property if it's ansvgand weight at70 - Detect only the
faviconconfig property regardless of type and weight at70(this is what the prior code kind of did and what the code in the PR does currently)
One other note is that there is no way to "disable" starlight from generating the favicon link (unless Head is overridden) since it seems merge would not detect them as the same (see stackblitz repro) so there is no way for a user to completely control sort order short of overriding Head. Possibly favicon config should support a value of false to disable it but that's a separate feature request and may not be necessary.
In meantime;
- Since the
headconfig doesn't provide a way to control sort order of meta tags I think minimizing what is sorted may be best so possibly option 1 should be followed especially given how user agents/browsers determine which to use is not part of a spec and varies. - If the goal is to control sorting of favicon at some level, I think option 4 may be best since it's basically saying Starlights
faviconconfig property will always come last regardless of its type (would require thedata-favicon='default'attribute) - If the goal is to ensure SVG is last, then the question is whether or not this applies to:
- Any SVG - Option 2, understanding that it would detect
link,rel='icon'and eitherhrefends with.svgortype='image/svg+xml'(note that a customheaditem may not havetypeset so some SVGs may be missed) - Only
faviconconfig property if SVG - Option 3 with checks forlink,rel='icon'andtype='image/svg+xml'anddata-favicon='default'
- Any SVG - Option 2, understanding that it would detect
Sorry for the long winded comment, this one isn't as straightforward as it may first appear. I think it all comes down to seeing if you can recall why you conditioned the sort weight originally and then deciding from there how to best adjust/handle 😄
Lemme know your thoughts, thanks!
@SnowDingo - Please see the reply I left to @delucis's question for an answer to this. |
I don’t see any reply, but maybe I’m looking in the wrong place? I see this: 
|
Description
rel="shortcut icon"- can just berel="icon"#2112shortcutfrom favicon. As discussed in favicon icons don't needrel="shortcut icon"- can just berel="icon"#2112, 'shortcut' only existed for IE <=8 which Starlight does not support. While agents should properly parse the rel value even with shortcut, it is non-conforming and should not be used (see MDN Docs).