WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 3 months ago

#16434 new feature request

Give site admin ability to upload favicon in Settings, General

Reported by: jane Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.1
Component: Administration Keywords: ux-feedback needs-patch
Focuses: Cc:

Description

WordPress has come a long way in terms of making it possible for someone completely non-technical to create a professional web site for a person or business. One of the little things that is still annoyingly technical is adding a favicon. WordPress.com does this via the Blavatar feature, a name I wouldn't really want us to adopt b/c this is more CMS-oriented, but the ease of uploading an image, scaling and cropping it, then having it become the favicon is something I do want to adopt. Keeping it a theme-based thing means the non-technical can't control it for their sites, which is lame.

Attachments (9)

trac-16434-01.patch (16.8 KB) - added by TomAuger 2 years ago.
Early patch with basic add image functionality
ico2_2.php (26.8 KB) - added by brandondove 2 years ago.
ImageICO PHP library
16434.diff (6.3 KB) - added by jorbin 2 years ago.
trac-16434-03.patch (21.0 KB) - added by TomAuger 2 years ago.
Update includes new API calls to generate the thumbnail IMG tag, verify existence of a custom favicon, and get the favicon file path. Refactoring to some functions from previous patch.
ico2_3.php (26.9 KB) - added by brandondove 2 years ago.
GPLv2 version of ImageICO php lib
trac-16434-04.patch (50.0 KB) - added by TomAuger 2 years ago.
Included ico2_3.php, "remove" functionality, and refactoring of get_favicon_thumbnail_img() to get_favicon_img()
16434.2.diff (21.1 KB) - added by jorbin 2 years ago.
remove ico library, clean things up a bit, add in nonces
ico2_3.2.php (29.8 KB) - added by Otto42 2 years ago.
Re-formatted version of the ico library which isn't so painful to read.
trac-16434-05.patch (56.0 KB) - added by tomauger 2 years ago.
Re-integrated ico library, large scale refactoring, same great taste.

Download all attachments as: .zip

Change History (145)

comment:1 jane3 years ago

Not specifically related, but possibly relevant: http://core.trac.wordpress.org/ticket/3426

comment:2 follow-up: Denis-de-Bernardy3 years ago

Plugin material?

comment:3 in reply to: ↑ 2 mikeschinkel3 years ago

Replying to Denis-de-Bernardy:

Plugin material?

+1

comment:4 mikeschinkel3 years ago

  • Cc mikeschinkel@… added

comment:5 GaryJ3 years ago

  • Cc GaryJ added

comment:6 follow-up: westi3 years ago

I'm not sure that this is core material but I can see that from a CMS point of view it is a missing feature.

As for the Plugin Material question - does a popular plugin exist for this?

comment:7 in reply to: ↑ 6 Denis-de-Bernardy3 years ago

Replying to westi:

As for the Plugin Material question - does a popular plugin exist for this?

Dunno about popularity, but users seem to have 85 options to choose from... :-)

http://wordpress.org/extend/plugins/search.php?q=favicon

comment:8 markjaquith3 years ago

Scaling images down to favicon size will have very mixed results. What if v1 just allowed them to specify a URL?

comment:9 DH-Shredder3 years ago

  • Cc mike.schroder@… added

comment:10 follow-up: devinreams3 years ago

  • Cc devin@… added

