Moono: Use retina iconset

[Matthew Leffler]

Summary: The moono skin should use a retina iconset.

Steps:

• Using a retina-capable computer (MBP)
• Load the ckeditor demo ([Demo](​http://ckeditor.com/demo))

Actual result:

The iconset is normal (ie, 1x).

Expected result:

A retina (2x) iconset should be displayed.

[Jakub Ś]

Here are two links that perhaps might help detect retina display -
​http://www.quirksmode.org/blog/archives/2012/06/devicepixelrati.html
​http://anthonygthomas.com/2012/04/22/media-queries-for-high-pixel-density-devices/

[Frederico Caldeira Knabben]

I'm testing it in a MacBook with Retina. The only browser that has some negative impact is Firefox. Still not so bad, just some noticeable pixelation if you touch the screen with your eyes to check it. Chrome, Safari and Opera look very good.

Firefox should definitely do better in this sense. In any case, we certainly want to bring perfect results in Retina, so a solution for it will take place.

[Olek Nowodziński]

The following is my vision of the solution:

Detection

For JS the most common method of detecting Retina (HiDPI in general) is:

window.devicePixelRatio >= 2

See: ​Retina.js, ​Conditionizr

For CSS media queries are preferred. Explained by ​the article. Also used ​in JS.

@media only screen and (-webkit-min-device-pixel-ratio: 2), 
       only screen and (min-resolution: 192dpi) {
}

Hopefully this won't be necessary.

Implementation

The idea is to introduce:

• CKEDITOR.env.hidpi or CKEDITOR.env.retina used e.g. by CKEDITOR.skin.getIconStyle to determine correct image size. I'd prefer "hidpi" rather than "retina" because it's generic. There are more and more hidpi devices produced by other manufacturers so I assume this will be far more relevant.
• CKEDITOR.skin.hidpi boolean flag that informs that the skin supports HiDPI icons (i.e. Kama would have this disabled).
• Sub-directories:
  • For plugins.
    • Icons stored in directories named "16" and "32" (or "96 and "192") i.e.: plugins/name/icons/32/foo.png.
    • Additional images loaded by plugins (like magicline NL-arrow or anchor flag) that require manual detection with CKEDITOR.env.hidpi in plugin's code (i.e. plugins/name/images/32/foo.png).
  • For skins. I.e. skins/moono/images/32/foo.png.
• A class added to editor container (i.e. cke_hidpi) via JS. This would allow changing icons loaded by skins according to the DPI, without media queries (possibly unstable). There are only few icons loaded this way.

Packages should contain two separate sprites (32 and 64) generated by the builder. It's possible, isn't it, Wiktor?

[Wiktor Walc]

If I understood correctly, the following sample rule in a release package (in editor.css)

.cke_ltr .cke_button__source_icon{
    background: url(icons.png) no-repeat 0 -2400px !important;
}

as of 4.2 will look more or less like this:

.cke_ltr .cke_button__source_icon{
    background: url(icons16.png) no-repeat 0 -2400px !important;
}

.cke_hdpi.cke_ltr .cke_button__source_icon{
    background: url(icons32.png) no-repeat 0 -4800px !important;
}

That looks fine to me.

Of course, CKBuilder (both online version and command line) does not support this at this moment, but this can be coded.

Backwards compatibility

Not sure how the planned changes regarding the place where icons are kept will affect 3rd party plugins from the addons repository, all of them of course will not be immediately updated when 4.2 comes out. Keep in mind that they'll all have icons directly in the icons folder.

Anyway, regarding the build process, to remain compatible we'd need to do one of two things:

• have the builder still searching for files directly in the "icons" folder and in subfolders "16" and "32", to create the sprite file
• or leave the "old" icons as is in the icons folder, and create a dedicated folder (icons/hdpi or icons/32) only for hdpi icons

The second approach would be even more backwards compatible, for anyone using direct URLs to icons in his application already (e.g. in toolbar builder, an administration area where one may uncheck buttons and a button icon is displayed there etc.)

API changes

(I'm writing this for myself probably) Currently, builder adds the following code at the end of ckeditor.js, to register all icons coming from plugins:

(function() {
    var icons = ( 'about,0,bold,32,italic,64,...,spellchecker,2752' ),
        path = CKEDITOR.getUrl( 'plugins/icons.png' ),
        icons = icons.split( ',' );

    for ( var i = 0; i < icons.length; i++ )
        CKEDITOR.skin.icons[ icons[ i ] ] = { path: path, offset: -icons[ ++i ] };
})();

This should be changed to support both sizes of icons as well. In order to code this, I'd need to know what values will be now expected in CKEDITOR.skin.icons.

[Frederico Caldeira Knabben]

The plan looks great. My bits:

• CKEDITOR.env.hidpi, definitely.

• Moving the current icons is bad. Better to have "default" 16x16 icons inside icons/ and the new 32x32 ones inside icons/32/

• @wwalc, maybe @media queries should be used instead in editor.css, but this is to be confirmed by a.nowodzinski.

As for the rest, you guys are on the right track ;)

[Olek Nowodziński]

Created branch t/9923 in ckeditor-dev.

What's new?

• iconmaker: A dev, node-based utility that replaced good ol' iconextractor written in python. Because we love JS. Also because we keep icons in plugin sub-directories, which made the old tool slightly deprecated.
  • I'm wondering if keeping node dependencies (node_modules) in our repo is OK.
• CKEDITOR.env.hidpi - self-explanatory.
• cke_hidpi class for dialogs. There was no practical application for such class in editable/editor container. We can add it on demand in the future.
• icons32.svg containing enhanced iconset for HiDPI. Some icons required "de-pixelperfection". Mostly fonts (pixel ones) but also other icons needed some care.
• Dozens of icons exported to /32/ sub-directories (namely 95).
• Other images in 32px:
  • Magicline "caret icon"
  • Logotype in about dialog.
  • Anchor flag (Sic! this is so funny :D).
• To simplify things I sliced moono/images/mini.png (back) into 3 separate images. Thanks to that, these icons can also be automatically generated by iconmaker. Otherwise, the script requires additional code.

Possible TODOs (?)

• Showblocks images (with block names)?
• Having moono/images/mini.png back again?

Things to focus on while reviewing

• Firefox. I'm unable to test it on my machine to be totally sure about how things look in this browser.

Misc

HiDPI mode can be emulated on non-HiDPI displays with the following in the page stylesheet:

body { zoom: 200%; -moz-transform: scale(2); -moz-transform-origin: 0 0; }

and CKEDITOR.env.hidpi flag overwritten before editor is created. It works in Webkit. In Firefox, background-size issues emerge (possible bugs?).

[Olek Nowodziński]

Naive HiDPI emulation sample

[Frederico Caldeira Knabben]

To start off, the retina icons look wonderful. It's in fact a strong enhancement when compared to the normal ones.

Works well with Chrome, Safari and Opera in a Retina Mac.

Firefox is not on the list, but just because of one issue: the anchor icon is broken (big and cropped). It's ok for the rest.

Additionally, hidpi icons are supposed to be *optional*. I see that you proposed them as mandatory and things will simply break if they're missing. Therefore we need to talk about this, because the plugin should explicitly specify that it provides hidpi icons.

Btw, the Kama skin also worked, with I found magic. Is this logic touching icons provided by the skin as well?

Finally, this ticket must come with the feature implemented in CKBuilder as well, so building will work straight,

[Olek Nowodziński]

Updated t/9923 in ckeditor-dev

I added several features:

• pluginDefinition.hidpi property: It notifies the loadPlugins procedure in core/plugins.js that HiDPI icons are included in this plugin.

All CKEditor-maintained plugins are HiDPI ready, but some 3rd-party plugins are not. This flag is to distinguish between these two types.

• skin.hidpi property: It notifies editor that the skin provides either optimized CSS (which can handle 32px icons) or a HiDPI iconset.

The first information is crucial for loadPlugins. Even though custom skins call CKEDITOR.skin.addIcon() before plugins and "reserve" icon slots (first come first served, never overwritten), some 3rd party plugins will still load own 32px icons. And it doesn't make sense if the skin has no optimized CSS, e.g. for toolbars.

Also:

• Fixed anchor flag in FF.
• Updated moono and kama docs (this may need care).

At the moment, HiDPI behavior looks as follows:

Some custom skin

SKIN.HIDPI | PLUGIN.HIDPI | Plugin icons (toolbar)          | Plugin images (extra stuff)
-----------|--------------|---------------------------------|-----------------------------
YES        | NO           | 32px: loaded by the skin (FCFS) | 16px: loaded by the plugin
NO         | YES          | 16px: loaded by the skin (FCFS) | 32px: loaded by the plugin
NO         | NO           | 16px: loaded by the skin (FCFS) | 16px: loaded by the plugin
YES        | YES          | 32px: loaded by the skin (FCFS) | 32px: loaded by the plugin

Moono

SKIN.HIDPI | PLUGIN.HIDPI | Plugin icons (toolbar)          | Plugin images (extra stuff)
-----------|--------------|---------------------------------|-----------------------------
YES        | NO           | 16px: loaded by the plugin      | 16px: loaded by the plugin
YES        | YES          | 32px: loaded by the plugin      | 32px: loaded by the plugin|

TO-DOs

• Builder. Still talking with Wiktor and synchronizing stuff. I'm looking forward to builder beta with HiDPI support.
• ​Skin SDK guide needs care.

[Frederico Caldeira Knabben]

• We may avoid having CKEDITOR.skin.hidpi.
• In plugins, we should mark hidpi: true; with // %REMOVE_LINE_CORE%.

[Wiktor Walc]

An experimental version of CKBuilder has been uploaded as version 1.7. To try it out, change version number in build.sh

Note 1) the hidpi.html sample attached to this ticket does not work properly, unless you modify ckeditor.js in a place where CKEDITOR.env.hidpi flag is checked (iirc the trick with configLoaded changes the flag too late for release version):

So, if you do not have a hidpi device, at the bottom of ckeditor.js change

CKEDITOR.env.hidpi?b("about

into

(true||CKEDITOR.env.hidpi)?b("about

Note 2) @media query does not work when we emulate hidpi through env.hidpi flag. So for now, CKBuilder provides hidpi support using .cke_hidpi class that is set for the main container.

Note 3) Just realized that handling .cke_mixed_dir_content class is broken (might be that not only in this branch), will investigate that later.

[Wim Leers]

Wow, seems like awesome progress has been made here!

[Frederico Caldeira Knabben]

I'm fine with the code. I just made a minor commit and rebased it on major.

Still, the way the builder use the strips is not optimal. It's in fact adding 16px icons into the 32px strip and using them on Retina. This makes the icons much blurrier than the original 16px.

There should be a solution to use the 16px original strip for those icons that are 16px only.

I talked with Wiktor and we agreed that a new solution will be in place for this. Therefore, I'll wait for his ok to review this again.

[Wiktor Walc]

After a chat with Fred, we agree that we'll use "hidpi" name for folders with hidpi icons. So far CKEditor & CKBuilder supported icons of different sizes (not only 16px).

So if one used 24px icons in his skin, having to use a subfolder named "32" for 48px icons would be confusing.

For the source version of a skin, to overcome the background size that will be now set to 16px by default, it should be enough to add

.cke_button_icon{background-size: 24px !important;}

to toolbar.css.

For the release version, CKBuilder should just calculate everything properly.

[Wiktor Walc]

I just noticed that 16px icons in t/9923 branch are different than in master.

[Wiktor Walc]

I pushed two changes:

1) I renamed "32" folders into "hidpi" (explained two comments earlier)

2) I added support for setting background-size to a different value than hard-coded 16px.

The CKBuilder binary has been updated and it now creates two strip images: icons.png and icons_hidpi.png

When a plugin (or a skin) does not provide a hidpi icon, then icons_hidpi.png contains simply an icon with a normal size (which is not scaled up). Thanks to that still only one HTTP request is needed / one image is loaded in hidpi environment.

The possibility to set background-size to a value!= 16px is needed for example when the following conditions are met:

• we have a release version of a skin that does not have an icon/definition for button "Foo"
• we have a release version of CKEditor, where plugins/icons_hidpi.png contains a 16px ("1x") version of icon for "Foo" (the plugin did not provide a hidpi version of an icon)

In such case, background-size:16px wrongly reduced the size of an icon (see the "Bold" button):

[Olek Nowodziński]

Replying to wwalc:

I just noticed that 16px icons in t/9923 branch are different than in master.

Pushed improved icons to the branch to fix this.

[Frederico Caldeira Knabben]

Wonderful job guys!

[Olek Nowodziński]

Merged t/9923 into major, dev (​​git:3f161cb).