Skip to content
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

chore: remove rel shortcut from favicon #2646

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+15 −3
Open

chore: remove rel shortcut from favicon #2646

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
5 changes: 5 additions & 0 deletions .changeset/lazy-rats-decide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@astrojs/starlight': patch
---

Remove 'shortcut' from favicon rel attribute
3 changes: 2 additions & 1 deletion packages/starlight/__tests__/basics/head.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,10 @@ describe('createHead', () => {
const defaultFavicon = {
tag: 'link',
attrs: {
rel: 'shortcut icon',
rel: 'icon',
href: '/favicon.svg',
type: 'image/svg+xml',
'data-favicon': 'default',
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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!

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@techfg techfg Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. It was attempting to ensure that the favicon defined in the Starlight configuration property was always last (regardless of its type - e.g., svg, png, ico, jpg, etc.) - this is what the default favicon portion of the comment may have been referring to along with potentially assuming it would be an SVG.
  2. It was attempting to ensure that the favicon defined in the Starlight configuration property if its an SVG was always last
  3. It was attempting to ensure that any SVG (regardless of whether it was the Starlight configuration property favicon or any other head configuration link) 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:

  1. Remove the check entirely and just leave at 80
  2. Detect any link that is for an svg and weight at 70
  3. Detect only the favicon config property if it's an svg and weight at 70
  4. Detect only the favicon config property regardless of type and weight at 70 (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;

  1. Since the head config 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.
  2. 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 favicon config property will always come last regardless of its type (would require the data-favicon='default' attribute)
  3. If the goal is to ensure SVG is last, then the question is whether or not this applies to:
    1. Any SVG - Option 2, understanding that it would detect link, rel='icon' and either href ends with .svg or type='image/svg+xml' (note that a custom head item may not have type set so some SVGs may be missed)
    2. Only favicon config property if SVG - Option 3 with checks for link, rel='icon' and type='image/svg+xml' and data-favicon='default'

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!

},
} as const;

Expand Down
3 changes: 2 additions & 1 deletion packages/starlight/components/Head.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,10 @@ const headDefaults: z.input<ReturnType<typeof HeadConfigSchema>> = [
{
tag: 'link',
attrs: {
rel: 'shortcut icon',
rel: 'icon',
href: fileWithBase(config.favicon.href),
type: config.favicon.type,
'data-favicon': 'default',
},
},
// OpenGraph Tags
Expand Down
7 changes: 6 additions & 1 deletion packages/starlight/utils/head.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,12 @@ function getImportance(entry: HeadConfig[number]) {
// 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.
if (entry.tag === 'link' && 'rel' in entry.attrs && entry.attrs.rel === 'shortcut icon') {
if (
entry.tag === 'link' &&
'rel' in entry.attrs &&
entry.attrs.rel === 'icon' &&
entry.attrs['data-favicon'] === 'default'
) {
return 70;
}
return 80;
Expand Down