Comment 1 Pierre Ossman 2023-07-06 17:13:18 CEST

Conveniently, Sphinx has a setting for this:

https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-html_favicon

So it looks like we just need to give it an appropriate file.

Comment 2 Pierre Ossman 2023-07-07 10:58:10 CEST

The HTML generated by Sphinx:

> <link rel="shortcut icon" href="_static/thinlinc.ico"/>

is slightly different from the HTML we have in Web Access and Web Admin:

> <link rel="icon" type="image/x-icon" href="/images/icons/thinlinc.ico">

Sphinx still has the legacy "shortcut" prefix. This has apparently been removed in Sphinx 6.1.0, but we are still on 4.2.0:

https://github.com/sphinx-doc/sphinx/commit/ede68fa423107fbab74d07b307d7dcb03c00dfbd

I doubt any browser will drop support for that prefix anytime soon, though.

There is also no "type" specified by Sphinx. But that field isn't mandatory, and MDN's example also lacks it:

https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/rel#icon

So it should hopefully be fine without. We'll need to test.

Comment 4 Pierre Ossman 2023-07-07 14:05:01 CEST

I tested the icon both when loaded via HTTP, and when loaded directly from files, in various browsers:

HTTP:

  Linux: Firefox, Chrome, Epiphany
  Windows: Edge, Firefox, Chrome
  macOS: Safari, Edge, Firefox, Chrome
  Android: Edge, Firefox, Chrome, Samsung Internet
  iOS: Safari

Files:

  Linux: Firefox, Chrome, Epiphany
  Windows: Edge, Firefox, Chrome
  macOS: Safari, Edge, Firefox, Chrome

It worked fine in all cases except two, and that is direct files in Safari on macOS, and direct files in Epiphany on Linux. Both of these are WebKit-based, so not entirely surprising they share this bug.

I tried many variations of the <link> tag, but I could not find anything that worked. I did not find many hits on the issue on Google, but there was one which claimed the same problem and also failed to find a way to solve it.

WebKit doesn't load the favicons using the normal network stack, which makes debugging very hard. So I don't see any way forward here. I've reported the issue at least:

https://bugs.webkit.org/show_bug.cgi?id=258969

I think we can live without the icon being shown in just one out of all of these cases. Hopefully, Safari addresses this issue in the future.

Comment 6 Pierre Ossman 2023-07-07 14:08:07 CEST

> MUST:
> 
>  * The ThinLinc icon is shown as the icon for the TAG in all common use cases

Everything except WebKit browsers loading the files locally. But it doesn't look like we can do much there.

> SHOULD:
> 
>  * There should be an explicit connection to the icon so that the icon is
>    robustly shown

The file is explicitly referenced in the same way as other resources (such as CSS and images), so it should be as robust as the rest of the content.

>  * The icon should use the same method and contents as for Web Access and Web
>    Admin as a lot of compatibility research was put into those

It uses the same icon file, so it should have the compatibility there. It doesn't use an identical <link> tag, but I don't think the differences are worth the large effort of getting them perfectly in sync.

Comment 7 Samuel Mannehed 2023-07-11 16:21:08 CEST

Looks good. I tested build 3316 installed on Fedora 38 for local HTML file access, and the same build on https://www.cendio.com/resources/docs/tag-devel/html/ from other devices.

I tested the following browsers:

 ✓ Fedora 38 (local file access "file:///opt/thinlinc/share/doc/thinlinc-server/html/")
   ✓ Chrome 114
   ✓ Firefox 115
   ✓ Edge 114
   ✗ Epiphany 44.3 (see previous comments)
 ✓ Windows 11 (cendio.com/resources/docs/tag-devel/html)
   ✓ Chrome 114
   ✓ Firefox 115
   ✓ Edge 114
 ✓ Android 13 (cendio.com/resources/docs/tag-devel/html)
   ✓ Chrome 114
   ✓ Firefox 115
   ✓ Edge 114
   ✓ Samsung Internet 21

I checked multiple different pages, and attempted to sidestep cache issues by using inkognito/private mode in the browsers.

Note that there doesn't seem to be any way to embed a favicon inside a PDF file. This means the PDF version will get the default favicon from the web server. In the case of cendio.com, this becomes the WordPress icon at the moment of writing.

Commits look good.

The acceptance criteria look sane.

> MUST:
> 
>  * The ThinLinc icon is shown as the icon for the TAG in all common use cases
Yep.

> SHOULD:
> 
>  * There should be an explicit connection to the icon so that the icon is robustly shown
> 
>  * The icon should use the same method and contents as for Web Access and Web Admin as a lot of compatibility research was put into those

Looks good enough, the reasoning in comment 6 makes sense.