This seems more pertinent now that we have the "blavatar" appearing in the slimmed-down network bar (profile drop-down shows blogs you're a member of). Screenshot: http://cl.ly/0K2y0m423q133q092b0R

I admittedly have not looked very deeply but there is no way to set this AFAIK.

comment:11 in reply to: ↑ 10 jane3 years ago

  • Keywords 3.4-early added; 3.2-early removed

This is still a missing feature. Every time I see some small business owner super excited about how they now control their online identity with WP, but there's a generic globe in the browser tab instead of their own chosen image, I cringe. We should be better than this, and it shouldn't require a plugin.

comment:12 nacin3 years ago

add_theme_support( 'favicon' );

Core then provides a UI to upload/specify a favicon. Once specified, we generate the <link> tag automatically.

Theme support is desired basically to ensure that the theme avoids handling/printing an existing favicon. However I don't necessarily find this to be a requirement -- we likely can echo out a favicon without much side effect. Unsure if browsers rely on the last favicon specified.

(We could also even intercept /favicon.ico requests via .htaccess if we're installed at the root and rewrite it to the uploaded file, and thus specify a proper favicon.ico file in the <link>. As it is, though, core does provide for an early exit with a 404 via PHP.)

Ultimately, though, we're going to need a DB option. In particular, because my-sites and the admin bar will need to do get_blog_option() against each blog. Any theme support will not be detectable. Perhaps we could introduce get_blog_options() (or perhaps a deliberately more cryptic name) that attempts to fetch all of the options we need for a single site in a single query.

As an actual implementation, is favicon something for Settings, or for Appearance? Our cognitive approach here will affect our code implementation.

comment:13 DH-Shredder3 years ago

From a host perspective, we get so many hits/"checks" for favicon.ico and favicon.gif that we are actually placing empty files for them in each new domain's directory to lower potential load due to the 404s that would otherwise occur.

This just to say -- I'd be cautious with having PHP do any of the redirection for favicons due to the frequency in which they're hit both by bots and users.

comment:14 sabreuse2 years ago

  • Cc sabreuse@… added

comment:15 SergeyBiryukov2 years ago

  • Milestone changed from Future Release to 3.4

comment:16 jane2 years ago

@nacin: "Unsure if browsers rely on the last favicon specified." Seems so, based on experimenting with switching between Pagelines' Platform theme and Twenty Eleven.

When I go to WordCamps or meet business owners, most of them don't even know the word favicon, but they know they want that image to be theirs, not a g or a globe or a blue e. This fits squarely with the stated goal of 3.4 to make your site look the way you want. I don't really care if v1 is something as full-on as wordpress.com's blavatar feature or something as simple as providing a way to specify a URL, but using the uploader with some text about size requirement would be the best user experience.

comment:17 jane2 years ago

@nacin: "As an actual implementation, is favicon something for Settings, or for Appearance?"

Settings, General. Though it's appearance-related, from user perspective this is completely tied to the site name in the browser tab and should be edited in the same place.

comment:18 Mamaduka2 years ago

  • Cc georgemamadashvili@… added

comment:19 neoxx2 years ago

  • Cc neo@… added

comment:20 kencook2 years ago

I think @Jane is right - there will be users trying to upload 100k pixels and complaining about the results.

comment:21 cais2 years ago

  • Cc edward.caissie@… added

comment:22 chipbennett2 years ago

  • Cc chip@… added

+1 to having this in core.

It is not Plugin territory, and it is definitely not Theme territory.

+1 to add_theme_support( 'favicon' )

The WPTRT folks will be following this one closely, as its implementation will solve one (albeit minor) headache for us.

comment:23 follow-ups: Ipstenu2 years ago

Having it in core would be great for MultiSite users, too, and NOT in themes means a site on a network could keep their favicon while switching through themes.

comment:24 in reply to: ↑ 23 ; follow-up: jane2 years ago

Replying to Ipstenu:

Having it in core would be great for MultiSite users, too, and NOT in themes means a site on a network could keep their favicon while switching through themes.

Exactly. Changing your theme doesn't affect your site name, so why would it affect your logo? It's an image, but it's content, not presentation.

comment:25 in reply to: ↑ 24 chipbennett2 years ago

Replying to jane:

This is exactly why it is definitely not Theme territory. I would go even farther, to say that the favicon isn't just content, but rather identity.

Last edited 2 years ago by scribu (previous) (diff)

comment:26 beaulebens2 years ago

  • Cc beau@… added

comment:27 toscho2 years ago

  • Cc info@… added

comment:28 in reply to: ↑ 23 ; follow-up: RMarks2 years ago

  • Cc RMarks added

Replying to Ipstenu:

+1 Theme

My B2B company uses Multisite+Domain Mapping as a CMS. Each site represents one of our publications. We would want to have different favicons for each theme. Although, we know how to override it in HTML if needed.

Last edited 2 years ago by scribu (previous) (diff)

comment:29 in reply to: ↑ 28 Mamaduka2 years ago

Replying to RMarks:

Maybe we can have option for Multisite, to allow use different favicons or just use one general.

Last edited 2 years ago by scribu (previous) (diff)

comment:30 neoxx2 years ago

+1 for an individual favicon for each site in multi-site with fallback to default favicon as network-setting

comment:31 follow-up: Otto422 years ago

What does the world of browser support look like for favicons?

I know that we no longer have to be an ICO file, for example, and it doesn't need to be 16x16 anymore either. So what are the limitations that should be set on the resulting icon, for maximum compatibility across browsers?

comment:32 brandondove2 years ago

  • Cc brandon@… added

We built a plugin that allows a user to upload any image and convert it to favicons. (http://wordpress.org/extend/plugins/favicon-generator/). I've been retooling the interface because it's long overdue for an update, so maybe it makes sense to stop doing that and refocus our efforts on getting that functionality into core.

One complaint that we get constantly is that favicons don't update right away because of the way browsers cache them. Explaining to a non developer that they have to clear their cache doesn't generally go over very well.

comment:33 in reply to: ↑ 31 Mamaduka2 years ago

Replying to Otto42:

What does the world of browser support look like for favicons?

I know that we no longer have to be an ICO file, for example, and it doesn't need to be 16x16 anymore either. So what are the limitations that should be set on the resulting icon, for maximum compatibility across browsers?

http://en.wikipedia.org/wiki/Favicon#Browser_implementation

comment:34 Otto422 years ago

F'ing IE... (grumble)

Long story short, IE still only supports ICO files, and only 32x32 and 16x16 resolutions.

Every other browser can use PNGs, have alpha channel support, any resolution, etc.

Note: code to output ICO file format from PHP exists in this project: http://phpthumb.sourceforge.net/

Last edited 2 years ago by Otto42 (previous) (diff)

comment:35 TomAuger2 years ago

What do we feel about supporting other devices (iPhone / Android). Markup is slightly different, according to the Wiki article: http://en.wikipedia.org/wiki/Favicon#Device_support

comment:36 DrewAPicture2 years ago

  • Cc xoodrew@… added

Multisite

One big headache I've had some problems with were (non-multisite) sub-directory installs automatically inheriting the parent domain's favicon, so I can't even imagine the issues that might spawn from multiple subfolder installs. So as long as we had the ability to delineate between the sites and their favicons that would be cool.

Core

Multisite aside, inclusion in Settings > General would be great. +1

comment:37 scribu2 years ago

  • Keywords 3.4-early removed

comment:38 jorbin2 years ago

I think we can do this using a <link rel="SHORTCUT ICON" href="http://www.mydomain.com/myicon.ico"/> tag in the header which is supported by all of the brosers[1]. One thing we will need to consider is:

User uploads a .png . We convert it to a .ico file so it works in all browsers. Should we do some simple browser sniffing to serve the png if the user is in a browser that supports it and fall back to the ico for browsers that don't.

http://msdn.microsoft.com/en-us/library/ms537656(VS.85).aspx

comment:39 Otto422 years ago

If you're going to support IE and add ICO files for them, don't use browser sniffing.

You can use code like this:

<link href="http://example.com/favicon.ico" rel="shortcut icon" type="image/x-icon" />
<link href="http://example.com/favicon.png" rel="icon" type="image/png" />

This appears to work fine to me as long as the "shortcut icon" with the ICO file is first in the HTML.

Alternatively, the really correct way to do this is with conditional tags.

<!--[if IE]>
<link href="http://example.com/favicon.ico" rel="shortcut icon" type="image/x-icon" />
<![endif]-->

Non-IE browsers will then ignore the ICO file.

comment:40 chipbennett2 years ago

By the way, to whomever takes the lead on this one: I'm not on any of the other teams, and would be happy to help contribute code.

comment:41 follow-up: brandondove2 years ago

Is there a benefit to using png files for favicons instead of ico files other than not having to convert to a different file format?

comment:42 in reply to: ↑ 41 ; follow-up: Otto422 years ago

Replying to brandondove:

Is there a benefit to using png files for favicons instead of ico files other than not having to convert to a different file format?

  • Alpha channel support allows for transparency, which looks better in the browser because you can let the background color through.
  • You can use any size instead of being limited to 32x32 or 16x16.
  • PNGs are easily supported. Writing ICOs is kind of a pain.

PNGs are more standard. ICOs are weird. It's not hard to do both if this is done.

comment:43 in reply to: ↑ 42 ; follow-up: TomAuger2 years ago

Replying to Otto42:

  • Alpha channel support allows for transparency, which looks better in the browser because you can let the background color through.

Because we're letting the user upload any image format and doing the resize/crop, it's unlikely we'll want to support transparency. Unless we have some kind of PNG pass through option, whereby if they happen to upload a 24bit PNG, they get the option to save the raw (downscaled) PNG in addition to the ICO. That seems like an enhancement rather than a first release feature though.

  • You can use any size instead of being limited to 32x32 or 16x16.

I"m not sure I see how this works - after all, the browser chrome doesn't allow for a larger icon, at least not in any browser I'm familiar with. IE, Chrome, FF etc all put the favicon inside the address bar, and that ain't going to grow.

  • PNGs are easily supported. Writing ICOs is kind of a pain.

Yes, but with the lib you point to in an earlier comment this should be moot.

PNGs are more standard. ICOs are weird. It's not hard to do both if this is done.

We could certainly consider saving out both versions and the serving up the ICO based on conditional tags. I suppose the process should be:

  1. upload
  2. crop if necessary
  3. resize if necessasry
  4. save out to PNG with imageMagick
  5. convert to ICO

comment:44 in reply to: ↑ 43 DrewAPicture2 years ago

Replying to TomAuger:

  1. save out to PNG with imageMagick

Probably shouldn't just assume that everyone has imageMagick installed -- none of my servers do. Is this something that can be handled with existing Core functionality?

Last edited 2 years ago by DrewAPicture (previous) (diff)

comment:45 follow-up: brandondove2 years ago

We use a set of PHP functions to convert various image file formats to ico files. It would allow us to bundle the necessary functions in core. They use a combination of the GD lib and some bit swapping functions to handle the conversion. I'll attach the library so you can all check it out. I talked to the original author of the library and he was fine with us including it in our plugin on the repo.

comment:46 in reply to: ↑ 45 TomAuger2 years ago

Replying to brandondove:

I talked to the original author of the library and he was fine with us including it in our plugin on the repo.

I'm not 100% sure, but I believe the library would need to be GPL2 in order to be included in WP core.

comment:47 follow-up: Otto422 years ago

It needs to be GPL2 compatible to be in a plugin in the repository as well. Just the author's permission isn't good enough.

So.. you might want to double check on that one.

comment:48 in reply to: ↑ 47 brandondove2 years ago

Replying to Otto42:

It needs to be GPL2 compatible to be in a plugin in the repository as well. Just the author's permission isn't good enough.

So.. you might want to double check on that one.

From the header of the php library:

/*

  • License:
  • - you can freely use it
  • - you can freely distribute sourcecode
  • - you can freely modify it as long as you leave my copyright/author info in source code
  • - if you developing closesource application, you should add my name at least to "about" page of your web application
  • - if you create an amazing modification, please contact me... I can publish link to your webpage if you're interested...
  • - if you want to use my script in commercial application for earning money, you should make a donation to me first */

It doesn't specifically say GPL though. I'll see if I can track him down and get him to specify GPLv2 as the license type.

comment:49 follow-ups: nacin2 years ago

Before Otto's comment, I deleted ico2_1.php because its "license" is incompatible, or at the very least, too vague. brandondove, you should get him to re-release it as GPL, MIT, or some other GPL-compatible license to ensure that you can still use it in a plugin.

24	 * License:
25	 *      - you can freely use it
26	 *      - you can freely distribute sourcecode
27	 *      - you can freely modify it as long as you leave my copyright/author info in source code
28	 *      - if you developing closesource application, you should add my name at least to "about" page of your web application
29	 *      - if you create an amazing modification, please contact me... I can publish link to your webpage if you're interested...
30	 *      - if you want to use my script in commercial application for earning money, you should make a donation to me first

comment:50 TomAuger2 years ago

Status update - currently working: generating the proper HTML for the header based on an (as yet) textfield option that can be set under Settings > General. In my dev environment I still have to manually upload the PNG/ICO file to the media library. I don't think this is worth putting up a patch yet. Just thought I'd post an update.

Now for the fun part - the actual upload DB. I intend to replicate the UX of "set featured image". The challenge is that a lot of the thumbnail upload stuff is hard-coded specifically to tie it in to a post (references global $post_id, etc). This has been a pet peeve of mine for some time, happy to have an excuse to refactor that part of media.php.

comment:51 TomAuger2 years ago

  • Cc TomAuger added

comment:52 in reply to: ↑ 49 brandondove2 years ago

Replying to nacin:

Before Otto's comment, I deleted ico2_1.php because its "license" is incompatible, or at the very least, too vague. brandondove, you should get him to re-release it as GPL, MIT, or some other GPL-compatible license to ensure that you can still use it in a plugin.

Understood. I have extended an invitation for him to get involved, so we'll see what he says. I'll post an update when I have one. While we wait, is this kind of a code library something that we'd even consider using for this functionality, or are we truly looking to only support png files for newer browsers?

comment:53 TomAuger2 years ago

According to this link: http://phpthumb.sourceforge.net/demo/docs/phpthumb.license.txt PHPThumb is available under GNU2. It purports to being able to do an ICO export, and we probably don't need more than http://phpthumb.sourceforge.net/index.php?source=phpthumb.ico.php to make it work.

comment:54 TomAuger2 years ago

Working on a non-AJAX implementation first. The UI is trickier than it first appears. If we avoid the right-hand metabox that is used on the .com blavatar implementation, we get into a collision with the Settings > General form - that is, we need a multipart form to handle the file upload, but you can't nest forms.

Options:

  1. Use the metabox after all so its form can live in its own section of the HTML code
  2. Put the upload / favicon option above or below the other General options so it can have a different form
  3. Put it inside the General settings form, change that form to use multipart, and do the file processing within the code that handles the updating of the general options. I like this approach the least since it could really get messy, and it isn't immediately obvious to the user that clicking "upload" button would also save any other settings they may have accidentally messed with in the Options.

For the time being, putting it on the top of the form (option #2) seems like the easiest approach, but is that giving the favicon too much importance?

comment:55 nacin2 years ago

Put it at the very top for now. Once that works, we'll shoehorn it to work how we want it to end up.

comment:56 azizur2 years ago

  • Cc azizur added

comment:57 ericlewis2 years ago

  • Cc eric.andrew.lewis@… added

comment:59 in reply to: ↑ 58 nacin2 years ago

Replying to TomAuger:

Has https://wpcom.automattic.com/ticket/1294 been resolved?

Tom is referring to the avatar-upload code on WP.com, which he has and I will upload here. I wouldn't worry about that — the comment indicates that it is simply an image replication thing having to do with Gravatar, multiple servers, heavy caching, etc. I'd ignore it.

comment:60 TomAuger2 years ago

Note - if my (forthcoming) patch gets committed, it also includes the '.ico' MIME TYPE fix from this ticket: http://core.trac.wordpress.org/ticket/11824, so let's remember to circle back and mark it as complete when the time comes.

Last edited 2 years ago by TomAuger (previous) (diff)

TomAuger2 years ago

Early patch with basic add image functionality

comment:61 TomAuger2 years ago

  • Keywords has-patch needs-testing added

So here's what we have so far:

  • Rudimentary favicon image upload under Settings > General.
  • Crop and save out a .PNG and .ICO file (.ico file format is currently incorrect and does NOT work in IE)
  • Original upload AND both thumbs are stored as attachments
  • Hooks into wp_head and admin_head to add the x-browser HTML that (once the ICO format is fixed) works

Immediate TODOs before we could even consider releasing this:

  • clean up the Settings UI and display the uploaded thumb
  • Add the Remove feature
  • fix the ICO format or provide a workaround for IE

Later:

  • encapsulate in a class
  • some error trapping was skipped (marked in TODOs)
  • slicker upload UI (at least have javascript click the "upload" button once you've selected a file!
  • admin notice or some other method to "close the circle" and let the admin know the favicon was uploaded successfully.

comment:62 in reply to: ↑ 49 ; follow-up: brandondove2 years ago

Replying to nacin:

Before Otto's comment, I deleted ico2_1.php because its "license" is incompatible, or at the very least, too vague. brandondove, you should get him to re-release it as GPL, MIT, or some other GPL-compatible license to ensure that you can still use it in a plugin.

JPEXS re-released his ImageICO php library under the GPL. Details can be found here: http://www.jpexs.com/php.html. I'll also attach the updated library with corrected license.

brandondove2 years ago

ImageICO PHP library

comment:63 in reply to: ↑ 62 ; follow-up: nacin2 years ago

Replying to brandondove:

Replying to nacin:

Before Otto's comment, I deleted ico2_1.php because its "license" is incompatible, or at the very least, too vague. brandondove, you should get him to re-release it as GPL, MIT, or some other GPL-compatible license to ensure that you can still use it in a plugin.

JPEXS re-released his ImageICO php library under the GPL. Details can be found here: http://www.jpexs.com/php.html. I'll also attach the updated library with corrected license.

GPLv3 is incompatible with WordPress's license, GPLv2. Sorry I wasn't clear in my initial reply.

comment:64 in reply to: ↑ 63 ; follow-up: GaryJ2 years ago

Replying to nacin:

GPLv3 is incompatible with WordPress's license, GPLv2. Sorry I wasn't clear in my initial reply.

"The license under which the WordPress software is released is the GPLv2 (or later)..." => http://wordpress.org/about/license/

comment:65 dd322 years ago

"The license under which the WordPress software is released is the GPLv2 (or later)..." => http://wordpress.org/about/license/

Basically: That means, that any software integrated into the core WordPress package must be compatible with GPLv2 AND later, however, WordPress can be used in conjunction with other software under the GPLv2 OR later

comment:66 in reply to: ↑ 64 mikeschinkel2 years ago

Replying to GaryJ:

Replying to nacin:

GPLv3 is incompatible with WordPress's license, GPLv2. Sorry I wasn't clear in my initial reply.

"The license under which the WordPress software is released is the GPLv2 (or later)..." => http://wordpress.org/about/license/

Not for plugins, see: http://core.trac.wordpress.org/ticket/16898

jorbin2 years ago

comment:67 jorbin2 years ago

I did a little work building upon TomAuger's patch. We are generating some notices, so eliminating those is going to be my next step.

Note that the wp-facvicon.js file is just a copy of the .dev version so that you don't need to have SCRIPT_DEBUG set to true in order to use the patch. Once we have something committable, we'll need to generate the proper file.

UX wise:

I think we should consider is having the upload take place in a lightbox. The experience of uploading a file and having that jump you to a new page seems wrong to me.

comment:68 follow-up: TomAuger2 years ago

Excellent. What do you think about letting .bmp files through the uploader on the JS side (I've let it through on the PHP side). My thinking is that for those users that have legitimate favicons already generated (perhaps from their old, HTML site), they would probably want to just upload them and let them pass-through. That means possibly also supporting a .ico upload.

comment:69 follow-up: TomAuger2 years ago

Okay, what's the final answer on a .ICO converter library? Can we use JPEXS' library (GPL v3) or not, Licensing is not my area of expertise. Would like a final, canon answer on this one - the "GPL V2 or later" to me sounds like it is inclusive of GPL v3 and therefore should allow JPEXS' library.

comment:70 TomAuger2 years ago

To be TESTED: does the current PNG upload process preserve transparency (ie: allow for non-square icons)?

comment:71 TomAuger2 years ago

I'm not going to be working on this ticket today (Feb 21) but will pick up again tomorrow. Jorbin - do you have experience integrating the Lightbox UI? I will continue with the Remove functionality and general cleanup stuff.

comment:72 in reply to: ↑ 69 ; follow-up: Otto422 years ago

Replying to TomAuger:

Okay, what's the final answer on a .ICO converter library? Can we use JPEXS' library (GPL v3) or not, Licensing is not my area of expertise. Would like a final, canon answer on this one - the "GPL V2 or later" to me sounds like it is inclusive of GPL v3 and therefore should allow JPEXS' library.

Canonical answer: We can not use GPLv3 code in the core. All code in core must be GPLv2 compatible. We release WordPress under the "GPLv2 or later" license, and this is not inclusive. We cannot re-release somebody else's code under a different and incompatible license.

comment:73 in reply to: ↑ 72 brandondove2 years ago

Replying to Otto42:

Replying to TomAuger:

Okay, what's the final answer on a .ICO converter library? Can we use JPEXS' library (GPL v3) or not, Licensing is not my area of expertise. Would like a final, canon answer on this one - the "GPL V2 or later" to me sounds like it is inclusive of GPL v3 and therefore should allow JPEXS' library.

Canonical answer: We can not use GPLv3 code in the core. All code in core must be GPLv2 compatible. We release WordPress under the "GPLv2 or later" license, and this is not inclusive. We cannot re-release somebody else's code under a different and incompatible license.

I've asked jpexs if he's willing to re-release it (again) under GPLv2. My guess is it won't be a problem as his intent is to openly share the library.

comment:74 in reply to: ↑ 68 ; follow-up: jorbin2 years ago

  • Keywords ux-feedback added

Replying to TomAuger:

Excellent. What do you think about letting .bmp files through the uploader on the JS side (I've let it through on the PHP side). My thinking is that for those users that have legitimate favicons already generated (perhaps from their old, HTML site), they would probably want to just upload them and let them pass-through. That means possibly also supporting a .ico upload.

If the backend supports it, we can let it in. I more so wanted to add in some basic checking in order to not make users have to go back if they upload the wrong file. Feel free to edit the list in the JS or just let me know what you want.

I'm going to pick this up tomorrow. I have no problem adding in the lightbox experience once we have everything ironed out.

Basic UX flow that I'm planning -

1) User initiates the process by selecting a file.

2) If the file extension is supported, continue. Else bail and message why.

3) Lightbox appears wich contains the submitted image and allows the user to crop the image

4) Upon completion, the icon will apear next to the image upload form so that the user knows what they have selected.

TomAuger2 years ago

Update includes new API calls to generate the thumbnail IMG tag, verify existence of a custom favicon, and get the favicon file path. Refactoring to some functions from previous patch.

comment:75 in reply to: ↑ 74 TomAuger2 years ago

Replying to jorbin:

Basic UX flow that I'm planning -

1) User initiates the process by selecting a file.

2) If the file extension is supported, continue. Else bail and message why.

3) Lightbox appears wich contains the submitted image and allows the user to crop the image

4) Upon completion, the icon will apear next to the image upload form so that the user knows what they have selected.

Re: 4) Check my latest patch, esp. options-general.php for use of has_custom_favicon() and get_favicon_thumbnail_img()

Next I will be adding the Remove functionality. I suppose I should add a little JS for a confirmation "are you sure?" box.

comment:76 follow-up: scribu2 years ago

Nitpick: Why is that function called get_favicon_thumbnail_img()? Shouldn't it be just get_favicon_img()?

brandondove2 years ago

GPLv2 version of ImageICO php lib

comment:77 follow-up: brandondove2 years ago

Just uploaded version 2.3 of the ImageICO php library that has been re-released under the GPLv2 license. As always, you can check the details at jpexs website: http://www.jpexs.com/php.html.

comment:78 in reply to: ↑ 76 TomAuger2 years ago

Replying to scribu:

Nitpick: Why is that function called get_favicon_thumbnail_img()? Shouldn't it be just get_favicon_img()?

Yep. Will do.

comment:79 in reply to: ↑ 77 TomAuger2 years ago

Replying to brandondove:

Just uploaded version 2.3 of the ImageICO php library that has been re-released under the GPLv2 license. As always, you can check the details at jpexs website: http://www.jpexs.com/php.html.

This is great. Thanks for championing this. We need to make sure that if we do end up using this library, it's acknowledged in the credits along with the other libs that are used. Might be nice to do a blog post about the new favicon feature and credit the library author.

