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.

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

[NEXT-1332] App Router: Chromium browsers prefer favicon.ico over icon.svg #52002

Closed
1 task done
seidior opened this issue Jun 29, 2023 · 10 comments · Fixed by #52609 or #53343
Closed
1 task done

[NEXT-1332] App Router: Chromium browsers prefer favicon.ico over icon.svg #52002

seidior opened this issue Jun 29, 2023 · 10 comments · Fixed by #52609 or #53343
Assignees
Labels
bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team. locked Metadata Related to Next.js' Metadata API.

Comments

@seidior
Copy link

seidior commented Jun 29, 2023

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

Operating System:
      Platform: darwin
      Arch: x64
      Version: Darwin Kernel Version 22.6.0: Wed Jun  7 17:29:47 PDT 2023; root:xnu-8796.140.23.0.2~38/RELEASE_X86_64
    Binaries:
      Node: 20.3.1
      npm: 9.6.7
      Yarn: 3.5.1
      pnpm: 8.6.5
    Relevant Packages:
      next: 13.4.8-canary.11
      eslint-config-next: N/A
      react: 18.2.0
      react-dom: 18.2.0
      typescript: 5.1.6
    Next.js Config:
      output: standalone


warn  - Latest canary version not detected, detected: "13.4.8-canary.11", newest: "13.4.8-canary.9".
        Please try the latest canary version (`npm install next@canary`) to confirm the issue still exists before creating a new issue.
        Read more - https://nextjs.org/docs/messages/opening-an-issue

Which area(s) of Next.js are affected? (leave empty if unsure)

Metadata (metadata, generateMetadata, next/head, head.js)

Link to the code that reproduces this issue or a replay of the bug

https://github.com/seidior/next-favicon-issue

To Reproduce

Environment setup:

pnpm install
pnpm run dev

Then, open a new (Chromium-based) web browser window in private mode. Use the Web Inspector to disable your browser's cache via the "Network" tab. Then navigate to http://localhost:3000 in this window.

With the "Network" tab in Web Inspector open, observe that favicon.ico is retrieved, but icon.svg is not, even though both appear in the <head> of the page:

Describe the Bug

With Next.js 13.4 (stable or canary) set up to use App Router, when both /app/favicon.ico and /app/icon.svg are present, Chromium-based browsers (Google Chrome and Vivaldi were tested) do not use the SVG-based favorites icon and opt only to download the favorites icon in Windows Icon (.ico) format. This is following the metadata documentation.

In my attached test case, the following output is generated in the <head> section of the page:

<link rel="icon" href="/favicon.ico" type="image/x-icon" sizes="any"/>
<link rel="icon" href="/icon.svg?2e107c23de4929db" type="image/svg+xml" sizes="32x32"/>

However, this does not conform to best practices nor to the HTML spec.

  • The HTML specification suggests using sizes="any" for SVG icons.
  • Best practices suggest omitting the "sizes" attribute for SVG icons.

However, Next.js — as seen above — specifies the "sizes" attribute with the value chosen from the viewBox from the SVG image. (This issue presents no matter if the "height" and "width" attributes are specified for the SVG image.)

This causes an issue with Chromium-based browsers, which will not choose the SVG icon over the favorites icon if "sizes" is specified with pixel values for the SVG icon.

This issue appears regardless of whether or not standalone is specified and whether or not Next.js is running in dev mode or built as a standalone app.

Expected Behavior

When given an SVG-based favorites icon, Next.js should use one of the above methods (either sizes="any" or no "sizes" attribute) so these Chromium-based browsers will use the SVG icon regardless of any other icons specified. It should disregard any sizing information provided by the SVG.

Which browser are you using? (if relevant)

Google Chrome 114.0.5735.198 (Official Build) (x86_64)

How are you deploying your application? (if relevant)

dev environment / standalone

From SyncLinear.com | NEXT-1332

@seidior seidior added the bug Issue was opened via the bug report template. label Jun 29, 2023
@github-actions github-actions bot added the Metadata Related to Next.js' Metadata API. label Jun 29, 2023
@FrimJo
Copy link

FrimJo commented Jun 29, 2023

@seidior
I believe it is this code snipped which only checks for ico when deciding to sett attribute size to any: next-metadata-image-loader.ts:165

Maybe it should look for svg as well, or maybe icon.svg specific?

