Problem/Motivation

Adding an icon sprite for UAs that don't support data URIs as arguments to the CSS url() function. This is part of toolbar followup further to code freeze.
Meta issue:#1846970: [meta] Responsive Toolbar follow-ups

Proposed resolution

To be determined.

Remaining tasks

To be determined.

User interface changes

None.

API changes

None.

Dependencies

#1849082: Implement the data URI converter for CSS images

Files: 
CommentFileSizeAuthor
#18 horiztool.jpg36.68 KBdcrocks
#18 verttool.jpg33.59 KBdcrocks
#15 toolbar-sprite-1849078-15.patch26.39 KBoresh
PASSED: [[SimpleTest]]: [MySQL] 54,039 pass(es).
[ View ]
#14 3-4-2013 9-21-52.png8.26 KBdroplet
#12 toolbarD8-Chrome.png68.75 KBahimsauzi
#12 toolbarD8-Firefox.png37.93 KBahimsauzi
#10 toolbar-sprite-1849078-10.patch24.08 KBoresh
PASSED: [[SimpleTest]]: [MySQL] 53,096 pass(es).
[ View ]
#9 after patch.png191.07 KBMandakini_Kumari
#5 toolbar-sprite-1849078-5.patch13.16 KBbryanbraun
PASSED: [[SimpleTest]]: [MySQL] 52,195 pass(es).
[ View ]

Comments

Shyamala’s picture

Status:Needs work» Active

change status to active

jenlampton’s picture

Title:Adding an icon sprite for Toolbar UAs» Replace many toolbar icon files with a single CSS sprite image

I don't know what UAs are, (User Agents?) but I'm guessing the point of this issue is to replace all the icon files in the toolbar module with a single sprite image? I think the active state versions could probably also be done with opacity rather than using a whole other icon.

Updating title. (If this is not what this issue is about, please comment so I can create a new issue) :)

Shyamala’s picture

Yes you are right 'UA' is User Agent, got copied from the description. Thanks for the title edit!

Please go ahead and create another issue for

I think the active state versions could probably also be done with opacity rather than using a whole other icon.

Owen Barton’s picture

It might be worth starting with a simpler approach, but Assetic should be able to handle sprite generation/formatting, once that is in.

bryanbraun’s picture

Status:Active» Needs review
StatusFileSize
new13.16 KB
PASSED: [[SimpleTest]]: [MySQL] 52,195 pass(es).
[ View ]

Here is a patch to get the ball rolling. This combines all the Toolbar icons into one sprite and changes the CSS to reference the sprite. I'm unsure about whether to include other icons in the sprite, like those provided by the Edit or Shortcut modules. Those icons were impacted by the change and haven't been repositioned yet.

jessebeach’s picture

Issue tags:+Novice

Adding tag novice.

This issue needs review.

droplet’s picture

Status:Needs review» Needs work

- Menu -> Help icon, some pixels cuts off
- the icons on sprite image placed too close. It should add more spacing between each icons. eg. when you increase the font size to 20px or add more paddings. the top & bottom icons will show on the menus.
- why all icons on the stylesheel missing ".toolbar" qualifier
- position of shortcuts & admins icons doesn't looks right.

webchick’s picture

Issue tags:+Spark

Tagging so we can keep track of this. Thanks for the patch, bryan!

Mandakini_Kumari’s picture

StatusFileSize
new191.07 KB

Tested patch on firefox 19.x and found below issues:

  1. Shortcuts and Admin images are not align with text.
  2. shortcut.png and icon-user.png need to be part of toolbar-sprite.png
oresh’s picture

Status:Needs work» Needs review
StatusFileSize
new24.08 KB
PASSED: [[SimpleTest]]: [MySQL] 53,096 pass(es).
[ View ]

Fixes for #5 patch:
- Background image repeat in css removed,
- Icon being cut for help and admin links changed
- Upper toolbar icons alignment fixed
- Some other icons position breaking fixed - orientation and dropdown icons
- Deleted unused icons
- Image sprite compressed with http://tinypng.org/

Please review. Tested on Chrome v24 on Ubuntu.

Shyamala’s picture

Issue tags:+Needs manual testing

+adding tags for testing

ahimsauzi’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new37.93 KB
new68.75 KB

Patch applies cleanly and tested with Firefox, chrome and Safari.

Repo of applied patch: https://github.com/ahimsauzi/Drupal-Ladder

webchick’s picture

Assigned:Unassigned» jessebeach

Awesome, thanks! Performance++

Assigning to Jesse to just give a once-over.

droplet’s picture

StatusFileSize
new8.26 KB

Don't mean to block it move forward.

+++ b/core/modules/toolbar/css/toolbar.icons.cssundefined
@@ -39,79 +42,89 @@
+  background-position: -1px -379px;
...
+  background-position: -1px -347px;

+++ b/core/modules/toolbar/css/toolbar.menu.cssundefined
@@ -87,15 +87,15 @@
+  background: url("../images/toolbar-sprite.png") 1px -503px;
...
+  background: url("../images/toolbar-sprite.png") 1px -474px;

