Bug 148582 - classList.toggle(name, force) treats undefined `force` argument as false
Summary: classList.toggle(name, force) treats undefined `force` argument as false
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-08-28 11:25 PDT by Michał Gołębiowski-Owczarek
Modified: 2015-09-18 10:43 PDT (History)
9 users (show)

See Also:


Attachments
Patch (20.47 KB, patch)
2015-09-17 19:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (20.53 KB, patch)
2015-09-17 22:19 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (20.47 KB, patch)
2015-09-18 08:08 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michał Gołębiowski-Owczarek 2015-08-28 11:25:51 PDT
Test case:

var div = document.createElement('div');
div.classList.toggle('a', undefined); // <div></div>

`toggle` should add the class and not treat `undefined` as false.

Chrome & Edge also share this bug. Firefox implements it correctly.

Chrome bug report: https://code.google.com/p/chromium/issues/detail?id=489665
Comment 1 Michał Gołębiowski-Owczarek 2015-08-28 11:40:48 PDT
Edge bug report: https://connect.microsoft.com/IE/feedbackdetail/view/1725606/
Comment 2 Chris Dumez 2015-08-28 13:33:15 PDT
Yes, this is not specific to this API but a general bug in our JS bindings code. undefined gets converted to 0 / false instead of being treated as no argument when the argument is optional. I will try and address this in the near future.
Comment 3 Michał Gołębiowski-Owczarek 2015-08-28 13:46:44 PDT
(In reply to comment #2)
> I will try and address this in the near future.

Great to hear that!

This is very useful when you need to proxy the method. I'm developing an experimental jQuery plugin that replaces its class manipulation code with one utilizing classList and I had to write something like:

if (force !== undefined) {
    this.classList.toggle(clazz, force);
} else {
    this.classList.toggle(clazz);
}

instead of just passing the value. This is the full source:
https://github.com/mzgol/jquery.classList/blob/3a7018254bf0545c54f6ff7d73eb21aadcfb8ec9/src/jquery.class_list.js#L108-L119
Comment 4 Chris Dumez 2015-09-02 14:22:07 PDT
rdar://problem/22545600
Comment 5 Chris Dumez 2015-09-17 19:48:52 PDT
Created attachment 261474 [details]
Patch
Comment 6 Chris Dumez 2015-09-17 22:19:18 PDT
Created attachment 261490 [details]
Patch
Comment 7 Ryosuke Niwa 2015-09-18 00:09:36 PDT
Comment on attachment 261490 [details]
Patch

Does this match the behavior of other browsers?
Comment 8 Chris Dumez 2015-09-18 07:57:01 PDT
(In reply to comment #7)
> Comment on attachment 261490 [details]
> Patch
> 
> Does this match the behavior of other browsers?

It matches Firefox and the specification.
Comment 9 Chris Dumez 2015-09-18 08:08:55 PDT
Created attachment 261503 [details]
Patch
Comment 10 WebKit Commit Bot 2015-09-18 09:53:27 PDT
Comment on attachment 261503 [details]
Patch

Clearing flags on attachment: 261503

Committed r189969: <http://trac.webkit.org/changeset/189969>
Comment 11 WebKit Commit Bot 2015-09-18 09:53:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Michał Gołębiowski-Owczarek 2015-09-18 10:04:43 PDT
Thanks! \o/

Do you also plan to fix the more general issue about WebIDL bindings mistreating undefined?
Comment 13 Chris Dumez 2015-09-18 10:43:23 PDT
(In reply to comment #12)
> Thanks! \o/
> 
> Do you also plan to fix the more general issue about WebIDL bindings
> mistreating undefined?

Yes, I am fixing a lot of those via:
https://bugs.webkit.org/show_bug.cgi?id=149263
https://bugs.webkit.org/show_bug.cgi?id=149331

If you know about more cases, please let me know.