@huozhi huozhi added the linear: next Confirmed issue that is tracked by the Next.js team. label Jun 29, 2023
@huozhi huozhi self-assigned this Jun 29, 2023
@huozhi huozhi changed the title App Router: Chromium browsers prefer favicon.ico over icon.svg [NEXT-1332] App Router: Chromium browsers prefer favicon.ico over icon.svg Jun 29, 2023
@seidior
Copy link
Author

seidior commented Jun 29, 2023

@FrimJo Yeah, that's what I had narrowed it down to on my end as well, and I was going to add it as a PR directly after setting up a dev environment and making sure there were not any unintended effects.

My biggest worry with this is that there will be a side effect whenever an SVG file is incorporated on the page, since it's a reasonable assumption that an .ico file would not be used in an on-page <Image />, but not so with SVG. I just don't know enough about Next's codebase to make that guarantee.

On top of this, I'm not as clear with the culture of the repo to make a determination one way or the other on "sizes" here; I figured that would come out in the PR had I decided to include .svg alongside .ico to add sizes="any" or to have it return null instead just for SVG files.

I see that @huozhi self-assigned this in the meantime though; I'm happy to help wherever possible here.

@seidior
Copy link
Author

seidior commented Jun 29, 2023

Finally got my local dev environment working only to find out there's a Chromium bug relating to this issue.

The first relevant Chromium issue is #1162276, which is regarding the "sizes" attribute with .ico files. This issue is closed, with a commit fixing the issue landing in December 2022. That's not relevant for the issue at hand, however; that only explains why the conditional logic that's already there is there now.

The second relevant Chromium issue is #1450857, which relates to Chromium's internal scoring for favorites icons for figuring out which favicon to use. It's linked from that previous issue, and is currently marked as open.

The .ico that is in this test case does actually have three embedded sizes inside it, 48x48, 32x32, and 16x16. It's not clear to me why Chrome would ever request the .ico after having seen there's an .svg, but given that it does request it, there's no way to fix the code in Next.js and verify that it is working without the Chromium fixes landing first.

So:

  • Swapping out the .ico with one that only has one size embedded inside of it allows us to test and ensure that Chromium browsers use the .svg favorites icon.
  • I can follow up if/when a fix lands in Chromium for it requesting .ico after seeing .svg ever lands.

That leaves us the most minimal code changes that resolve the issue being:

    ...(extension in imageExtMimeTypeMap && {
      type: imageExtMimeTypeMap[extension as keyof typeof imageExtMimeTypeMap],
    }),
    ...(numericSizes
      ? { width: imageSize.width as number, height: imageSize.height as number }
      : {
          sizes:
            ['ico', 'svg'].includes(extension)
              ? 'any'
              : `${imageSize.width}x${imageSize.height}`,
        }),
  }

This produces the technically correct but not-to-best-practices code:

<link rel="icon" href="/favicon.ico" type="image/x-icon" sizes="any"/>
<link rel="icon" href="/icon.svg?2e107c23de4929db" type="image/svg+xml" sizes="any"/>

Which, again, does resolve the issue. Chromium still requests the .ico but doesn't rank it higher because the .svg doesn't have the pixel sizes specified in its markup.

However, if we wanted to get closer to the example best practices code, code that looks more like this:

    ...(extension in imageExtMimeTypeMap && {
      type:
        extension === 'ico'
          ? undefined
          : imageExtMimeTypeMap[extension as keyof typeof imageExtMimeTypeMap],
    }),
    ...(numericSizes
      ? { width: imageSize.width as number, height: imageSize.height as number }
      : {
          sizes:
            ['ico', 'svg'].includes(extension)
              ? extension === 'ico'
                ? 'any'
                : null
              : `${imageSize.width}x${imageSize.height}`,
        }),
  }

will produce output that looks more like the reference material:

<link rel="icon" href="/favicon.ico" sizes="any"/>
<link rel="icon" href="/icon.svg?2e107c23de4929db" type="image/svg+xml"/>

but that TypeScript code is awful, dirty, and hacky, and I'd want to refactor that before it landed in the codebase. I'm just including it here as a proof-of-concept, and I'll leave the implementation details to people more familiar with the codebase, code conventions, and styles.

@miukimiu
Copy link

miukimiu commented Jul 12, 2023

@seidior I run into the same issue and the following solution that is documented here worked well for me:

<link rel="icon" href="/favicon.ico" sizes="48x48" >
<link rel="icon" href="/favicon.svg" sizes="any" type="image/svg+xml">
<link rel="apple-touch-icon" href="/apple-touch-icon.png"/>
<link rel="manifest" href="/site.webmanifest" />

@seidior
Copy link
Author

seidior commented Jul 12, 2023

@miukimiu For sure! Thanks for sharing that. That page does actually link to the best practices document I linked in my bug report, and I've used guides like this in the past when making sites that aren't using Next.js.

However, the bug report is for creating an idiomatic solution that uses App Router and Next.js's built-in favorites icon support, as placing link tags inside the <head> is not supported or encouraged in App Router.

Thank you for sharing the resource though, and for sharing the example code! Hopefully anyone else who comes across this bug report can use that code on their own sites in the meantime. Although that may only work if your Windows Icon file (favicon.ico) has specifically a 48x48px image embedded inside, which some may not.

@philwolstenholme
Copy link
Contributor

EvilMartians have just updated their blog post: https://twitter.com/sitnikcode/status/1679164028541714432

In new version we replaced sizes="any" to "32x32" for ICO file. It prevents Chrome from downloading both ICO and SVG files.

@kodiakhq kodiakhq bot closed this as completed in #52609 Jul 13, 2023
kodiakhq bot pushed a commit that referenced this issue Jul 13, 2023
For `link` tag's `sizes` property, the property is defined as:

> This attribute defines the sizes of the icons for visual media contained in the resource. It must be present only if the [rel](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link#rel) contains a value of icon or a non-standard type such as Apple's apple-touch-icon. It may have the following values:
`any`, meaning that the icon can be scaled to any size as it is in a vector format, like image/svg+xml.

x-ref: https://html.spec.whatwg.org/#attr-link-sizes
x-ref: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link (`sizes` property definition section) 

Closes #52002
Closes NEXT-133
@huozhi
Copy link
Member

huozhi commented Jul 13, 2023

Updated the svg sizes to "any" as it matches the spec

EvilMartians have just updated their blog post: twitter.com/sitnikcode/status/1679164028541714432

In new version we replaced sizes="any" to "32x32" for ICO file. It prevents Chrome from downloading both ICO and SVG files.

I don't think we should change favicon.ico 's size as the problem is not related to favico.ico itself, more like handling svg with chrome

@Emilios1995
Copy link

Emilios1995 commented Jul 26, 2023

I don't think we should change favicon.ico 's size as the problem is not related to favico.ico itself, more like handling svg with chrome

But then how are users of the app router supposed to use SVG icons? Chrome is still preferring the .ico version.

@Emilios1995
Copy link

@huozhi Sorry about the ping, but just wanted to make sure you knew this is still broken, and can be fixed by adding 32 x 32 to the .ico file.

Can this be reopened, or should we open a new issue?

@huozhi huozhi reopened this Jul 29, 2023
ijjk added a commit that referenced this issue Jul 31, 2023
### What?

Change the `favicon.ico` metadata `sizes` property from always `"any"`
to using the actual size possible

### Why?

When chrome/firefox browsers search for icon it needs the proper
metadata to determine which one to use, giving `favicon.ico` sizes with
`"any"` will prevent it loading the `icon.svg` as default icon when
available.

Changing to set proper size of `favicon.ico` (as the `favicon.ico` sizes
could be 16x16, 32x32, etc.) fixes the issue.

It works (loading svg icon) for chrome and firefox:

Firefox
<img width="505" alt="image"
src="https://github.com/vercel/next.js/assets/4800338/6873e595-479d-4d9e-ae5c-39e5f938fcf5">

Chrome
<img width="460" alt="image"
src="https://github.com/vercel/next.js/assets/4800338/03bbe4c7-ef76-4f34-a611-337b0d4b97a3">

It loads favicon.ico for Safari:
Using the `test/e2e/app-dir/metadata` app it shows the favicon.ico for
Safari
<img width="1000" alt="image"
src="https://github.com/vercel/next.js/assets/4800338/92cc8714-ea5e-432d-8144-2a4a42ee4ce2">

Can't have it as an e2e test as I tested it always loads the
`favicon.ico` in headless mode which can't determine the behaviors

Fixes #52002

Co-authored-by: JJ Kasper <jj@jjsweb.site>
@github-actions
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue was opened via the bug report template. linear: next Confirmed issue that is tracked by the Next.js team. locked Metadata Related to Next.js' Metadata API.
Projects
None yet
6 participants