+++ b/core/modules/toolbar/css/toolbar.theme.cssundefined
@@ -143,8 +143,8 @@
+  background: url('../images/toolbar-sprite.png') 1px -721px;

Just out of curious why these icons added "-1px". I'm only check the icon-home button. It's 21px but width of icon DIV is 20px.

Also about #7:

- the icons on sprite image placed too close. It should add more spacing between each icons. eg. when you increase the font size to 20px or add more paddings. the top & bottom icons will show on the menus.

3-4-2013 9-21-52.png

Also,
If icons meant to be used on everywhere, therefore this is remove ".toolbar" qualifier. Why not move it to CORE stylesheet ?

oresh’s picture

StatusFileSize
new26.39 KB
PASSED: [[SimpleTest]]: [MySQL] 54,039 pass(es).
[ View ]

@droplet,
I'm not sure about moving the icons to core stylesheet. And where exactly do you propose to move them?

Anyway here's a patch covering #14 issues.
I'd like it it be reviews asap, i'm working (actually done) on refactoring the css and js for toolbar module for the smacss principle and reducing the selector weight.

For example:
.toolbar .vertical to .toolbar-vertical
Hope this makes sens.

oresh’s picture

Status:Reviewed & tested by the community» Needs review
xmacinfo’s picture

Can we implement the retina-sized version (200%) of the icons? I could make these if I can find the original full-size icons.

dcrocks’s picture

StatusFileSize
new33.59 KB
new36.68 KB

Patched 8.x-dev. Wanted to see if it is faster. Still can be slow, but at least images aren't being painted 1 by 1.

xmacinfo’s picture

Created #1963886: Use HiDPI icons in the toolbar for my comment in #17.

droplet’s picture

@oresh,

Thanks, it looks better now.

@droplet,
I'm not sure about moving the icons to core stylesheet. And where exactly do you propose to move them?

Leave it to maintainers. :)

dcrocks’s picture

Not pertinent but relevant. What is the source of the icons? Is there any notion of a 'drupal standard icon' collection? As more iconic menus show up in drupal core I would think there is an interest in a consistent ui for drupal core.

robin.prieschl’s picture

What about creating an icon font?

We have played with the idea of creating an icon font for this sort of thing for Drupal and I am more than happy allocating time for it.

oresh’s picture

Assigned:jessebeach» Unassigned

@robin.prieschl, So, we talked about icon fonts.
Guys are trying to write an understandable document on using icon font. The thing is - everybody knows how to use sprites, but not a lot of people know how to use icon fonts. I've never done that before for example.
Also this issues is about sprites. Font icon will weight more then this sprite ( i guess ) and it needs specific documentation, when sprite do not. So if you want font icons search for this issue, if there's nothing about that, create a follow up :)

xmacinfo’s picture

@robin.prieschl: Yes, please open a new issue for icon fonts.

dcrocks’s picture

I tried re-doing this with icon fonts instead of images or a sprite file. It is pretty straight forward but before I open another issue with a patch there are some things to think about. I don't see any performance or page size issues but using a font or a sprite file does mitigate the 'late' drawing of the images when the page is repainted that you see with individual images. The icon font solution does lessen the css required. The problems I wasn't able to or couldn't figure out how to address are:

1) The toolbar is actually built from 4 different menus from 4 different modules; toolbar, user, shortcut, and contextual. With the possibility of other modules, including contrib, chiming in. Currently each module does its own styling of added icons. So, any changes to toolbar icons have to be aware that other modules have to be considered, and any other module adding to the toolbar has to be aware it needs to do its own styling for icons.

2) There are accessibility issues that arise from using icons with links and they all apply to the current toolbar implementation. I don't see a straightforward way to address these in the current implementation. Are there any future plans for enhancing the toolbar module?

droplet’s picture

I think we need to create an issue to address the icon link pattern.

Naming all icons in CORE and make the icons button/links html structure more consistently.

eg.

<i class="icon-flag"></i><a href="#">Link</a>
<i class="icon-menu"></i><button>Button</button>

It will be more easy to reuse/replace SAME icon everywhere.

droplet’s picture

Status:Needs review» Reviewed & tested by the community

Review again, it's good enough now, other issue could work in a new issue thread. RTBC

dcrocks’s picture

This does not convert the code in modules user, shortcut, and contextual to using the sprite file. You still get the late painting affect. Is it really better for those to be separate issues? You will still see the late painting affect.

droplet’s picture

@dcrocks,

What is "late painting affect" ?

dcrocks’s picture

In the current D8, there are times when the page you are on is completely redrawn on the screen. For example if you were in the admin overlay and made some change, saved, and after the overlay returned you click on the 'home' link. When the page is redrawn you 1st see the home page itself and the toolbar, but it only has the text. Then the icon images are painted on the toolbar overlay one by one with a perceptible pause between each. With the sprite patch you only see this on user and shortcut link icons because they are still using background images.
This is very evident on firefox(20.0), much less so on chrome(26..) and less on safari(5.1.9). I'm sure it is also a factor of my machine's age, old but not that old. I see it even when I use a fast server on a fast connection that is idle otherwise. Each image can mean a trip to the server, that's why sprites are better. I am beginning to think that it is firefox specific, but a lot of people use firefox.

