Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Font Awesome places a strain on CSS selector engine #568

Closed
greim opened this issue Dec 7, 2012 · 30 comments
Closed

Font Awesome places a strain on CSS selector engine #568

greim opened this issue Dec 7, 2012 · 30 comments
Assignees
Milestone

Comments

@greim
Copy link

greim commented Dec 7, 2012

[class^="icon-"]:before, [class*=" icon-"]:before

Based on some tests I did in Chrome's CSS profiler, the above selectors put a lot of strain on the browser's CSS engine, which is bad for dynamic sites with animations or other things that depend on smooth reflow. Not so much an issue for static "brochure-style" sites. The reason is it forces substring parsing on all class-bearing elements.

What if FontAwesome had an alternate performance-enhanced stylesheet such that:

/* css */
.icon:before, .icon:after

<!-- html -->
<i class="icon icon-remove">

Not as pretty in the markup, but should put significantly less strain on the browser for those of us who are running into performance issues with Font Awesome.

@greim
Copy link
Author

greim commented Dec 7, 2012

See the alternate stylesheet in the forked version of the repo here:

https://github.com/greim/Font-Awesome/blob/master/css/font-awesome-fast.css

Haven't tested yet, but this shows the basic idea. I may use this fork in my own projects, to avoid the above issues.

@aensley
Copy link

aensley commented Dec 7, 2012

+1

This is how jQuery UI does it (with a 'ui-icon' class and 'ui-icon-' sub-classes). This change would take a huge load off the browser.

I've just loaded your forked version and it works perfectly. Thank you!

@davegandy
Copy link
Member

This is an interesting suggestion. Would also mean people could directly reference the PUA code. And combine this with some other magic, and we might have better usability. Thanks for the heads up.

@davegandy
Copy link
Member

Combine this with #589 and we might have a winner.

@Arkkimaagi
Copy link

+1 for the class="icon icon-remove" approach. I think it would be a good way to help people get more efficient sites.

@davegandy
Copy link
Member

Agreed. Tentatively putting this in 4.0, as it would break backward compatibility.

@jjlharrison
Copy link

+1 from me.

I personally add the 'icon' class anyway.

@aensley
Copy link

aensley commented May 10, 2013

Yeah, I've been hacking each release to do this anyway, so it would be nice not to have to do that. Maybe you could provide two stylesheets to maintain backwards compatibility:

  • The current style, as is in font-awesome.css, no file name change.
  • The new icon icon-remove style in a font-awesome-optimized.css file (or whatever you'd like to call it).

Just a thought. The changes in the CSS file to implement this new style aren't very complex.

@Arkkimaagi
Copy link

Yep, you can have <i class="icon icon-remove"></i> supported with both approaches at the same time, while <i class="icon-remove"></i> is supported only by the classic approach, and like flamadiddle said, it's probably quite easy to have both versions in separate files for a while so that people can migrate.

@davegandy
Copy link
Member

I know it's a little hard to tell, but this is one of the 4.0 milestones. And the prefix will likely be easily customizable, as well. I'm thinking of just using something like <i class="fa fa-rocket"></i>.

A lot of groundwork has been laid in the 3.1.0-wip branch for making these changes very quickly.

@aensley
Copy link

aensley commented May 10, 2013

Just my two cents: please either go with your plan to make the prefix customizable or use icon as in the examples above. My projects already have a lot of code using this pattern and it would be a pain to have to update them.

Glad to see there's already work on this. Thank you!

@davegandy
Copy link
Member

Global project search and replace not do it for you? :)

@aensley
Copy link

aensley commented May 10, 2013

It might for one or two of them, but 'icon' is a pretty common term...

@varemenos
Copy link

I'm against adding a 2nd general class just to turn an element into an icon.

Specially with the 4.0 version release and it's plan to use something like this <i class="fa-delete">.
Would we try anything of the following?

<i class="fontawesome fa-delete"> // kinda too specific and long to write
<i class="fa fa-delete"> // .fa would definitely break a site or two
<i class="fontawesome fontawesome-delete"> // this is definitely way to long to type

They all look kinda sily to me, I'd rather replace this:

[class^="icon-"]:before, [class*=" icon-"]:before

with this:

.icon-a:before,
.icon-b:before,
.icon-c:before,
.icon-d:before{
}

