Last Comment Bug 814090 - [classList] implements the new toggle spec
: [classList] implements the new toggle spec
Status: RESOLVED FIXED
: doc-bug-filed
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla24
Assigned To: Julien Wajsberg [:julienw]
:
: Andrew Overholt [:overholt]
Mentors:
http://dom.spec.whatwg.org/#dom-domto...
Depends on: 885279
Blocks: 814014
  Show dependency treegraph
 
Reported: 2012-11-21 11:00 PST by Julien Wajsberg [:julienw]
Modified: 2013-08-21 23:11 PDT (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (8.70 KB, patch)
2013-06-10 04:36 PDT, Julien Wajsberg [:julienw]
bzbarsky: review+
Details | Diff | Splinter Review
patch v2 (8.61 KB, patch)
2013-06-14 14:24 PDT, Julien Wajsberg [:julienw]
felash: review+
Details | Diff | Splinter Review

Description User image Julien Wajsberg [:julienw] 2012-11-21 11:00:32 PST
The new specification for classList.toggle added a second argument which can force the toggle. This is useful and can lead to clearer JS code.
Comment 1 User image Julien Wajsberg [:julienw] 2013-04-24 08:25:51 PDT
This doesn't need variadic idl support, so could we add this support ? This is so useful when developing JS using the DOM.

Developing on B2G without extra framework there is probably one situation each day when I wish I had this implemented.
Comment 3 User image Julien Wajsberg [:julienw] 2013-06-10 04:36:36 PDT
Created attachment 760342 [details] [diff] [review]
patch v1

I still don't exactly get how to push to try (it finds no change when I push) but I added some tests and they all passes locally.
Comment 4 User image Boris Zbarsky [:bz] (still a bit busy) 2013-06-10 11:50:08 PDT
Before I start looking at the code... what is the point of this argument?  It looks to me like:  .toggle('foo') will add 'foo' to the list if not there, and remove it otherwise.  But .toggle('foo', true) will act just like .add('foo') and .toggle('foo', false) will act just like .remove('foo'), except for having a non-void return value.

Is my reading of the spec correct?  If so, why is this a useful thing for a method called toggle() to do?  If I'm misreading it, what am I missing?
Comment 5 User image :Ms2ger (⌚ UTC+1/+2) 2013-06-10 11:51:58 PDT
AIUI, you're reading it correctly. It's meant to replace classList[aBooleanArg ? "add" : "remove"](), I think.
Comment 6 User image Julien Wajsberg [:julienw] 2013-06-10 11:57:27 PDT
This is especially useful when you have a boolean that results from a bool calculation. This is more a sugar syntax, because this allows nothing that can't be done without it.

Compare for example (a simplified code taken from the Sms app in Gaia) :

  if (recipientCount > 0) {
    this.contactPickButton.classList.add('disabled');
  } else {
    this.contactPickButton.classList.remove('disabled');
  }

with this code, this can be simplidied like this :

  this.contactPickButton.classList.toggle('disabled', recipientCount > 0);
Comment 7 User image Boris Zbarsky [:bz] (still a bit busy) 2013-06-10 12:23:49 PDT
Hmm...  ok, I see.  Seems like slightly odd API, but I can't come up with a better one right this second.  And the spec is ... unreadable.  :(  And of course toggle('foo', true) is not specced to do the same thing as add('foo'), but we'll worry about that when tests get written for the spec.  :(
Comment 8 User image Boris Zbarsky [:bz] (still a bit busy) 2013-06-10 12:35:40 PDT
Comment on attachment 760342 [details] [diff] [review]
patch v1

>+++ b/content/base/src/nsDOMTokenList.cpp
>+                       const mozilla::dom::Optional<bool>& optionalIsForce,

Please drop the "mozilla::dom::" bit.  See the "using namespace" near the top of this file.

Also, the argument should probably be called "aForce".

>+  const bool hasForceArgument = optionalIsForce.WasPassed();
>+  const bool isForce = optionalIsForce.Value();

Doing Value() if !WasPassed() should trigger a fatal assert.  Was the relevant codepath just not tested?

I think you want something more like:

  const bool forceOn = aForce.WasPassed() && aForce.Value();
  const bool forceOff = aForce.WasPassed() && !aForce.Value();

>+  bool isPresent = (attr && attr->Contains(aToken));

Please remove the extra parens.

>+    if ((hasForceArgument && !isForce) || (!hasForceArgument)) {

And then this would become:

  if (!forceOn) {

>+    if ((hasForceArgument && isForce) || (!hasForceArgument)) {

And this would become:

  if (!forceOff) {

>+++ b/content/base/src/nsDOMTokenList.h
>+              const mozilla::dom::Optional<bool>& force,

aForce.

r=me with the above fixed.
Comment 9 User image Julien Wajsberg [:julienw] 2013-06-10 13:12:52 PDT
(In reply to Boris Zbarsky (:bz) from comment #8)
> Comment on attachment 760342 [details] [diff] [review]
> patch v1
> 
> 
> >+  const bool hasForceArgument = optionalIsForce.WasPassed();
> >+  const bool isForce = optionalIsForce.Value();
> 
> Doing Value() if !WasPassed() should trigger a fatal assert.  Was the
> relevant codepath just not tested?

strangely, I'm quite sure it is in the mochitests... The tests were existing prior this patch. I also briefly tested using the Web Console.

How is it possible then ?
Comment 10 User image Boris Zbarsky [:bz] (still a bit busy) 2013-06-10 13:14:20 PDT
Did you test an opt build or debug build?
Comment 11 User image Julien Wajsberg [:julienw] 2013-06-10 13:25:30 PDT
I had no .mozconfig so I think it was an opt build. I'm building again with the debug option. Thanks a lot.
Comment 12 User image Julien Wajsberg [:julienw] 2013-06-10 14:22:32 PDT
I get the assert failure while running the mochitests with a debug build.
Comment 13 User image Julien Wajsberg [:julienw] 2013-06-14 14:24:23 PDT
Created attachment 762917 [details] [diff] [review]
patch v2

carrying r=bzbarsky

try is https://tbpl.mozilla.org/?tree=Try&rev=85804cbf8361
Comment 14 User image Julien Wajsberg [:julienw] 2013-06-16 02:23:35 PDT
This looks good to me.

Ryan, would you double check if try looks good and do the check in ? Thanks !
Comment 15 User image Phil Ringnalda (:philor) 2013-06-16 12:43:03 PDT
(In reply to Julien Wajsberg [:julienw] from comment #3)
> I added some tests and they all passes locally.

Looks like your added tests didn't actually make it into the patch, though.
Comment 16 User image Julien Wajsberg [:julienw] 2013-06-16 23:18:15 PDT
Phil, I do see them in the patch, don't you see the changes to `test_classList.html` ?
Comment 17 User image Phil Ringnalda (:philor) 2013-06-16 23:45:41 PDT
Reading is hard.
Comment 18 User image Ryan VanderMeulen [:RyanVM] 2013-06-17 13:08:48 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/dccd693173c9
Comment 19 User image Ed Morley [:emorley] 2013-06-18 04:08:31 PDT
https://hg.mozilla.org/mozilla-central/rev/dccd693173c9
Comment 20 User image Jeremie Patonnier :Jeremie 2013-06-20 03:28:37 PDT
Follow up for documentation: https://bugzilla.mozilla.org/show_bug.cgi?id=885279
Comment 21 User image Anne (:annevk) 2013-06-24 02:39:15 PDT
The specification has been updated with clearer wording: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22321

Note You need to log in before you can comment on or make changes to this bug.