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

Adding classList support to the various class manipulation methods #228

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
8 participants
@dominicbarnes

(this is my first pull request with jQuery, so bear with me)

The following functions in attributes were modified: addClass, removeClass, toggleClass, hasClass

Added support for the HTML5 classList feature. As far as I can tell, at least Firefox and Chrome both support this.

I've added this in such a way that if it does not exist in the DOM for a particular element, it continues with the original method.

I'm more than happy to refactor my code if necessary, but I made sure that I passed the JSLint bulid test as well as all the tests in the jQuery Test Suite.

@louisremi

This comment has been minimized.

Show comment
Hide comment
@louisremi

louisremi Feb 11, 2011

Contributor

code looks good, but this adds 26 lines of js, what does it adds to performance?

Contributor

louisremi commented Feb 11, 2011

code looks good, but this adds 26 lines of js, what does it adds to performance?

@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Feb 11, 2011

Contributor

You should create a http://jsperf.com/ test to show the performance benefit.

Contributor

sindresorhus commented Feb 11, 2011

You should create a http://jsperf.com/ test to show the performance benefit.

var elem = this[i];
if ( elem.classList ) {
if ( elem.classList.contains( selector ) ) {
return true;

This comment has been minimized.

@sindresorhus

sindresorhus Feb 11, 2011

Contributor

This can be shortened to:
return elem.classList.contains( selector );

@sindresorhus

sindresorhus Feb 11, 2011

Contributor

This can be shortened to:
return elem.classList.contains( selector );

This comment has been minimized.

@dominicbarnes

dominicbarnes Feb 11, 2011

I don't believe so, since we don't want to return false until all the items in the collection have been checked. Just like line 172-4.

@dominicbarnes

dominicbarnes Feb 11, 2011

I don't believe so, since we don't want to return false until all the items in the collection have been checked. Just like line 172-4.

This comment has been minimized.

@sindresorhus

sindresorhus Feb 11, 2011

Contributor

You're right. I was a little bit too fast there.

@sindresorhus

sindresorhus Feb 11, 2011

Contributor

You're right. I was a little bit too fast there.

@dominicbarnes

This comment has been minimized.

Show comment
Hide comment
@dominicbarnes

dominicbarnes Feb 11, 2011

I created some JSPerf test cases. This is my first time doing that as well, but I think I structured the tests well enough.

The only one that performed any faster was toggleClass, which is definately not the most common use-case. Not only that, but the performance hit to the others seems pretty big, at least based on these tests.

I created some JSPerf test cases. This is my first time doing that as well, but I think I structured the tests well enough.

The only one that performed any faster was toggleClass, which is definately not the most common use-case. Not only that, but the performance hit to the others seems pretty big, at least based on these tests.

@louisremi

This comment has been minimized.

Show comment
Hide comment
@louisremi

louisremi Feb 11, 2011

Contributor

Interesting

Contributor

louisremi commented Feb 11, 2011

Interesting

@dominicbarnes

This comment has been minimized.

Show comment
Hide comment
@dominicbarnes

dominicbarnes Feb 11, 2011

Yeah, I was surprised that a "native" feature like this would be slower than RegExp and string manipulation.

Yeah, I was surprised that a "native" feature like this would be slower than RegExp and string manipulation.

@louisremi

This comment has been minimized.

Show comment
Hide comment
@louisremi

louisremi Feb 11, 2011

Contributor

compared to simplified string manipulation methods, native API is actually way faster: http://jsperf.com/classname-manipulation
The perf problem with the classList API implementation might be somewhere else.

Contributor

louisremi commented Feb 11, 2011

compared to simplified string manipulation methods, native API is actually way faster: http://jsperf.com/classname-manipulation
The perf problem with the classList API implementation might be somewhere else.

@paulirish

This comment has been minimized.

Show comment
Hide comment
@paulirish

paulirish Feb 11, 2011

Member

This is associated with http://bugs.jquery.com/ticket/5087

The thought in that ticket was to create a $.support.classlist to avoid the check on every use.

Member

paulirish commented Feb 11, 2011

This is associated with http://bugs.jquery.com/ticket/5087

The thought in that ticket was to create a $.support.classlist to avoid the check on every use.

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Feb 11, 2011

Member

+1 to paulirish's note about adding jQuery.support.classList

Member

rwaldron commented Feb 11, 2011

+1 to paulirish's note about adding jQuery.support.classList

@dominicbarnes

This comment has been minimized.

Show comment
Hide comment
@dominicbarnes

dominicbarnes Feb 11, 2011

I'll try applying that, and I'll run those tests again before submitting another pull request.

I'll try applying that, and I'll run those tests again before submitting another pull request.

@paulirish

This comment has been minimized.

Show comment
Hide comment
@paulirish

paulirish Feb 11, 2011

Member

github protip: all following commits on this branch will automatically be added to the pull request. :)

Member

paulirish commented Feb 11, 2011

github protip: all following commits on this branch will automatically be added to the pull request. :)

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Feb 11, 2011

