TryGhost / Ghost

Independent technology for modern publishing, memberships, subscriptions and newsletters.
https://ghost.org
MIT License
47.38k stars 10.34k forks source link

Feature: Blog Icon #7688

Closed ErisDS closed 7 years ago

ErisDS commented 7 years ago

In external sites, we want to be able to display an icon for each blog. This is effectively the same problem as in issue #4869 - we need a static URL at which an external service such as an RSS feed or ghost.org can access to fetch a representative URL.

This icon should be used as the favicon for the blog. It should also be used when any external service looks for an icon. Browsers always look for a /favicon.ico file, at the root of the current domain. This means the behaviour of this file will always be slightly different for blogs which use a subdirectory. When rendering HTML, browsers use

Ideally, we want to serve a .png file. I'm not 100% on what we can or can't do here, and Googling didn't turn up much. Most browsers support a /favicon.png , and it'll be fine to use this for browsers & ghost.org, but to ensure we also capture the RSS issue, it may be worth 301-redirecting favicon.ico -> favicon.png for blogs which have an icon uploaded. This is an unknown, and will require a little bit of experimenting with. Any solution which works in feedburner should be considered acceptable for this task.

Image Requirements

Solution

Part A: Getting the icon uploaded:

The icon will be a 3rd image on the general settings screen in Ghost. The main difficulty here is implementing the image validation, to ensure the image meets our requirements. At least for now, this can most easily be done server side, I believe.

Part B: Using the icon

Any blog that has an uploaded icon should use this instead of any theme favicons.

Note : the existing favicon implementation works as described in the asset helper documentation . It is odd & fiddly and needs overhauling. The existing implementation requires users to output their own meta tag, as Ghost 1.0 will need to output the meta tag itself, this will constitute a breaking change to the theme API.

Ghost will need an internal concept of what its "current favicon" is. This is possibly the 2nd MOST! tricky part of this whole implementation. In the case that a favicon has been uploaded, it's super easy, but otherwise, it requires some work to detect favicons in the active theme (and this must be refreshed when a theme is activated or overridden).

The ideal order of precedence for the favicon for a blog, is:

The {{ghost_head}} helper should be updated to output a new meta tag.

In the case that the "current favicon" is a .png file:

<link rel="shortcut icon" href="favicon.png" type="image/png" />

In the case that the "current favicon" is a .ico file:

<link rel="shortcut icon" href="favicon.ico" type="image/x-icon" />

Note : It would be good to experiment here with what will happen for themes that already have this output above {{ghost_head}} i.e. Casper.

Note: I don't know if the {{amp_head}} should output something different?

As well as outputting the meta tag with {{ghost_head}} , we should also make sure that the icon url (e.g. favicon.png) is available as {{@blog.icon}} .

Outputting the icon on a URI

In order for the icon to work for external sites, we need to render the icon whenever it is accessed via a URI, e.g. /favicon.png

The existing favicon implementation uses a very odd piece of logic to serve any requests for /favicon.ico using our shared file helper , which is totally wrong for this usecase - this code should basically be deleted and forgotten about, and a new piece of middleware for favicons should be written.

The new middleware should live in the exact same place in the stack (e.g. after the two themeHandler middlewares) and the logic should be as follows:

In terms of implementation, this middleware should be small and super fast, it's going to need to read the contents of the icon file, and then cache those contents.

Update: The existing favicon implementation assumes that it's ok to read from the file system, totally ignoring our storage adapter layer. The new solution needs a read method added to the storage adapters (see #7687) so that if a file has been uploaded, we are able to access it. If no file has been uploaded, then for now it is ok to read from the file system, as currently we only support local file system storage for themes & the shared files.

We then need to manage ensuring that the cache is cleared at the right times, so that if a user uploads a new logo, or deletes a logo, we correctly display the right one without requiring a restart of Ghost. Handling the caching is the 1st MOST! tricky part of this.

The rules are, to the best of my knowledge, as follows:

Useful links

Some places to use for inspiration for how to write the file reading and caching logic are:

JohnONolan commented 7 years ago

There was a discussion about this in slack ages ago, but just updating requirements for the record:

The ideal order of precedence for the favicon for a blog, is:

  1. Uploaded icon (png or ico)
  2. activetheme/assets/favicon.png
  3. activetheme/assets/favicon.ico
  4. activetheme/favicon.png
  5. activetheme/favicon.ico
  6. Ghost's favicon.png file (this is a new default, that needs to be added)
  7. Ghost's favicon.ico file (this is a new default, that needs to be added)
ErisDS commented 7 years ago

Ah good point - the theme ones were there because I thought this feature was going to land in LTS and I wanted to keep backwards compat 😁

Have added a note to the theme API changelog 👍

aileen commented 7 years ago

Left over TODOs for this issue:

ErisDS commented 7 years ago

I don't think this issue should be closed just yet? There are still a bunch of todos in the previous comment and I think we need to also add an item to update Casper ready for Ghost 1.0?

cc @AileenCGN I'm particularly interested in the outcome of the tests as I am not sure how much of a breaking change this is.

ErisDS commented 7 years ago

@JohnONolan (or maybe @AileenCGN already knows the answer to this): With the addition of this new feature, what should we serve as the favicon for the admin panel? Should we serve the default Ghost icon always, or should this now also use the custom one if available?

JohnONolan commented 7 years ago

Should use default Ghost icon always for admin

Also this issue should def not be closed till UX testing is complete. In fact any new features w/ UX testing needed probably best to assign to me for final pass design review rather than close?

ErisDS commented 7 years ago

@JohnONolan Roger on the icon. Also really like the idea of UX review 👍 cc @kirrg001 & @kevinansfield on that.

aileen commented 7 years ago

Yeah, had the UX feedback still open as a todo in the admin PR I made. But lost track of it, because it got merged already. Sorry about that. So, yes @JohnONolan: happy about feedback. You'll find it on master already and in general settings.

I also recommend to everyone to try it and test the browser and our cache behaviour. It worked fine for me and @kirrg001, but @kevinansfield had some caching issues in the browser.

Also, I never worked with RSS, so any testing/help/instructions are highly appreciated!

jloh commented 7 years ago

If the favicon for the admin interface is always going to be served as the Ghost icon (no issue with this btw) can it be served from the adminURL if this is set?

This would prevent the current issues on Ghost(Pro) where you visit your Ghost interface on ghost.io and your browser requests the favicon returning a 301 -> http://your.domain if you've got a custom domain setup. This causes the page be marked as having SSL errors in firefox.

ErisDS commented 7 years ago

@jloh yar very good point - I'm revisiting this in conjunction with #6922, #7958, #6864 and #7491 etc, trying to iron out the assets story across themes vs admin.

aileen commented 7 years ago

@JohnONolan: This is how the upload for the blog icon looks like atm:

image

Error, when trying to upload wrong file extension: image

Error, when file to small: image

Error, when too large: image

Finally, error when not square: image

JohnONolan commented 7 years ago

@AileenCGN

image

aileen commented 7 years ago

Update about what's left to do to finish up this feature and also working towards the clean split of admin and server, which includes the usage of the {{asset}} helper:

Ghost Admin

The favicon in Ghost Admin needs to be the default Ghost favicon


Currently we’re requesting the favicon in Admin in two different places:

  1. The favicon itself in general (which ends up in the browser tab as the symbol). We're currently loading the favicon from Ghost, but this can be a custom uploaded one.

    Admin Favicon should always be ghost, never custom

    It should definitely not change to a custom favicon. Safest and easiest way to do so, is to upload our default favicon in /assets/img for the Admin (see comments here as well). There is a similar image in the Admin asset folder already: /assets/img/ghosticon.jpg
 but reg. #8221 the usage of it will be removed. I suggest to replace it with Ghosts new favicon.png.

  2. The favicon which we’re requesting as a background image next to the user name here: image This, on the other hand could be either Ghosts default blog icon, or the custom uploaded one. The style (@JohnONolan) needs an overhaul, as it's always showing a black background, which doesn't not look appealing when a logo like this is uploaded: image

