Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#18 | horiztool.jpg | 36.68 KB | dcrocks |
#18 | verttool.jpg | 33.59 KB | dcrocks |
#15 | toolbar-sprite-1849078-15.patch | 26.39 KB | oresh |
#14 | 3-4-2013 9-21-52.png | 8.26 KB | droplet |
#12 | toolbarD8-Chrome.png | 68.75 KB | ahimsauzi |
Comments
Comment #1
Shyamala CreditAttribution: Shyamala commentedchange status to active
Comment #2
jenlamptonI 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) :)
Comment #3
Shyamala CreditAttribution: Shyamala commentedYes 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
Comment #4
Owen Barton CreditAttribution: Owen Barton commentedIt might be worth starting with a simpler approach, but Assetic should be able to handle sprite generation/formatting, once that is in.
Comment #5
bryanbraun CreditAttribution: bryanbraun commentedHere 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.
Comment #6
jessebeach CreditAttribution: jessebeach commentedAdding tag novice.
This issue needs review.
Comment #7
droplet CreditAttribution: droplet commented- 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.
Comment #8
webchickTagging so we can keep track of this. Thanks for the patch, bryan!
Comment #9
Mandakini_Kumari CreditAttribution: Mandakini_Kumari commentedTested patch on firefox 19.x and found below issues:
Comment #10
oresh CreditAttribution: oresh commentedFixes 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.
Comment #11
Shyamala CreditAttribution: Shyamala commented+adding tags for testing
Comment #12
ahimsauziPatch applies cleanly and tested with Firefox, chrome and Safari.
Repo of applied patch: https://github.com/ahimsauzi/Drupal-Ladder
Comment #13
webchickAwesome, thanks! Performance++
Assigning to Jesse to just give a once-over.
Comment #14
droplet CreditAttribution: droplet commentedDon't mean to block it move forward.
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:
Also,
If icons meant to be used on everywhere, therefore this is remove ".toolbar" qualifier. Why not move it to CORE stylesheet ?
Comment #15
oresh CreditAttribution: oresh commented@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.
Comment #16
oresh CreditAttribution: oresh commentedComment #17
xmacinfoCan we implement the retina-sized version (200%) of the icons? I could make these if I can find the original full-size icons.
Comment #18
dcrocks CreditAttribution: dcrocks commentedPatched 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.
Comment #19
xmacinfoCreated #1963886: Use HiDPI icons in the toolbar for my comment in #17.
Comment #20
droplet CreditAttribution: droplet commented@oresh,
Thanks, it looks better now.
Leave it to maintainers. :)
Comment #21
dcrocks CreditAttribution: dcrocks commentedNot 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.
Comment #22
robin.prieschl CreditAttribution: robin.prieschl commentedWhat 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.
Comment #23
oresh CreditAttribution: oresh commented@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 :)
Comment #24
xmacinfo@robin.prieschl: Yes, please open a new issue for icon fonts.
Comment #25
dcrocks CreditAttribution: dcrocks commentedI 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?
Comment #26
droplet CreditAttribution: droplet commentedI 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.
It will be more easy to reuse/replace SAME icon everywhere.
Comment #27
droplet CreditAttribution: droplet commentedReview again, it's good enough now, other issue could work in a new issue thread. RTBC
Comment #28
dcrocks CreditAttribution: dcrocks commentedThis 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.
Comment #29
droplet CreditAttribution: droplet commented@dcrocks,
What is "late painting affect" ?
Comment #30
dcrocks CreditAttribution: dcrocks commentedIn 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.
Comment #31
droplet CreditAttribution: droplet commentedAhh. missing 2 icons in sprite image.
Comment #32
eatingsNot 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.
Comment #33
eatingsPatch from #15 does not apply cleanly to 8.x HEAD as of May 24.
Gonna attempt a re-roll.
Comment #34
hass CreditAttribution: hass commentedLet's get the monster patch #1938044: Prefix all toolbar classes to prevent theme clashes in first and then re-role here.
Comment #35
mgiffordtagging
Comment #36
hass CreditAttribution: hass commentedBack to CNW.
Comment #37
oresh CreditAttribution: oresh commentedSo, are we still going to create a sprite, or a icon font or something?
Comment #38
jessebeach CreditAttribution: jessebeach commentedWe're addressing toolbar icons in #1963886: Use HiDPI icons in the toolbar. That issue supercedes this issue.
Comment #39
hass CreditAttribution: hass commentedIt 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.
Comment #40
jessebeach CreditAttribution: jessebeach commentedThat'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.
Comment #40.0
jessebeach CreditAttribution: jessebeach commentedadded details
Comment #40.1
angel.h"uris" is incomprehensible.
Comment #41
Sam152 CreditAttribution: Sam152 commentedGiven 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?
Comment #42
jwilson3We 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.
Comment #43
tkoleary CreditAttribution: tkoleary commentedWhy not just have the different color states in the CSS embedded in the SVG? eg. https://useiconic.com/tour/
Comment #44
xmacinfoIndeed, SVG are part of the DOM that is available to CSS.
Comment #45
nod_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.Comment #46
tkoleary CreditAttribution: tkoleary commented@nod
Right, I forgot that they use js to do the inlining of the svgs.
Comment #47
LewisNymanRelated: #2306499: Experiment with styling inline SVGs using CSS
It seems like it's simple to inline SVGs in PHP?
Comment #48
tkoleary CreditAttribution: tkoleary commentedeclipsegc 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?
Comment #49
LewisNymanI 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
Comment #50
Wim LeersMost 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.