Member

@dominicbarnes - I actually have several months worth of research and perf testing that I've done with regard to classList
s viability as an optimization. The latest and plainest perf is here: http://jsperf.com/classlist-vs-addclass (empty browserscope blocks are where the UA does not implement)

At present, classList is not as performant as jQuery. As much as I'd like to see perf optimizations whenever possible, the classList API doesn't come in fast enough when dealing with a large number of elements that have multiple classes.

Member

rwaldron commented Feb 11, 2011

@dominicbarnes - I actually have several months worth of research and perf testing that I've done with regard to classList
s viability as an optimization. The latest and plainest perf is here: http://jsperf.com/classlist-vs-addclass (empty browserscope blocks are where the UA does not implement)

At present, classList is not as performant as jQuery. As much as I'd like to see perf optimizations whenever possible, the classList API doesn't come in fast enough when dealing with a large number of elements that have multiple classes.

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Feb 11, 2011

Member

So, here's the scoop... I pulled this into my local clone and ran the test suite - it passed. BUT after reviewing the code I noticed there are no tests for multiple class adding and removing. So, these should suffice: http://pastebin.com/4rDRUhMk

Please add those in somewhere around line 604 of test/unit/attributes.js

Member

rwaldron commented Feb 11, 2011

So, here's the scoop... I pulled this into my local clone and ran the test suite - it passed. BUT after reviewing the code I noticed there are no tests for multiple class adding and removing. So, these should suffice: http://pastebin.com/4rDRUhMk

Please add those in somewhere around line 604 of test/unit/attributes.js

@dominicbarnes

This comment has been minimized.

Show comment
Hide comment
@dominicbarnes

dominicbarnes Feb 11, 2011

In my local repo, I added jQuery.support.classList and used that instead of checking elem.classList. It made little, if any difference to the overall performance. @rwldrn is right on this one.

In my local repo, I added jQuery.support.classList and used that instead of checking elem.classList. It made little, if any difference to the overall performance. @rwldrn is right on this one.

@louisremi

This comment has been minimized.

Show comment
Hide comment
@louisremi

louisremi Feb 13, 2011

Contributor

@dominicbarnes: accessing elem.classList is one DOM access and this is something we're trying to avoid whenever possible.

Contributor

louisremi commented Feb 13, 2011

@dominicbarnes: accessing elem.classList is one DOM access and this is something we're trying to avoid whenever possible.

@dominicbarnes

This comment has been minimized.

Show comment
Hide comment
@dominicbarnes

dominicbarnes Feb 13, 2011

Yeah, that occurs in both add/remove. It was added in order to pass the unit tests. Removing it helped, and it made remove actually faster than the original in my first few tests. I should probably add a few of these changes to this pull request.

Yeah, that occurs in both add/remove. It was added in order to pass the unit tests. Removing it helped, and it made remove actually faster than the original in my first few tests. I should probably add a few of these changes to this pull request.

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Feb 13, 2011

Member

lrbabe's comment just got me thinking (note: everything i'm about to say is speculation - I'm posting from my mobile and can't actually test this)... I'm wondering if classList.add/remove are causing reflows? Technically any operation that has potential to change the geometry of an element will cause a reflow. If we look at JUST addClass, we're talking about moving from a single property setting operation and one reflow to a function call and a reflow for each class passed - which is only one if the
function is passed only one class, but of course - potentially many.

Member

rwaldron commented Feb 13, 2011

