Docfx: Wrong favicon.ico in _site

[richardschneider]

Title

Wrong favicon.ico in _site

Functional impact

The generated _site contains the wrong favicon.ico when _appFaviconPath is specified in docfx.json.

Minimal repro steps

1. Add your own favicon.ico to ./images
2. Add _appFaviconPath to docfx.json

Expected result

My ./images/favicon.ico should be copied to _site.

Actual result

The default docfx favicon.ico is copied to _site.

[superyyrrzz]

As a part of default template, favicon.ico is copied to _site because it's in the root of default template. As a resource file, ipfs-favicon.ico will not be changed to another output location unless file mapping is specified. _appFaviconPath doesn't take this logic.

[richardschneider]

@superyyrrzz Are you saying that to set a favicon I need to make two changes to docfx.json? Why have _appFaviconPath?

[superyyrrzz]

favicon.ico is part of the template, actually _appFaviconPath provides a quick way to set it through config without customizing template.

[richardschneider]

@superyyrrzz Thanks for all the explanations. Its nice to know how the product works.

But to a naive user, the website favicon should be the one specified in the docfx.json!!!

[superyyrrzz]

Thank you for your feedback, and we'll keep improving DocFX 😄

[richardschneider]

@superyyrrzz If you are improving DocFX, why have you closed this issue!

[emrgee]

I confirm this issue. After the icon specified in _appFaviconPath is copied, it gets overwritten by the default favicon.ico from the template. Please fix before closing this issue.

[superyyrrzz]

@emrgee It should work if you also add it in resource like:

"resource": [
  {
    "files": ["myfavicon.ico"]
  }
],
"globalMetadata": {
  "_appFaviconPath": "myfavicon.ico"
}

I think in your case, your favicon has the same filename as the one in template, so it's overwritten?

[emrgee]

Thanks for reopening the issue. The workaround you describe still pollutes the output directory with the default "favicon.ico" of the template, which it shouldn't after the fix.
I'd like to add that the same applies to _appLogoPath: the default "logo.svg" of the template should only be copied to the output if not overridden in the docfx.json.
The workaround I currently apply: export the default template, and in the three *.tmpl files, remove the lines
{{!include(favicon.ico)}} {{!include(logo.svg)}}, then build with the custom template: docfx build .\docfx.json -t .\template --serve

[superyyrrzz]

I think the gap lies in that users may expect _appLogo/FaviconPath to handle all the work to specify a logo/favicon, like copying to output, or overwriting existing one. In fact, _appLogo/FaviconPath just provides a quick way to change the logo/favicon's path in default template without customizing template. I agree this metadata may pollute the output, or confuse the user as part of the config, which I doesn't think of when adding it.

If you have some knowledge of DocFX's template system, customizing template is a cleaner way. You can put your favicon.ico in .\template, then use docfx build .\docfx.json -t .\template --serve. Custom template overwrites the defaulte one. From user's perspective, do you think this is clearer way than _appLogo/FaviconPath?

[emrgee]

The behavior you describe is not consistent with 3.2.2 Reserved Metadata:

_appLogoPath: Logo file's path from output root. Will show DocFX's logo if not specified. Remember to add file to resource.

_appFaviconPath: Favicon file's path from output root. Will show DocFX's favicon if not specified. Remember to add file to resource.

Actual behavior is more like "Will show DocFX's logo/favicon if not specified and given names different from logo.svg/favicon.ico". So I got confused when my logo setting was executed (because I happened to give it a different name), but there still was the file logo.svg in the output, and when the favicon.ico in the output was that of docfx and not the custom one that I had specified. After loosing time on investigating what went wrong, I was happy to find this issue by @richardschneider who seemed to have had the same confusion.

I vote for fixing the actual behavior so that it meets the description for the two parameters.

[superyyrrzz]

The description is intended to mean that it can specify a path from output root. If the file doesn't exist in the output, this metadata will not work. It doesn't affect which or where input should be copied.
If these two metadata's usage is confusing, what about remove these metadata, and add some other doc on how to replace logo/favicon? I think template customization option is not complicated.

[emrgee]

The template provides layout, navigation and style, the user of the template provides content, i.e. text and graphics, including logo and favicon. I would claim this to be common expectations for a templating system and should be possible without creating a custom template. This helps keeping the learning curve small. Also, when learning docfx and during development, it's nice to have defaults provided by the template. It's a bug if the default logo and favicon overwrite those provided by the user. It doesn't smell ok that the user must name its custom logo different from "logo.svg" and its favicon different from "favicon.ico" to not have it overwritten.

[superyyrrzz]

So the gap still lies in that user think this metadata is to specify logo/favicon, actually it just modifies part of the template. If I change the usage as you expect, it should