(generating this with SASS would be a breeze (creating a variables collection (array-like data) and then iterating the array with @each).

tl;dr, my suggestion is that we should make the process less painful for the FA users, not the FA developers.

@davegandy
Copy link
Member

@varemenos I can play around with the css file size when gzipped to see the difference in size. If that technique can be pulled off without too much overhead in size, it might be ideal. But if it bloats the file size too much, we'll have to find another solution. This is a very good thought to push back on though. Thanks for it. I agree, two classes is worse than one.

@varemenos
Copy link

If my calculations are correct, the minified version (with 300ish icon classes) will have about 10KB of extra filesize.

@davegandy
Copy link
Member

Does that include gzipping? Everything should be gzipped across the wire, so that's the stat that really matters.

@varemenos
Copy link

That only includes minifing the css stylesheet.

@dstroot
Copy link

dstroot commented Aug 9, 2013

With Bootstrap 3 at release candidate it would be nice to see FA 4 come out soon as well. My view is just get to 4.0 with whatever new selector approach makes sense. Then in 4.1, 4.2 etc get back to adding features/icons.

@tagliala
Copy link
Member

@varemenos there are also some hacks here and there, so a common class for icons should be needed

E.g: https://github.com/FortAwesome/Font-Awesome/blob/master/less/bootstrap.less#L36

fontawesome fa-* could be fine and is shorter than
glyphicon glyphicon-*

@ghost ghost assigned tagliala Aug 21, 2013
@tagliala
Copy link
Member

@ everyone please express your thoughts about this


edit: I like .faicon.fa-icon_name, e.g.: .faicon.fa-user more than the solution I proposed here. Let me know.


I would go for .fontawesome.fa-icon_name, e.g.: .fontawesome.fa-user I will explain why:

  • we must not use icon-* anymore to avoid issues with icon-* universal selector. Classes like icon-2x and icon-spin would be a problem. If we don't use icon-*, we can get rid of all the universal selectors in bootstrap.less
  • fa-icon_name should not be a common name, so they should not break most of existing layouts.
  • If we use .fa.fa-icon_name instead, it will break someone's design because .fa is short and bad and someone could be using it for his stylesheets... (look at @varemenos comment above)
  • .fontawesome.fontawesome-icon is too long and we can experience the following issue: let's say we have an icon called spin (or 2x, which represents "2x speed" in a media player) then we will have a name collision: .fontawesome-spin will be the class for both spin animation and spin icon
  • Without a common class (e.g: .fontawesome-icon_name), it's hard to make hacks, icon stacks and you need universal selector to change the look and feel in your stylesheet, e.g.:
<div class="only-red-icons-inside-this-div">
  <span class="fontawesome-user"></span> User
  <span class="fontawesome-fire"></span> Fire
</div>

You can't assign the red color to all icons inside the main div without using universal selectors or explicitly list all of them.

Sorry for my English

@tagliala tagliala mentioned this issue Aug 22, 2013
@aensley
Copy link

aensley commented Aug 22, 2013

@tagliala I agree with your points, and I think they are the same as others above.

I have a quick question for you about the common class: What do you think of the .icon.icon-user pattern as used in the first post on this issue? I think it answers all your concerns and is shorter too.

@tagliala
Copy link
Member

I would avoid icon-* because we could get rid of all the resets (with universal selectors) here: https://github.com/FortAwesome/Font-Awesome/blob/master/less/bootstrap.less

AFAIK Dave want to keep compatibility with BS 2.3.2, so icon-* matters

.faicon.fa-* would be fine too. 2x and spin will become faicon-2x, faicon-spin

edit I like .faicon.fa-* more than .fontawesome.fa-* :)

@aensley
Copy link

aensley commented Aug 22, 2013

As suggested earlier, the new pattern could be in a new stylesheet with a different name so that backwards compatibility wouldn't be broken. Anyone adjusting to this new pattern is going to have to update their code regardless, so changing the source for the stylesheet would be a trivial change. If that doesn't happen, I really hope the common class is configurable.

@tagliala
Copy link
Member

tagliala commented Oct 1, 2013

I think this one is closed via e32be98

@tagliala tagliala closed this as completed Oct 1, 2013
@tagliala
Copy link
Member

tagliala commented Oct 1, 2013

@davegandy reopen if I'm wrong

@aensley
Copy link

aensley commented Oct 1, 2013

It seems like e32be98 will break backwards compatibility, but I'm glad to see a change in this direction!

@tagliala
Copy link
Member

tagliala commented Oct 1, 2013

It's more than fine, because it will come in 4.0.0, a new major release.

@aensley
Copy link

aensley commented Oct 1, 2013

Ok. Just thought I'd point that out since it was part of the discussion on this issue. :-)

@awebdeveloper
Copy link

in what state is 4.0.0. Just the icons... not in conjunction with various bs components but not expect major issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants