WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#26333 closed defect (bug) (fixed)

svg-painter.js and friends

Reported by: azaozz Owned by: azaozz
Milestone: 3.8 Priority: high
Severity: normal Version: 3.8
Component: Administration Keywords:
Focuses: Cc:

Description

Not convinced that this functionality is needed.

  • Not used in core.
  • Would work only if a plugin decides to add svg icon for use in the admin menu or the toolbar, and that icon is supplied as base64 encoded code in CSS. Then if the user changes admin color schemes, svg-painter would kick in and try to hack any svg icons to change their color so it matches the rest of the admin menu icons color.

A (much?) better option would be for the plugin to use icon font, the same way as used in core.

In addition, the way this is currently implemented is not so good:

  • svg-painter.js requires jQuery to run (would throw a fatal error otherwise) but jQiery is not listed as dependency.
  • It runs on $(document).ready() but is enqueued for the HTML head...
  • It is available to be enqueued on the front-end, but the needed wp_color_scheme JS global cannot be outputted there.

Also, looking at set_color_scheme_json() in wp-admin/misc.php: it erroneously outputs an empty array by default (isset should probably be ! empty). The name could probably be better, and the JS global should probably be wpColorScheme.

Attachments (6)

26333.patch (6.4 KB) - added by azaozz 7 years ago.
26333.2.patch (12.6 KB) - added by azaozz 7 years ago.
26333.diff (576 bytes) - added by nacin 7 years ago.
26333.2.diff (665 bytes) - added by nacin 7 years ago.
26333.3.diff (5.6 KB) - added by azaozz 7 years ago.
26333.4.diff (5.6 KB) - added by azaozz 7 years ago.

Download all attachments as: .zip

Change History (33)

#1 @kovshenin
7 years ago

  • Cc kovshenin added

#2 @melchoyce
7 years ago

Related discussion: http://make.wordpress.org/ui/2013/05/30/the-mp6-team-has-been-working-on-recommendations/

Summary: We thought that creating icon fonts might be too much overhead for some plugin developers, who could use SVG icons as a much simpler way to be MP6-compatible with their custom icons. Tillkruess put together this helpful guide for developers: https://github.com/tillkruess/MP6-Icon-Examples

If we think creating icon fonts is simple enough that we can write up documentation for plugin authors to do it themselves, then let's go in that direction.

Empireoflight created a great video tutorial for making Dashicons-style icons: https://www.youtube.com/watch?v=0HYAYZpSVbI&feature=share&list=UUmFVVQ7fcW2REzodxERiHHA Maybe we could see if he'd be interested in doing the same thing for creating an icon font?

A problem we might run into is finding a reasonable recommendation for developers to create their own fonts. We've been using Glyphs, which is paid software, to create the initial .otf for Dashicons (which we then run through fontsquirrel for a webfont). Services like IcoMoon will let you create a custom icon font for free. Hoever, I think when we tried using that, we ran into some issues with IcoMoon actually resizing the pixel grid Dashicons was built on. This led to some pixelated and distorted icons. We should give this a try again and see if it's still the case.

Last edited 7 years ago by melchoyce (previous) (diff)

#3 @melchoyce
7 years ago

Looks like IcoMoon now has an option (or maybe it's just easier to find) to adjust the pixel grid your font is built upon. The results look to be as clear as using Glyphs: https://dl.dropboxusercontent.com/u/7467403/dashicon-test/demo.html

#4 @Joen
7 years ago

  • Cc asmussen@… added

There's no doubt icon fonts, in the future, will become easy to create, especially with the appearance of IcoMoon and services like that.

As it stands, however, it's still a significantly higher bar to create an entire icon font compared to creating an SVG, which with js-painter in place will be colored according to the color scheme chosen by the user.

I firmly believe that if we leave the JS painter in place, we'll see third parties adopt the SVG method for creating icons, resulting in a much more visually color-coherent admin. If we take it out, people are going to stick to creating PNG icons.

#5 @nacin
7 years ago

I'm OK with svg-painter.js but I don't like how it forks an external library in the process. We should look into breaking that out into its own file. What was modified and why? Many questions need answering.

It should also become wp.svgPainter or something along those lines, if it is indeed a WP utility. However, it appears to be coded specifically to the admin menu. It should be moved to wp-admin in its current state. Or, it can become a dependency of common.js, or whatever.

As azaozz pointed out, jQuery should be a dependency of it, and it should be pushed to the footer.

Finally, wp_color_scheme should be added through script-loader and localize().

#6 @nacin
7 years ago

Finally, wp_color_scheme should be added through script-loader and localize().

Strike that. But it should probably have a better function and variable name, such as _wpColorScheme and wp_color_scheme_settings().

#7 @azaozz
7 years ago

To summarize:

  1. The best way to add a top level icon is to use one of the icons from Dashicons.
  2. Second best is to get/create an icon font and use it the same way as core.
  3. Third would be to create a 2 tone (black and white?) svg image, optimize it, base64 encode it, and add it in the css as described at https://github.com/tillkruess/MP6-Icon-Examples/blob/master/mp6-svg-icon.php. This method will also need a fallback for IE < 9, either a PNG or Dashicon.
  4. Not recommended: PNG image sprite (needs 4 or 6 icons, base, (highlighted?), selected plus 2x sizes).

The various web based services for creating icon fonts have improved during the last 6 months or so, and will continue to improve. Would it be necessary to add option 3 and support it for years, or would it be better to require a custom icon font (which may be harder to make but once done will be futureproof)?

@azaozz
7 years ago

#8 follow-up: @azaozz
7 years ago

In case we decide to keep svg-painter, in 26333.patch:

  • Clean up svg-painter.js and make it more error-proof.
  • Rename the PHP function and JS global.
  • Enqueue it better.

Still to consider: make the base64 encode/decode part of svg-painter.js local or separate it in another file. All modern browsers have window.atob() and window.btoa() already. However this is not used as part of the base64 script (not sure why), has to be used before calling it.

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

#9 @tillkruess
7 years ago

  • Cc me@… added

#10 @empireoflight
7 years ago

http://www.youtube.com/watch?v=5AfafGsuQTI
Making icons with AI and IcoMoo.io for plugin authors, part 1.

#11 @azaozz
7 years ago

Related: #26370.

#12 in reply to: ↑ 8 ; follow-up: @nacin
7 years ago

Replying to azaozz:

All modern browsers have window.atob() and window.btoa() already.

SVG is supported in IE9, while there were introduced in IE10. So I am guessing this chunk of code is here specifically for IE9.

tillkruess, any comments/thoughts?

#13 in reply to: ↑ 12 @azaozz
7 years ago

Replying to nacin:

Right, that code block is only for IE9, converting it to local, svg-painter only usage makes sense.

Another patch coming up soon, will include this change too.

@azaozz
7 years ago

#14 @azaozz
7 years ago

In 26333.2.patch:

  • Refactor svg-painter.js, make the base64 functions local to it.
  • Fix missing icon colors for the default theme.
  • Add some more error-proofing.

#15 follow-up: @tillkruess
7 years ago

I think it was FireFox were the native window.atob() and window.btoa() failed, because of some encoding issues, that's why we implemented the local functions.

azaozz suggestions to first using Dashicons or a custom icon font is what we had in mind, the SVG CSS background image is just a cheap, simple fallback.

#16 @azaozz
7 years ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

In 26601:

Svg-painter:

  • Clean up the JS, better names, etc.
  • Convert the base64 encode/decode code from jQuery plugin to local use.
  • Add missing icon colors for the default theme.
  • Make it more error-proof.

Fixes #26333.

#17 in reply to: ↑ 15 ; follow-up: @azaozz
7 years ago

Replying to tillkruess:

Do you remember what was the exact problem with FF? There was a small error in the base64 encoded xml matching regex. After fixing it seems FF works properly with all tests.

#18 @azaozz
7 years ago

In 26603:

Svg-painter: fix couple of jshint warnings, see #26333.

#19 in reply to: ↑ 17 @tillkruess
7 years ago

Replying to azaozz:

Do you remember what was the exact problem with FF? There was a small error in the base64 encoded xml matching regex. After fixing it seems FF works properly with all tests.

No can't remember exactly, sorry.

@nacin
7 years ago

#20 @nacin
7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

In trunk:

  • Doesn't work in Chrome. :-( In chrome, $element.css( 'background-image' ) doesn't include quotes or spaces. The regex assumes a space might not be there, but doesn't account for the lack of a quote. Patch attached; commit incoming.
  • base64 is a immediately-invoked function expression, and it does things other than defining other functions (speciifcally, the while loop, maybe a few other things), even though it is only needed in IE9. Can we avoid initializing all of that unless we need it?
  • Can we move this script to wp-admin? [26601] already shifted it to only being registered in the admin.
  • Can we namespace this as wp.svgPainter or something along those lines?

#21 @nacin
7 years ago

In 26663:

Fix svg-painter in Chrome. see #26333.

@nacin
7 years ago

#22 @nacin
7 years ago

Painted icons immediately lose their color after hover. Dashicons keep them for 100ms. The difference is noticeable, as the text and background also are not instant. 26333.2.diff attempts to correct that.

Other things unresolved as covered in comment:20:

  • not executing the base64 script unless we need it
  • moving the script to wp-admin. I'm happy to do it, but wanted to make sure there was agreement. I outlined some possible ways of thinking about svg-painter.js's utility in comment:5.
  • possibly namespacing it under wp. Seems like a WP-specific utility to me. It's also currently admin-specific, for that matter.

#23 @markoheijnen
7 years ago

I'm curious about this. Is there a way you need to build the SVG? I used the 1&1 logo to test it out but it ended up being a gray block. So if there are guidelines then we should write them down somewhere.

Edit: Two layers isn't a smart idea. I understand now how it works.

Last edited 7 years ago by markoheijnen (previous) (diff)

@azaozz
7 years ago

#24 @azaozz
7 years ago

In 26333.3.diff:

  • Don't execute the base64 code until needed.
  • Set svgPainter under the wp namespace.
  • Add some more error-proofing.
  • Restore setting the background-image style with !important.

Agreeing that it should be in wp-admin/js as it can only be used in the admin.

@azaozz
7 years ago

#25 @azaozz
7 years ago

In 26333.4.diff:

  • Use a local var painter instead of self.
  • No need of window.wp (wp is default global for jshint).
Last edited 7 years ago by azaozz (previous) (diff)

#26 @nacin
7 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 26693:

Final SVG painter fixes.

  • wp.svgPainter and now moved to wp-admin.
  • Restore !important background-image handling.
  • Delay executing the IE9-specific base64 code if we don't need it.
  • Make painted icons lose their color after hover at the same speed as dashicons (100ms).

props azaozz.
fixes #26333.

#27 @markoheijnen
7 years ago

I'm curious about the delay. It seems that the delay of a SVG icon is higher then dashicons. With the default icons the text is longer blue and with SVG icons the icon is longer blue.

edit: This is on hover out in Chrome

Last edited 7 years ago by markoheijnen (previous) (diff)
Note: See TracTickets for help on using tickets.