Before we go too far, I'll try rolling it in to the next patch and let's see if it does what we need it to do.

comment:80 follow-up: TomAuger2 years ago

ImageICO library works great as advertised. Only concern is that it uses fopen() and fwrite() to create the local file, and I wonder whether this could be a problem.

If this is a concern, then there are two strategies:

  1. edit/change the library to use more WP core methods
  2. capture the ICO on stdout and save it that way.

Any thoughts? I could use some guidance on the best practice here.

comment:81 in reply to: ↑ 80 ; follow-up: chipbennett2 years ago

Replying to TomAuger:

ImageICO library works great as advertised. Only concern is that it uses fopen() and fwrite() to create the local file, and I wonder whether this could be a problem.

If this is a concern, then there are two strategies:

  1. edit/change the library to use more WP core methods
  2. capture the ICO on stdout and save it that way.

Any thoughts? I could use some guidance on the best practice here.

+1 to integrating the WordPress Filesystem API into the library.

comment:82 in reply to: ↑ 81 ; follow-up: TomAuger2 years ago

Replying to chipbennett:

+1 to integrating the WordPress Filesystem API into the library.

So you don't think it would be a problem to refactor that library / ie: alter the source code? (it could use some rehabilitation IMHO)

I'm not that familiar with licensing, so my question is maybe more about that - is it okay to modify the source code of that library? What is the downside of doing that?

Last edited 2 years ago by TomAuger (previous) (diff)

comment:83 in reply to: ↑ 82 chipbennett2 years ago

Replying to TomAuger:

Replying to chipbennett:

+1 to integrating the WordPress Filesystem API into the library.

So you don't think it would be a problem to refactor that library / ie: alter the source code? (it could use some rehabilitation IMHO)

A problem in what sense: license, or implementation? If it's GPLv2, then license isn't an issue; we can modify it however we want. If it's ease of implementation, I don't know. Probably not something I have the chops for. Groking the Filesystem API is on my list of things to do, but it probably won't happen in time for the 3.4 release.

I just think that, 1) as a matter of principle of using core APIs were possible, and 2) as a matter of not introducing server configuration issues for end users, using the Filesystem API is the right approach.

comment:84 nacin2 years ago

The Filesystem API doesn't fit here. If we can't take an uploaded PNG and write out ICO, we use the PNG. This is the uploads directory — If we can upload the PNG, we can likely write the ICO to the same place. (The library *could* use an is_writeable() check, and/or some sort of error handling or suppression on the fopen(), but we can let it slide during testing.)