1. add file to resource. The question is where to copy? should add another metdata like _appLogoPathInOutput?
2. template should be aware of which file is logo and check the metadata to decide whether to overwrite the existing one.

I think if there's not a complete design of it, more expectation and more confusion will be introduced. I would still recommend the custom template option as a clear solution with minimal confusion.

[vicancy]

Hi @emrgee, why not put your 'favicon.ico' and 'logo.svg' inside your 'template' folder? It will automatically overwrites the default ones.

[icnocop]

@vicancy, I agree with @emrgee:

I would claim this to be common expectations for a templating system and should be possible without creating a custom template.

I didn't expect to have to modify the template in order to replace favicon.ico since it was already specified in _appFaviconPath and as a resource file.

@superyyrrzz, I'm thinking the best way to resolve this issue is to not overwrite files that may already exist, or at least have this as an option.

For example, if I add favicon.ico as a file resource as in the following docfx.json:

{
  "build": {
    "resource": [
      {
        "files": [
          "favicon.ico"
        ]
      }
    ],
    "globalMetadata": {
      "_appFaviconPath": "favicon.ico"
    },
    "dest": "_site",
    "template": [
      "default"
    ]
  }
}

I don't expect favicon.ico to get overwritten by the favionc.ico in the default template, especially without any warnings from docfx.exe:

Build succeeded.
        0 Warning(s)
        0 Error(s)

Thank you.

[superyyrrzz]

Thank you @icnocop for your comments. First, it makes sense to log a warning when overwrite occurs. DocFX has a check for overwriting among contents, but still doesn't check overwriting between contents and templates. We will improve it.

I didn't expect to have to modify the template in order to replace favicon.ico since it was already specified in _appFaviconPath and as a resource file.

If you customize the template, _appFaviconPath and resource is not needed.

@superyyrrzz, I'm thinking the best way to resolve this issue is to not overwrite files that may already exist, or at least have this as an option.

Personally I have some concern about this solution:

1. It's a breaking change to DocFX, from templates overwriting contents, to contents overwriting templates.
2. If DocFX changes to contents overwriting templates, is warning needed when overwrite occurs? It's not a good idea to overwrite silently. However, if this is the recommended way to custom favicon, log warning seems also not user friendly.

[smaillet]

@superyyrrzz re: your point 1 on a breaking change: That's actually a good thing in this case as the template overwriting user provided content is a violation of the principle of least surprise. Not t o mention basically defeats the whole idea of user provided content with fallback defaults in the template.

If the "content" inside a template is designed to be a default unless configured by the user then there's no need for warnings as the intent at the design and end user use is clear.

The current behavior is very surprising and frustrating as it doesn't conform to common expectations and isn't even self consistent with other configuration _app* settings, which just work.

[emrgee]

All users posting here so far claim that this is indeed just a simple bug. Trying to redefine what a templating system is and discussing this back and forth just appears as procrastination to me. We should trust the feedback from users that have worked with quite a few other templating systems. This bug is hanging around for 9 month without a fix from the dev team. Because we cannot vote on bug priority, I allowed myself to abuse the feature voting system and created an entry urging to fix this bug. Please upvote on https://feathub.com/docascode/docfx-feature-proposals.

[smaillet]

I don't see that everyone agrees it is a simple bug. I see people trying to explain why it is a problem and the docfx team arguing how users don't understand the "templating" system (e.g. effectively calling it user error or ignorance). I've used numerous doc generation tools from DocJet, DoxyGen, Innova, SandCastle etc... and all of them let you set things like your logo, company name copyright etc.. in a project configuration of some sort, and the generation system takes it from there to build the final docs that include your customizations. Users shouldn't need to care about the internal details of the templating system to effectively use a doc generator. Especially for cases like this, which are standard tweaks.

I'd upvote on the feathub but it requires granting permissions to my account on GitHub without any clear statements on use of information extracted from such a grant (bottom line I don't trust the site at all)

[superyyrrzz]

@smaillet Thank you for your comments and it's really convincing. It makes sense for content to overwrite template. I will fix it.
@emrgee After the fix, _appFaviconPath is not needed. Adding a resource is sufficient:

"resource": [
  {
    "files": ["favicon.ico"]
  }
]

[andreasblueher]

Hey @superyyrrzz what is the status on this change? I ran into the same issue like the other users before.

[superyyrrzz]

@andreasblueher Sorry for not following this issue for some time. I will handle this with high priority.

[superyyrrzz]

@andreasblueher After some test, I find this is already supported since #1036. Which version of DocFX are you using?

[superyyrrzz]

Close this as I believe it has been fixed.