Testing and optimising all the things

There is definitely some weird caching behaviour (at least for me). Once an icon is uploaded, it refreshes the favicon in the browser tab, but it will only refresh the icon inside Admin only after requesting http://blog.com/favicon.ico and png and making sure that the right favicon gets rendered (sometimes needs a few refreshes) and then finally go back to Admin and refresh it. Voila. I guess this should be optimised, but I'm not sure if it's us or the browser.

We also need to test RSS feed behaviour (using ngrok) with our favicon URI as described here:

If the "current favicon" is a .png file, ensure it is served from /favicon.png . Also, 301 redirect any request for /favicon.ico to /favicon.png (this may need testing with RSS readers to ensure it works).

Ghost

One simple error message for blog icon upload

There should be a single error message for all cases of "Blog icon must be a square .ico or .png file between 32px –1,000px, under 100kb. "

Currently there are four different error messages implemented, which can be unified as one error that rules them all.

my.ghost.org

Usage of publication icon in my.ghost.org


That is the bigger part, which is left to do. To do this, we can refer to the URI of each blog (http://blog.ghost.io/favicon.ico), which should always return the favicon. Assuming this works fine, this feature shouldn’t really take long.


tl;dr TODOs:

Ghost-Admin

Ghost

my.ghost.org

aileen commented 7 years ago

Little update on the test with RSS reader:

I used ngrok to add my development blog as content. After reading through the RSS board specs, I added an image property (see #8289) to it, but it would still not load the initial image. I tried it as well with other - definitely accessible - images with the same result.

aileen commented 7 years ago

image

Feedly seems to load the favicon from Google cache server... Just a thought, but maybe it's not possible to test it properly in dev environment?

cc @ErisDS

JohnONolan commented 7 years ago

Yeah just do transparent bg in admin

aileen commented 7 years ago

Update on RSS reader behaviour:

Feedly uses the - what I found - deprecated but still working and - of course - not documented Google Service to get a converted (into png) version of a favicon from any website: http://www.google.com/s2/favicons?domain=website.com This service is afaik pretty quick, so it would even serve the favicon for my localhost, which I ngrok'ed. Changing the favicon didn't changed it in the Google Service, tho. This might be related to the cache headers, I guess.

Newsblur and Feeder worked perfectly, except changing favicons, but again... cache.

Digg reader seems to go straight for the root favicon.ico but still wouldn't fetch the right favicon.

ErisDS commented 7 years ago

There are still some issues with the implementation here that I can see:

First is the image size requirement inconsistency that I mentioned here: https://github.com/TryGhost/Ghost/pull/8260#issuecomment-292722049 I'm not sure if that's me getting confused between icon and logo, but something doesn't seem to add up!

Second, is that after uploading a custom favicon it gets output by ghost head like this:

<link rel="shortcut icon" href="/content/images/2017/04/4448260-2.png" type="png">

The expectation (outlined in the original issue) is this:

<link rel="shortcut icon" href="favicon.png" type="image/png" />

Third, this line will only ever say that an icon is type x-icon if the file is called favicon.ico, rather than for instance if I upload a file called myicon.ico.

Finally, there seems to be a couple of places where logic is being done to determine a) what the icon url should be and b) what type it is (as in the previous item), when these should really be packaged up into a util.

I also think the code in serve-favicon.js can be simplified & the tests improved.

aileen commented 7 years ago

@ErisDS I fixed the issue in serve-favicon and improved the output in ghost_head with #8298.

Finally, there seems to be a couple of places where logic is being done to determine a) what the icon url should be and b) what type it is (as in the previous item), when these should really be packaged up into a util.

I also think the code in serve-favicon.js can be simplified & the tests improved.

I will do these things in a follow up PR now.

aileen commented 7 years ago

This feature has only one subtask left: Implement publication icons in my.ghost.org.

I will open a separate issue on my.ghost.org for it, so we can close this issue.