The Filesystem API is only designed for critical tasks. Updates, installs. Things like uploads are not critical and therefore avoid the filesystem API. It's also why permissions are going to be more lax on the uploads directory in many cases, because we need to be able to write files here in a normal process, rather than asking for your FTP credentials. (Not even sure how that'd work anyway.)

comment:85 nacin2 years ago

I'm not that familiar with licensing, so my question is maybe more about that - is it okay to modify the source code of that library? What is the downside of doing that?

From a licensing perspective, yes. From a maintenance perspective, we try not to. And when we do, we carefully consider why, and even then, we try to float the patch upstream.

So you don't think it would be a problem to refactor that library / ie: alter the source code? (it could use some rehabilitation IMHO)

This library has seen better days. I would not be comfortable including it in core in its current state, unless it receives an audit, at the very least.

Doing some research, it looks like ImageMagick supports creating ICO files. We should probably just use Imagick if it exists, and fallback to PNG-only if it does not. Here's some stuff on php.net on ICO to PNG, for example.

comment:86 nacin2 years ago

I think our efforts are best spent aimed at getting the PNG upload process polished and squared away. If we can generate ICO files at a later point, woo, but that is not necessary for version 1.

comment:87 TomAuger2 years ago

The only problem with only supporting PNG is that the entire Internet Explorer userbase would be alienated. This doesn't seem like the "WordPress Way" to me - rolling out a feature that just won't work for a popular browser.

Last edited 2 years ago by TomAuger (previous) (diff)

comment:88 follow-up: TomAuger2 years ago

Would we consider releasing the next version with a default WordPress favicon in place (possibly in the root directory) in .png and .ico format? A fallback for when you have not defined one yourself in Settings > General? This would also help with a lot of 404's that servers are getting.

comment:89 in reply to: ↑ 88 ; follow-up: nacin2 years ago

Replying to TomAuger:

Would we consider releasing the next version with a default WordPress favicon in place (possibly in the root directory) in .png and .ico format? A fallback for when you have not defined one yourself in Settings > General? This would also help with a lot of 404's that servers are getting.

No, a few reasons: One, it is difficult to then go ahead and remove that file (otherwise, some browsers will still read it). Two, there's really no reason for a default WordPress favicon on every WP.org site out there. Three, it only works for root-level sites. Four, we already intercept root-level /favicon.ico requests early and return a zero-length image/vnd.microsoft.com response, so 404s are not going to end up being logged if external rewrite rules are in place to map everything to index.php. Some hosts have simply placed favicon.ico files in root directories to even prevent WordPress from loading in these cases, but I don't think that's our responsibility beyond what we already do.

I agree that supporting ICO should be the goal. Let's just not catch ourselves up over it. Still need to build a working feature. :-)

TomAuger2 years ago

Included ico2_3.php, "remove" functionality, and refactoring of get_favicon_thumbnail_img() to get_favicon_img()

comment:90 TomAuger2 years ago

Okay, I'll need some guidance as to what to work on next, shooting for that 29th freeze. Not sure what jorbin is working on for the freeze, but I don't want to get into any duplication of effort. I'm feeling good about where we're at, functionality wise.

comment:91 in reply to: ↑ 89 TomAuger2 years ago

Replying to nacin:
You snuck yourself in there before my patch was up, so you'll see that the ico2_3.php is included (and works fine). At this point we have a working (if somewhat blunt) favicon solution. What's next?

comment:92 follow-up: azaozz2 years ago

Replying to nacin:

Doing some research, it looks like ImageMagick supports creating ICO files. We should probably just use Imagick if it exists, and fallback to PNG-only if it does not...

+1. Also not sure if there should be any support for image manipulation after uploading. The problem is that at such small sizes (16 x 16) most images don't look good. It doesn't matter if they are resized in PHP or by the browser on display.

So an "unaware" user will not be able to add a decent looking favicon by uploading an arbitrary image. Imho it would be much better to include some help text about favicons in general and the requirement for using .ico format image for best browser compatibility. There are many ways a user can generate decent looking .ico file, including online image editing websites that handle exactly that.

comment:93 in reply to: ↑ 92 ; follow-up: TomAuger2 years ago

Replying to azaozz:

The problem is that at such small sizes (16 x 16) most images don't look good. It doesn't matter if they are resized in PHP or by the browser on display.

Are you suggesting we only allow upload of 16 x 16 or 32 x 32 sized images, and don't allow for crop or resize?

I can see your arguments for this approach. However, all the images (photographic or otherwise) that I have used during the testing have turned out remarkably well. The most common things people will use would be their company logo, or a photo of themselves (ie: mugshot). Both of these would render quite decently with the antialias + interpolation settings that are currently in place in the resizing process.

I don't mind the idea of adding a "help" icon that would provide tips, or some contextual help on the General settings page that talks about good favicons, but it seems to me that it's out of scope for us to include training about how to make your site look good inline or onboard. Otherwise one could argue that we get into a slippery slope where we're now advising on content, so should we also provide tips on how to make a good masthead or background image, or write a good post title?

I believe that for the 80% we give them the ability to upload anything. Worst case scenario, they have a custom ugly favicon instead of the default ugly favicon; more likely they will have a mediocre custom favicon of their face, or their kid, or their logo. If they're smart, they'll take the text out of their logo and just use the icon.

And for the 20%, let's make sure that if we do get an icon in 16x16 or 32x32, or if it's already in ico format, it doesn't get interpolated or otherwise mutilated. So designers can do it themselves with their Photoshop ICO plugin, or some online service.

Last edited 2 years ago by TomAuger (previous) (diff)

comment:94 in reply to: ↑ 93 azaozz2 years ago

Replying to TomAuger:

Are you suggesting we only allow upload of 16 x 16 or 32 x 32 sized images, and don't allow for crop or resize?

Not exactly. Best probably would be to expect properly sized images. If the user uploads something above 32x32, it can be cropped and resized automatically and saved as .png (and .ico if possible). We need to allow uploading of favicons and properly include/link them, not make a tool for creating tham :)

I can see your arguments for this approach. However, all the images (photographic or otherwise) that I have used during the testing have turned out remarkably well...

Yes, many would turn well but many would fail too. It would be better if the user knows what the image looks like before uploading.

I don't mind the idea of adding a "help" icon that would provide tips, or some contextual help on the General settings page...

Yes, I meant a tab in the contextual help, probably with a short sentence under the "Upload" button that will send the user there. We won't need to add a lot of text, only 1-2 sentences explaining that favicons are very small images and should be generated by software or on particular websites.

I believe that for the 80% we give them the ability to upload anything.

Right, but also tell them what a favicon is and how they can make a good looking one.

And for the 20%, let's make sure that if we do get an icon in 16x16 or 32x32, or if it's already in ico format, it doesn't get interpolated or otherwise mutilated.

Agree. If an .ico image is uploaded or if the image is already 32x32 we shouldn't touch it at all (don't think we can touch .ico in GD anyways).

comment:95 ocean902 years ago

  • Cc ocean90 added

jorbin2 years ago

remove ico library, clean things up a bit, add in nonces

comment:96 follow-up: jorbin2 years ago

I took Nacin's advice and aimed for simplicity for v1.

my patch removes the ico2_3 library because reading it made me cry. My patch is based on Tom's and includes nonces and allows the full process ( image upload > image crop > image display by the theme) to take place error free. I think v1 is ready to go in.

This still will need a bit of a UI pass by someone with a better eye then I.

comment:97 follow-ups: ocean902 years ago

Just my 2 cents:

  • echo <<< ... do we really want this?
  • instead of FAVICON_SIZE I would prefer apply_filters( 'favicon_size', 32 )
  • Because the image will be uploaded to the media library, the image should be marked, see _media_states()
  • If I delete the favicon from the media library the options should get reset accordingly (like for theme mods _delete_attachment_theme_mod())
  • And should we delete the original image after cropping?
  • favicon-upload.php should set a $title