droplet’s picture

Status:Reviewed & tested by the community» Needs work

Ahh. missing 2 icons in sprite image.

eatings’s picture

Not all elements that appear in the toolbar are provided by toolbar.module, as described in #25. I don't think it's reasonable to grab images from edit and shortcut module into a sprite of toolbar.module's images right now, as 1. there is no consistent battleplan yet for how to handle icons in the menu additional to those provided by core, 2. we can't just shove every icon image that may ever appear into a sprite that lives in toolbar. (This is probably an argument for using a symbol font, which is probably the long-term play here).

All this to say, if we are to create a sprite, we should for the scope of this ticket limit it to the images currently provided by toolbar module alone, and worry about how to more efficiently package edit and shortcut and whatever other modules's admin icons in a separate ticket. It's not a full solution, but it would provide savings in terms of number of requests anyway.

eatings’s picture

Patch from #15 does not apply cleanly to 8.x HEAD as of May 24.

Gonna attempt a re-roll.

hass’s picture

Status:Needs work» Postponed

Let's get the monster patch #1938044: Prefix all toolbar classes to prevent theme clashes in first and then re-role here.

mgifford’s picture

Issue tags:+sprite

tagging

hass’s picture

Status:Postponed» Needs work

Back to CNW.

oresh’s picture

So, are we still going to create a sprite, or a icon font or something?

jessebeach’s picture

Status:Needs work» Closed (won't fix)

We're addressing toolbar icons in #1963886: Use HiDPI icons in the toolbar. That issue supercedes this issue.

hass’s picture

Status:Closed (won't fix)» Needs work

It looks like these issue does not implement a sprite to reduce http requests to an absolute minimum. The other case still requests a lot if single files.

jessebeach’s picture

That's true. We're using SVGs as the primary icon source, which means we can't spritesheet them. Only older browsers that don't support SVGs will get the PNG fallbacks. If we spritesheet the PNGs, it will be to benefit these older browsers.

jessebeach’s picture

Issue summary:View changes

added details

angel.h’s picture

Issue summary:View changes

"uris" is incomprehensible.

Sam152’s picture

Given that IE8 and lower is no longer supported in core, do we need to worry about generating the spritesheet for the fallback icons?

The toolbar (and anything that needs jQuery for that matter) doesn't even remotely work in IE8, so getting the fallback icons (which don't even match the SVG files) into a spritesheet seems superfluous.

Can we close this issue jessebeach?

jwilson3’s picture

Issue summary:View changes

We can definitely still do some optimizations here, eg the color variants can all be added into one single SVG and use css background position to select the color to use. We can either use SVGZ for better compression of external files, or place the uncompressed SVGs into data URIs inside CSS to reduce HTTP requests. I just started the discussion here: #2314959: Optimize SVGs (Libricons), and I'm inclined to mark this as a child (or duplicate?) of that issue.

tkoleary’s picture

We can definitely still do some optimizations here, eg the color variants can all be added into one single SVG and use css background position to select the color to use

Why not just have the different color states in the CSS embedded in the SVG? eg. https://useiconic.com/tour/

xmacinfo’s picture

Indeed, SVG are part of the DOM that is available to CSS.

nod_’s picture

Checked out iconic, it uses a script to do talk between the data attributes and the SVG included. I'm not fond of this. I'd much rather we inline the SVG and control it with CSS than use something like iconic in core.

If we're still going the <img> way, then SVG sprites makes sense.

tkoleary’s picture

@nod

Right, I forgot that they use js to do the inlining of the svgs.

LewisNyman’s picture

Related: #2306499: Experiment with styling inline SVGs using CSS

It seems like it's simple to inline SVGs in PHP?

tkoleary’s picture

eclipsegc told me a few days ago that the fact that menus are now entities means that they can include images just as any other piece of content would.

Wouldn't that solve the problem and provide a much more developer friendly solution for those who want to add items to toolbar?

LewisNyman’s picture

I think we need a light wrapper around how we implement icons so we can change the implementation easily. We are using them all over place in core and there are situations where contrib is extending the icon set. What happens if an admin theme uses a different icon set and wants to swap them out? I think it's time to reopen #1849712: Add theming template and prepare logic for rendering icons

Wim Leers’s picture

Status:Needs work» Closed (works as designed)
Issue tags:-Novice

Most of this issue (until #40) was about sprites. But now that we no longer have PNGs, sprites are simply unnecessary.

Instead, we've shifted the conversation towards using SVGs more optimally: coloring them differently via CSS. But doing that is blocked on #2306499: Experiment with styling inline SVGs using CSS and arguably also #1849712: Add theming template and prepare logic for rendering icons.

I added a comment to #2306499: Experiment with styling inline SVGs using CSS linking to this issue and stating that if that issue ever materializes, it should deal with the toolbar as its primary example.

Hence we can now close this issue, since the original purpose, and the majority of what was discussed here, is no longer relevant.