Bug 17708 - DOMTokenList: Ability to swap a class
DOMTokenList: Ability to swap a class
Status: RESOLVED WONTFIX
Product: WebAppsWG
Classification: Unclassified
Component: DOM
unspecified
PC All
: P2 normal
: ---
Assigned To: Anne
public-webapps-bugzilla
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-06 21:29 UTC by Marcos Caceres
Modified: 2012-11-09 14:41 UTC (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Marcos Caceres 2012-07-06 21:29:11 UTC
This is a feature request to add the ability to swap one class for another in DOMTokenList. 

At the W3C advance mobile web dev course, we recently ran an "experiment" where we asked students to use CSS's "transition:" to show and hide a <p> element based by clicking on a <h1> element (see https://vimeo.com/43676508 for a video of what they were asked to build). 

In the results the students submitted, we saw a lot of this:

$("article").removeClass("closed").addClass("opened");

If developers are going to swap CSS classes around in the method above, then they might as well have an efficient means to do that. 

Or other similar code. We also saw evidence that developers assume that class lists retain order:

if(elem.classList === "x y") {
   .... 
} 

This could lead to confusion when using DOMTokenList toggle(), as toggle destroys the ordering of CSS class list names of an attribute.  

Given the above, I'll like to request we add swap() method to DOMTokenList(). It would swap one token for another while retaining item order.
Comment 1 Glenn Maynard 2012-07-06 23:07:19 UTC
(In reply to comment #0)
> $("article").removeClass("closed").addClass("opened");
> 
> If developers are going to swap CSS classes around in the method above, then
> they might as well have an efficient means to do that. 

What do you mean by "efficient"?  I doubt there's any performance issue here.  If you mean "less typing", this is already pretty concise (even without the chaining shortcut).

> Or other similar code. We also saw evidence that developers assume that class
> lists retain order:
> 
> if(elem.classList === "x y") {
>    .... 
> } 
> 
> This could lead to confusion when using DOMTokenList toggle(), as toggle
> destroys the ordering of CSS class list names of an attribute.  
> 
> Given the above, I'll like to request we add swap() method to DOMTokenList().
> It would swap one token for another while retaining item order.

This seems to be increasing the surface area of the API just to make it easier to get bad practices to work.  It encourages people to write more code that depends on the order of the class list--which should be discouraged--instead of fixing the real problem (use classList.contains(), not string comparison).
Comment 2 Marcos Caceres 2012-07-07 10:33:39 UTC
(In reply to comment #1)
> (In reply to comment #0)
> > $("article").removeClass("closed").addClass("opened");
> > 
> > If developers are going to swap CSS classes around in the method above, then
> > they might as well have an efficient means to do that. 
> 
> What do you mean by "efficient"?  I doubt there's any performance issue here. 

There is potentially a significant performance gain. Check this out: 
http://youtu.be/hZJacl2VkKo 

(not that swap could also have been used there... but also toggle())

> If you mean "less typing", this is already pretty concise (even without the
> chaining shortcut).

Nope, raw performance gains. As shown in the video. 

> > Or other similar code. We also saw evidence that developers assume that class
> > lists retain order:
> > 
> > if(elem.classList === "x y") {
> >    .... 
> > } 
> > 
> > This could lead to confusion when using DOMTokenList toggle(), as toggle
> > destroys the ordering of CSS class list names of an attribute.  
> > 
> > Given the above, I'll like to request we add swap() method to DOMTokenList().
> > It would swap one token for another while retaining item order.
> 
> This seems to be increasing the surface area of the API just to make it easier
> to get bad practices to work.  It encourages people to write more code that
> depends on the order of the class list--which should be discouraged--instead of
> fixing the real problem (use classList.contains(), not string comparison).

To do the queries that you suggest is more annoying. You would need to do the following:

if(elem.classList.contains("x") && elem.classList.contains("y"))

When what you really want is:
if(elem.classList.contains("x", "y", "z")); 

or 
(elem.classList.contains(["x", "y", "z"]))

Getting off topic.
Comment 3 Glenn Maynard 2012-07-07 17:06:31 UTC
(In reply to comment #2)
> There is potentially a significant performance gain. Check this out: 
> http://youtu.be/hZJacl2VkKo 

Do you have a simple benchmark?  I don't really want to watch a video.

And, more importantly, why can't this be optimized by browsers without adding API?  I'd expect modern browsers to apply the effects of classes lazily, so making multiple changes is roughly free.  That's similar to how changing DOM properties usually doesn't trigger layout immediately, so making lots of changes to an object isn't expensive.  Does something make classes a harder problem than that?

FWIW, I rarely actually swap classes like this.  Toggling a class and using :not(.class) usually works well, and tends to give simpler code, too.

> To do the queries that you suggest is more annoying. You would need to do the
> following:
> 
> if(elem.classList.contains("x") && elem.classList.contains("y"))
> 
> When what you really want is:
> if(elem.classList.contains("x", "y", "z")); 
> 
> or 
> (elem.classList.contains(["x", "y", "z"]))
> 
> Getting off topic.

Sure, adding features that encourage not depending on class order is probably a good thing.  I'm just saying that adding features that do the opposite seems like a bad approach--it's just giving people workarounds so they can continue doing things in brittle ways.  (This is orthogonal to performance, of course.)
Comment 4 Marcos Caceres 2012-07-07 23:24:15 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > There is potentially a significant performance gain. Check this out: 
> > http://youtu.be/hZJacl2VkKo 
> 
> Do you have a simple benchmark?  I don't really want to watch a video.

Oh, I see. You are just here to troll. Very well: I don't have time to read the rest of your comments then.
Comment 5 Glenn Maynard 2012-07-08 00:41:21 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > There is potentially a significant performance gain. Check this out: 
> > > http://youtu.be/hZJacl2VkKo 
> > 
> > Do you have a simple benchmark?  I don't really want to watch a video.
> 
> Oh, I see. You are just here to troll. Very well: I don't have time to read the
> rest of your comments then.

Excuse me?  Please be civil, and please don't make performance assertions if you're not willing to show benchmarks.

All of my concerns stand; if you choose not to address them, that's up to you.
Comment 6 Marcos Caceres 2012-07-08 11:59:41 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > There is potentially a significant performance gain. Check this out: 
> > > > http://youtu.be/hZJacl2VkKo 
> > > 
> > > Do you have a simple benchmark?  I don't really want to watch a video.
> > 
> > Oh, I see. You are just here to troll. Very well: I don't have time to read the
> > rest of your comments then.
> 
> Excuse me?  Please be civil, and please don't make performance assertions if
> you're not willing to show benchmarks.

I did present the benchmarks. They are in the video. I'm not going to go and rewrite what is in the video for your benefit. Once you watch the video, we can continue the discussion - but if your not civil enough to do that, then I'm not interested in wasting time discussing things with you.
Comment 7 Anne 2012-07-08 12:07:17 UTC
The video might demonstrate that manipulating classList instead of className is more efficient (would have to see the code running in several browsers to be sure). However once you have classList you can do swap using add() and remove(). That is probably about as efficient as a replace() method would be. And I agree with Glenn that replace() is kind of a weird semantic for classes. (Although I will concede that many people seem to use a replace pattern.)
Comment 8 Glenn Maynard 2012-07-08 16:51:28 UTC
(In reply to comment #6)
> I did present the benchmarks. They are in the video. I'm not going to go and
> rewrite what is in the video for your benefit. Once you watch the video, we can
> continue the discussion - but if your not civil enough to do that, then I'm not
> interested in wasting time discussing things with you.

A video is not a benchmark.  Benchmarks must be independantly runnable and reproducable; a recording of a benchmark is just a demo.  (Discussions can stay in text, where it's readable and quotable, unless there's a specific reason video is necessary.)

You're free to leave objections unaddressed for any manufactured reason you like, but you do so at your cost only.
Comment 9 Marcos Caceres 2012-07-08 23:47:12 UTC
(In reply to comment #7)
> The video might demonstrate that manipulating classList instead of className is
> more efficient (would have to see the code running in several browsers to be
> sure). 

Agree. Need more testing here. 

>However once you have classList you can do swap using add() and
> remove(). That is probably about as efficient as a replace() method would be.
> And I agree with Glenn that replace() is kind of a weird semantic for classes.
> (Although I will concede that many people seem to use a replace pattern.)

The problem with add and remove is that it changes the ordering of the classes. This does not matter on the CSS cascade side, but it matters on the javascript side because devs tend to string literal checks:

if(foo.className == "a b"){ ... }
Comment 10 Marcos Caceres 2012-07-08 23:48:11 UTC
(In reply to comment #8)
> (In reply to comment #6)
> > I did present the benchmarks. They are in the video. I'm not going to go and
> > rewrite what is in the video for your benefit. Once you watch the video, we can
> > continue the discussion - but if your not civil enough to do that, then I'm not
> > interested in wasting time discussing things with you.
> 
> A video is not a benchmark.  Benchmarks must be independantly runnable and
> reproducable; a recording of a benchmark is just a demo.  (Discussions can stay
> in text, where it's readable and quotable, unless there's a specific reason
> video is necessary.)
> 
> You're free to leave objections unaddressed for any manufactured reason you
> like, but you do so at your cost only.

Did you watch the video yet?
Comment 11 Glenn Maynard 2012-07-09 00:13:57 UTC
(In reply to comment #10)
> Did you watch the video yet?

If you're not even going to read what you're replying to, then please don't waste the time of everyone on the CC list.  (In order to not do the same myself, I won't be replying to this bug further unless there's something relevant to reply to.)
Comment 12 Anne 2012-11-09 14:41:02 UTC
Classes are flags, using them in another way is inappropriate and results in less elegant code.