Last edited 2 years ago by ocean90 (previous) (diff)

comment:98 in reply to: ↑ 96 tomAuger2 years ago

Replying to jorbin:

I think v1 is ready to go in.

Right on Jorbin, this is dynamite.

This still will need a bit of a UI pass by someone with a better eye then I.

I'll get on the UI and will post an update in a few.

comment:99 in reply to: ↑ 97 tomAuger2 years ago

Replying to ocean90:

Just my 2 cents:

thanks for the awesome advice, see below...

  • echo <<< ... do we really want this?

What's wrong with the heredoc approach? Not that it's THAT many lines of code, but seems quite readable to me.

  • instead of FAVICON_SIZE I would prefer apply_filters( 'favicon_size', 32 )

Right you are. Consider it done.

  • Because the image will be uploaded to the media library, the image should be marked, see _media_states()
  • If I delete the favicon from the media library the options should get reset accordingly (like for theme mods _delete_attachment_theme_mod())

Both of these sound like best practices. I know nothing about either of these methods, so I will need to research a bit before committing to getting them into my next update.

  • And should we delete the original image after cropping?

I don't know the answer to this one. It depends on what the philosophy behind the Media Library is - do we keep a record of the full-size image of everything that goes into the library? Or is this a different case and the original scaled document is a throwaway - all we wanted was the smaller size?

  • favicon-upload.php should set a $title

Tru dat.

comment:100 nacin2 years ago

What's wrong with the heredoc approach? Not that it's THAT many lines of code, but seems quite readable to me.

We don't use heredoc or nowdoc anywhere in core. Not saying we shouldn't. We just haven't found a use for it.

comment:101 tomAuger2 years ago

Just some comments based on Jorbin's patch.

  1. Looks like when we upload a new image, it's not only creating the 32x32 icon PNG but also any number of additional thumbnail versions based on registered thumbnail sizes. This seems to follow standard practices for the Media Library, but I wonder whether this is appropriate here or not? Just a question really.
  1. Just to confirm - Jorbin, you dropped the idea of lightboxing the crop process? I didn't see any change to that workflow from my previous patch.

comment:102 in reply to: ↑ 97 ; follow-up: tomAuger2 years ago

Replying to ocean90:

  • favicon-upload.php should set a $title

The title is currently the attachment filename. What should the title be? I'm not sure how the title field of wp_posts should be used in this context.

comment:103 in reply to: ↑ 102 ; follow-up: ocean902 years ago

Replying to tomAuger:

Ah, sorry, I mean $title for <title></title>:

if (! current_user_can( 'manage_options' ) )
    wp_die( __( 'Cheatin&#8217; uh?' ) );

check_admin_referer( 'favicon_upload-options' );

$title = __( 'Crop Favicon' );
$parent_file = 'options-general.php';