lrbabe's comment just got me thinking (note: everything i'm about to say is speculation - I'm posting from my mobile and can't actually test this)... I'm wondering if classList.add/remove are causing reflows? Technically any operation that has potential to change the geometry of an element will cause a reflow. If we look at JUST addClass, we're talking about moving from a single property setting operation and one reflow to a function call and a reflow for each class passed - which is only one if the
function is passed only one class, but of course - potentially many.

@jeresig

This comment has been minimized.

Show comment
Hide comment
@jeresig

jeresig Apr 12, 2011

Member

So it seems like this doesn't provide any perf benefit over our current solution (which is unfortunate) - thus we'll probably want to hold off on landing this until we can get some appreciable gains.

Member

jeresig commented Apr 12, 2011

So it seems like this doesn't provide any perf benefit over our current solution (which is unfortunate) - thus we'll probably want to hold off on landing this until we can get some appreciable gains.

@jeresig jeresig closed this Apr 12, 2011

@paulirish

This comment has been minimized.

Show comment
Hide comment
@paulirish

paulirish Apr 13, 2011

Member

FYI I asked the webkit team if there are any optimizations that could be made at browser level to make it more attractive and they said

One of the problem with almost all the js solutions I've seen is that they are not complete. In other words they fail with some input data. If it turns out that a js solution that is complete is faster than the webkit one we should probably look at this again.

For completeness see http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/third_party/WebKit/LayoutTests/fast/dom/HTMLElement/script-tests/class-list.js&q=class-list&exact_package=chromium&sa=N&cd=2&ct=rc

Member

paulirish commented Apr 13, 2011

FYI I asked the webkit team if there are any optimizations that could be made at browser level to make it more attractive and they said

One of the problem with almost all the js solutions I've seen is that they are not complete. In other words they fail with some input data. If it turns out that a js solution that is complete is faster than the webkit one we should probably look at this again.

For completeness see http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/third_party/WebKit/LayoutTests/fast/dom/HTMLElement/script-tests/class-list.js&q=class-list&exact_package=chromium&sa=N&cd=2&ct=rc

@HolgerJeromin

This comment has been minimized.

Show comment
Hide comment
@HolgerJeromin

HolgerJeromin Dec 29, 2016

The layouttests from the last comment is now available here.

Has someone newer performance number that those 6 years old ones?
Edit:
jsperf.com is up again!

HolgerJeromin commented Dec 29, 2016

The layouttests from the last comment is now available here.

Has someone newer performance number that those 6 years old ones?
Edit:
jsperf.com is up again!

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Dec 29, 2016

Member

@HolgerJeromin You can fire up test cases in the repository of my jquery-classlist plugin. We've since switched from using the className property to using the class attribute to support SVG class manipulation.

We don't plan to switch to classList in any predictable future as even IE 11 doesn't support classList in SVG so we'd have to maintain duplicate code paths; performance gains would have to be huge in real-world scenarios (not just jsperf micro-benchmarks) to make the additional code path worth it.

Member

mgol commented Dec 29, 2016

@HolgerJeromin You can fire up test cases in the repository of my jquery-classlist plugin. We've since switched from using the className property to using the class attribute to support SVG class manipulation.

We don't plan to switch to classList in any predictable future as even IE 11 doesn't support classList in SVG so we'd have to maintain duplicate code paths; performance gains would have to be huge in real-world scenarios (not just jsperf micro-benchmarks) to make the additional code path worth it.

@HolgerJeromin

This comment has been minimized.

Show comment
Hide comment
@HolgerJeromin

HolgerJeromin Dec 29, 2016

OK, fine with me.
I had performance issues with this, but just changed to native DOM api in the critical and easy cases (one element, one className, HTML only, hundreds of calls)

OK, fine with me.
I had performance issues with this, but just changed to native DOM api in the critical and easy cases (one element, one className, HTML only, hundreds of calls)

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Dec 29, 2016

Member

@HolgerJeromin If you have performance issues with the current implementation on a real site, we would definitely want to know! Usually it turns out other things are responsible.

It's easiest to check that by including my linked plugin and seeing if it improves performance in a visible way.

Member

mgol commented Dec 29, 2016

@HolgerJeromin If you have performance issues with the current implementation on a real site, we would definitely want to know! Usually it turns out other things are responsible.

It's easiest to check that by including my linked plugin and seeing if it improves performance in a visible way.

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