if ( isset( $_POST['CROP_AND_SAVE'] ) ) {

What's wrong with the heredoc approach?

What Nacin said.

comment:104 in reply to: ↑ 103 tomAuger2 years ago

Replying to ocean90:

Replying to tomAuger:

Ah, sorry, I mean $title for <title></title>:

if (! current_user_can( 'manage_options' ) )
    wp_die( __( 'Cheatin&#8217; uh?' ) );

check_admin_referer( 'favicon_upload-options' );

$title = __( 'Crop Favicon' );
$parent_file = 'options-general.php';

if ( isset( $_POST['CROP_AND_SAVE'] ) ) {

Right on. Done.

What's wrong with the heredoc approach?

What Nacin said.

Hmmm. I find popping in and out of <?php ?> way more distracting than <<< but that's my Perl backgrouond speaking no doubt. Cool, we'll do it that way.

comment:105 follow-up: ocean902 years ago

I just saw, that you use the basename as the value for sitefavicon, can't we just use the attachment ID or better, the complete URL?

I asked because of this line

$posts = get_posts( array( 'name' => $favicon_fullname, 'post_type' => 'attachment' ) );

comment:106 follow-up: azaozz2 years ago

Few more things while testing 16434.2.diff:

  • When uploading a "favicon.ico" file, it's uploaded but is not set as favicon. Instead there's the error: "Please only use PNG (.png), JPEG (.jpg) or BMP (.bmp) image files for favicons." (this statement is wrong by itself).
  • When uploading an already resized 32x32 image it still goes to the crop screen, would probably be better to skip that step and set the image directly as favicon.
  • The UI on the General Settings screen should probably be after "E-mail Address" or at the bottom. Having it at the top doesn't "feel right".

Also using thickbox wouldn't be advisable. There are plans for scrapping thickbox and modal dialogs in general. Perhaps better would be to insert an iframe right under the UI on the General Settings screen.

Version 0, edited 2 years ago by azaozz (next)

comment:107 follow-up: ocean902 years ago

Note to my comment:105

A file named äöü.png throws out a notice and no favicon.

comment:108 in reply to: ↑ 105 TomAuger2 years ago

Replying to ocean90:

I just saw, that you use the basename as the value for sitefavicon, can't we just use the attachment ID or better, the complete URL?

Thanks for this feedback. Here's the rationale behind using the basename as the value: we need to accommodate both filename.png and filename.ico. When serving up the HTML we will (eventually) have to serve up some non-IE code referencing filename.png and then some IE-conditional code referencing filename.ico.

If we store the attachment ID or the URL, it would have to be one or the other (either the PNG or the ICO version of the file). Then, to serve up the other one, we would have to reverse-engineer the filename (strip out the TLA and substitute the correct one).

Not that that approach is all that difficult (in fact it's probably way easier to substitute the TLA at the end rather than stripping out the basename at the front, like I'm currently doing, but to me it doesn't feel "right".

Now, if there were some kind of parent attachment - perhaps a "favicon" post or something of that ilk - that both the ICO and the PNG attachment record could reference, then I would be comfortable storing that.

For now I don't feel there's a compelling reason to refactor the way it currently works, though I'm open to more opinions on the matter.

comment:109 in reply to: ↑ 106 ; follow-up: TomAuger2 years ago

Replying to azaozz:

Few more things while testing 16434.2.diff:

thanks for the testing!

  • When uploading a "favicon.ico" file, it's uploaded but is not set as favicon. Instead there's the error: "Please only use PNG (.png), JPEG (.jpg) or BMP (.bmp) image files for favicons." (this statement is wrong by itself).

Okay, I'll revise the front-end filter for now to only accept PNG and JPG, and make sure the error message reflects that.

  • When uploading an already resized 32x32 image it still goes to the crop screen, would probably be better to skip that step and set the image directly as favicon, converting it to .png if needed.

I'll perform a check on the uploaded size and if it's equal to or less than the default favicon size (as set by the 'favicon-size' filter), then skip the crop step.

  • The UI on the General Settings screen should probably be after "E-mail Address" or at the bottom. Having it at the top doesn't "feel right".

I'm happy to put it anywhere, however, the rationale for its position is twofold:

  1. Because the favicon appears near the site title in the browser tab, and because the topmost items under Settings > General are all related to the actual website itself (as opposed to the primary user, the timezone, etc), it seems to me that the favicon is most logically grouped there. Also, since it's one of the "flashier" bits of that particular page, there's a certain marketing appeal.
  1. More importantly, from a dev perspective, the rest of the page is wrapped in a FORM tag, and FORM tags can't be nested, so I'd either have to break the settings > general form tag, insert the favicon form, open a new form tag after it (which just seems crazy), or do something more sneaky like use an iFrame, which has its own issues. Now, if we do move forward with the iFrame approach for the crop section as you suggest (though that seems problematic to me, see below) then this could be a natural possibility.

I'll defer that decision until I get more feedback on the matter. Perhaps others can weigh in here.

Also using thickbox wouldn't be advisable. There are plans for scrapping thickbox and modal dialogs in general. Perhaps better would be to insert an iframe right under the UI on the General Settings screen.

The only issue I can see with this is that the crop UI is really (really) large at the moment, though arguably we could make the whole thing a bit smaller (though I wouldn't want to go less than 300x300 for the image size). So that's a pretty big chunk of real estate to just insert smack in the middle of the rest of the Settings > General form. Unless I'm not properly understanding your suggestion?

comment:110 in reply to: ↑ 107 TomAuger2 years ago

Replying to ocean90:

Note to my comment:105

A file named äöü.png throws out a notice and no favicon.

I'll take a look and see what that's all about.

Last edited 2 years ago by TomAuger (previous) (diff)

comment:111 in reply to: ↑ 109 ; follow-ups: azaozz2 years ago

Replying to TomAuger:

Okay, I'll revise the front-end filter for now to only accept PNG and JPG, and make sure the error message reflects that.

Perhaps it would be better to let the users add favicon.ico files (matching the entire file name) instead of banning uploads of .ico files.

In general letting the user upload an image, then crop it and set it as a favicon implies expectations that it would work. However that won't work in 30%-90% of the browsers (IE), depending on the region. Also if the users are in IE, the favicon won't show even for them.

Thinking the best UX here would be to include some help/explanation in the screen help tabs directing the users to upload ready-made favicon.ico files. Anything else would result in a lot of:

HELP! WP is broken, I uploaded image to show in the browser tab
but it's not working...

I'm happy to put it anywhere, however, the rationale for its position is twofold...
I'll defer that decision until I get more feedback on the matter.

Sounds good. Yes, the favicon is somewhat connected to the site name (identity) but is not "more important" than the name. Perhaps it would be better placed under the site name, tagline and address fields.

The only issue I can see with this is that the crop UI is really (really) large at the moment, though arguably we could make the whole thing a bit smaller (though I wouldn't want to go less than 300x300 for the image size). So that's a pretty big chunk of real estate to just insert smack in the middle of the rest of the Settings > General form. Unless I'm not properly understanding your suggestion?

Yeah, 300x300 or 350x350 seems large enough. The idea is to insert an iframe after uploading the image instead of using thickbox. The iframe would be pretty much the same as in TB but will be placed directly in the page rather than inserted at the bottom and shown with position:fixed in the middle or the screen. To make this work there can be a hidden iframe under the upload field with some basic JS that will $('iframe').slideDown('fast') when something is loaded there, and of course $('iframe').slideUp('fast') when not needed any more. That would make pretty sleek UI :)

Last edited 2 years ago by azaozz (previous) (diff)

comment:112 acsearles2 years ago

Just a question while you're trying to figure out all of the files that need to be created. Since we're already uploading an image at the 300X300 or 350X350 size couldn't we output a few more files to meet apple-touch-icon support? Since we're already here, wouldn't it be easy enough to add this now? For web pages that don’t specify a custom touch icon, a thumbnail screenshot of the page is used instead. Android has a default icon, and some systems fall back to the favicon if it’s available. We could even use a set of <link rel="apple-touch-icon" href="apple-touch-icon.png"> just like favicon. iOS also supports a no HTML option by putting the files in at the root.

There's a few resolutions that can be accommodated with only one file at 114X114. That's how apple does it. But it's recommended to create files with the following names and sizes.

apple-touch-icon.png
apple-touch-icon-precomposed.png
apple-touch-icon-114x114-precomposed.png
apple-touch-icon-72x72-precomposed.png
apple-touch-icon-57x57-precomposed.png

Here's an article explaining everything (http://mathiasbynens.be/notes/touch-icons) It's from a year ago but the information is still current. The device testing might not be.

comment:113 acsearles2 years ago

  • Cc acsearles added

comment:114 follow-up: toscho2 years ago

Aren’t proprietary image formats like the Apple icon or Opera’s Speed Dial icon plugin territory? Favicons are stable, anything else is a moving target. A hook for plugin authors to add more sizes and link elements should be good enough.

Last edited 2 years ago by toscho (previous) (diff)

comment:115 in reply to: ↑ 114 TomAuger2 years ago

Replying to toscho:

A hook for plugin authors to add more sizes and link elements should be good enough.

This sounds like a good approach, and viable for the current cycle.

comment:116 knutsp2 years ago

  • Cc knut@… added

comment:117 in reply to: ↑ 111 TomAuger2 years ago

Replying to azaozz:

In general letting the user upload an image, then crop it and set it as a favicon implies expectations that it would work. However that won't work in 30%-90% of the browsers (IE), depending on the region. Also if the users are in IE, the favicon won't show even for them.

The more I think about it, the more I feel strongly that releasing the favicon feature without IE / .ico support is just an out-and-out mistake. I think azaozz is right, support will be flooded, and the "build your own .ico and upload it" approach just seems too technical. If they can do that, why can't they just put the favicon.ico file in their site root and be done with it?

Doesn't make sense.

I think that releasing with some subset of ico2_3.php library, or possibly a homegrown version. All you need to do is play with encoding the bytestream, nothing too complicated or proprietary. Should be a matter of a couple hours to extract the relevant portions froom ico2_3, clean it up, add some WP error checking and best practices and roll it into favicon-upload.php.

At the rate this is all going (I mean getting concensus, not the actual development), it looks doubtful for 3.4, unless we can all agree on the exact release spec.

In the meanwhile, I'll work on cleaning up all the stuff that we DO have concensus on, and then see where we are in a day or two.

comment:118 in reply to: ↑ 111 TomAuger2 years ago

Replying to azaozz:

Yeah, 300x300 or 350x350 seems large enough. The idea is to insert an iframe after uploading the image instead of using thickbox. The iframe would be pretty much the same as in TB but will be placed directly in the page rather than inserted at the bottom and shown with position:fixed in the middle or the screen. To make this work there can be a hidden iframe under the upload field with some basic JS that will $('iframe').slideDown('fast') when something is loaded there, and of course $('iframe').slideUp('fast') when not needed any more.

Sounds good. But why create some custom JS UI components that would be unique to this feature, rather than leveraging Thickbox, that is used by the MediaCenter and other core features? I guess I just don't understand the need to roll our own here, when there's something perfectly workable.

Sorry, can you clarify that?

comment:119 follow-up: TomAuger2 years ago

Okay, scrolling up, I'm noticing some unanswered issues / questions needing decisions. In an effort to hopefully bring us closer to some decisions, let me just summarize them here and see if we can get some consensus.

  1. .ICO or not? If we don't support .ico at all, Explorer is out, so this doesn't seem like an option to me. So either we allow straight by-pass upload of an .ico file, or we figure out how to encode the .ico bytestream by either scavenging from sources we currently have or leveraging another library since GD doesn't support it natively.
  1. _media_states() not sure I understand what this is or how it's relevant. Looking for a little guidance on that one.
  1. Should we delete the original image after cropping and saving the 32x32 sized thumbs? Or do we leave the full-size attachment in the library? Do we also let it create all the other registered thumbnail sizes for the attachment?
  1. Thickbox or not for the crop UI?
  1. Location of the actual favicon field in Settings > General (see my comments on why it's currently on top). If we're not putting it on top (or bottom) then I'm looking for some creative suggestions to how to shim it into the page within the current form.
Last edited 2 years ago by TomAuger (previous) (diff)

comment:120 in reply to: ↑ 119 ; follow-up: jorbin2 years ago

Replying to TomAuger:

Okay, scrolling up, I'm noticing some unanswered issues / questions needing decisions. In an effort to hopefully bring us closer to some decisions, let me just summarize them here and see if we can get some consensus.

  1. .ICO or not? If we don't support .ico at all, Explorer is out, so this doesn't seem like an option to me. So either we allow straight by-pass upload of an .ico file, or we figure out how to encode the .ico bytestream by either scavenging from sources we currently have or leveraging another library since GD doesn't support it natively.

I think we need to. The library is a mess though and I think including it as-is is a bad idea. I would rather not include this in the release then include that library right now.

  1. _media_states() not sure I understand what this is or how it's relevant. Looking for a little guidance on that one.

We should include it in that function so that the image is properly marked as the favicon.

  1. Should we delete the original image after cropping and saving the 32x32 sized thumbs? Or do we leave the full-size attachment in the library? Do we also let it create all the other registered thumbnail sizes for the attachment?

My first thought is that we should treat it the same way as an image uploaded for the custom background and custom header. If we create the thumbnails for those, we should create the thumbnail for this.

  1. Thickbox or not for the crop UI?

Not Thickbox, but also having the user leave the page seems like bad UX to me. The more I think about it, the more I think an iframe inline is going to be the best approach.

  1. Location of the actual favicon field in Settings > General (see my comments on why it's currently on top). If we're not putting it on top (or bottom) then I'm looking for some creative suggestions to how to shim it into the page within the current form.

Not sure on this one.

Overall, I think we might be best developing this as a plugin that we can then bring in early in 3.5 rather then trying to force this into 3.4.

comment:121 in reply to: ↑ 120 tomauger2 years ago

Replying to jorbin:
Thanks for this input.

  1. .ICO or not?

I think we need to. The library is a mess though and I think including it as-is is a bad idea. I would rather not include this in the release then include that library right now.

So basically scavenge and build our own. I'm good with that.

  1. _media_states(). Looking for a little guidance on that one.

We should include it in that function so that the image is properly marked as the favicon.

Hmmm. Still looking for more info on this one.

  1. Should we delete the original image after cropping?

My first thought is that we should treat it the same way as an image uploaded for the custom background and custom header.

Okay, sounds good.

  1. Thickbox or not for the crop UI?

I think an iframe inline is going to be the best approach.

Okay.

  1. Location of the actual favicon field in Settings > General (see my comments on why it's currently on top). If we're not putting it on top (or bottom) then I'm looking for some creative suggestions to how to shim it into the page within the current form.

Not sure on this one.

I'ma keep it at the top for now, then.

Overall, I think we might be best developing this as a plugin that we can then bring in early in 3.5 rather then trying to force this into 3.4.

My feeling is that if we can't get the ICO library into this cycle, then we still release for 3.4, but only with a .ico upload passthrough option (no crop, only .ico files permitted). This will work across all browsers and will set the stage for something better in 3.5.

Otto422 years ago

Re-formatted version of the ico library which isn't so painful to read.

tomauger2 years ago

Re-integrated ico library, large scale refactoring, same great taste.

comment:122 tomauger2 years ago

Thanks Otto for cleaning up the ico2 library. It still needs a careful eye later, but I think it's eminently workable here.

My patch re-integrates this library (this time, within favicon-upload.php).

Please use DEBUG mode to leverage the JS and CSS changes (maybe this is obvious and doesn't need mentioning).

In this patch:

  • Uploading a PNG, JPG or GIF will function as usual. I have NOT implemented the iframe interface in this patch
  • Uploading an image that is <= 32px will result in a PASS THROUGH - the cropping UI will be bypassed and the image will be used as-is at that size, converted to PNG and ICO
  • Uploading an ICO file will pass through if it is <= 32px. Not sure what will happen if it's greater than 32px.
  • will display only the minimum required HTML (if only a PNG is present, will not generate the conditional IE stuff)

To DO:

  • iframe crop UI
  • _media_states

comment:123 eddiemoya2 years ago

  • Cc eddie.moya+wptrac@… added

comment:124 follow-up: jane2 years ago

  • Milestone changed from 3.4 to Future Release

We agreed that this wasn't quite ready for 3.4 at a recent dev chat before we released beta 1. Punting to clean up milestone.

comment:125 WraithKenny2 years ago

  • Cc Ken@… added

comment:126 in reply to: ↑ 124 bryceadams12320 months ago

Replying to jane:

We agreed that this wasn't quite ready for 3.4 at a recent dev chat before we released beta 1. Punting to clean up milestone.

Could this be planned for inclusion in 3.5?

comment:127 sennza19 months ago

  • Cc bronson@… added

comment:128 lkraav17 months ago

  • Cc leho@… added

comment:129 wdfee15 months ago

  • Cc info@… added

comment:130 WraithKenny14 months ago

In the mean time, https://github.com/chrisbliss18/php-ico was open-sourced (GPL2 or later).

comment:131 tomauger14 months ago

Coolness. I should probably go back into the code and see whether it's taking advantage of the 3.5 media updates, then see if this ever makes its way back to the right pair of eyes...

comment:132 ubernaut8 months ago

  • Cc ubernaut@… added

is this still alive? it's a great idea!

comment:133 follow-up: WraithKenny8 months ago

I suggested it as a potential plugin for 3.8, but no one really seemed excited to work on it...

comment:134 in reply to: ↑ 133 ; follow-up: brandondove8 months ago

Replying to WraithKenny:

I suggested it as a potential plugin for 3.8, but no one really seemed excited to work on it...

I'm certainly happy to work on this. It's been on my list of things to do for a while.

comment:135 in reply to: ↑ 134 paaljoachim7 months ago

Replying to brandondove:

Replying to WraithKenny:

I suggested it as a potential plugin for 3.8, but no one really seemed excited to work on it...

I'm certainly happy to work on this. It's been on my list of things to do for a while.

Awesome!

comment:136 celloexpressions3 months ago

  • Keywords needs-patch added; has-patch needs-testing removed

IE11 supports PNGs for favicons, so we no longer need to worry about creating ICOs. Because IE8 is approaching EOL (techinically WinXP is, but that's the only reason to still run IE8, and that happens a week before when WordPress 3.9 is planned for), and IE9 and 10 auto-update (albeit very slowly and not in all installations), I don't think we need to worry too much about older versions long-term. Even if this feature were launched today, it would work for well over 80% of users with only PNGs, and that number's going to keep going up (probably rapidly).

I don't think this project necessitates a feature plugin; it's pretty straightforward without the ICO part. Maybe an IRC meeting to nail down the implementation. I think we could take the following simplified approach for a long-overdue first-pass:

  • Upload button in Settings->general. No need for media library integration, since pictures and content-type images aren't generally appropriate.
  • On upload, resize and crop down to 32px square (via functions enhanced in 3.5). I see no reason for a crop UI (32px displayed as 16px is too tiny); if the user wants a different crop that can be done outside of WordPress, or we could add that in a future iteration.
  • If the user uploads an ICO file, use it directly.
  • Once a favicon has been uploaded, display it on Settings->General with an "upload new favicon" button and a remove link.
  • Bonus: duplicate this UI in the customizer, next to the site title & tagline options there.
  • Perhaps we should consider NOT saving the image as an attachment, and rather just saving its url in wp_options. Favicons are fundamentally different than everything else the media library is used for, and worst-case scenario the user re-uploads the image if they want to add it to a post, as a header image, etc.
  • Plugins and hooks: if we also save the location of the originally uploaded image, plugins can access that image and create additional sizes for proprietory OS-specific icons (for example, iOS icons and Windows 8 tiles). We can also add hooks in the upload process, so that custom sizes can be created and handled at the same time.

I think a big reason this still hasn't happened is that it became too complicated for a first-pass. Let's approach it bit by bit; we could even potentially get multiple iterations into the same release. I don't think this is nearly as complex of a feature as some of the patches on this ticket make it.

We probably need to look at a fresh approach, given the age of the existing discussion & patches.

Note: See TracTickets for help on using tickets.