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

DOMTokenList#toggle: don't treat everything non-undefined as true #64

Closed
mgol opened this issue Aug 28, 2015 · 8 comments

Comments

@mgol
Copy link

commented Aug 28, 2015

According to the 3rd & 4th step at https://dom.spec.whatwg.org/#dom-domtokenlist-toggle, DOMTokenList#toggle treats every non-false value of the second argument as true. In particular:

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

So, according to the spec, these should be div.classList.contains('a') results after the first & second toggle invocation:

1: true
2: true

This is because force is passed and is not false so the first sub-step of the 3rd step doesn't apply.

Now, these are the results in popular browsers:
Chrome 44, Safari 8, Edge:

1: false
2: false

Firefox 40, IE 11 (edit: IE just ignores the second parameter to toggle in all cases):

1: false
2: true

Now, if you change undefined to null in both those cases, all browsers return:

1: false
2: false

This indicates all browsers cast the force value to a boolean and only some of them make an exception for undefined, treating it as an argument not passed.

IMO the Firefox & IE 11 behavior is the most sensible one. This allows to proxy the toggle method without checking the number of arguments passed:

function proxyClassListToggle(node, value, force) {
    // I don't want to do this
    if (typeof force === 'boolean') {
        node.classList.toggle(value, force);
    } else {
        node.classList.toggle(value);
    }
}
proxyClassListToggle(node, 'a');

In any case, the currently specced behavior is not matching reality and is surprising (I'd certainly not expect undefined to be treated as true).

@domenic

This comment has been minimized.

Copy link
Member

commented Aug 28, 2015

I found this to be a very weird part of the spec as well. I agree with you on the best solution being:

  • undefined, including from omitted argument => toggle last state (default behavior)
  • any non-undefined falsy value => turn off
  • any truthy value => turn on.
@bzbarsky

This comment has been minimized.

Copy link

commented Aug 28, 2015

@mzgol The spec IDL says:

boolean toggle(DOMString token, optional boolean force);

which means that passing undefined is the same as passing no second argument at all (your observed Firefox and IE behavior). That's precisely for the proxying reason you describe.

The part that you link to at https://dom.spec.whatwg.org/#dom-domtokenlist-toggle will have force as "not passed" if undefined is passed to the method. That's all handled by the IDL layer.

@bzbarsky

This comment has been minimized.

Copy link

commented Aug 28, 2015

Oh, and to be clear what IDL does for an optional boolean argument is the following:

undefined -> "not passed"
Any other value "arg" -> ToBoolean(arg)
@domenic

This comment has been minimized.

Copy link
Member

commented Aug 28, 2015

Ah goodness, Web IDL. Thanks @bzbarsky.

Of course, now that Firefox is the only browser following the spec we might have a problem... especially as Edge seems to have purposefully regressed to match Chrome/Safari :(

@bzbarsky

This comment has been minimized.

Copy link

commented Aug 28, 2015

Edge is basically reverse-engineering whatever Chrome does, bugs and all. Can't say I blame them...

That said, the fact that Chrome mishandles the way undefined works when passed for an optional argument is a longstanding problem in Chrome's bindings. We changed it to the current setup in IDL years ago to support the forwarding/proxying use cases... and then Chrome just didn't bother to implement that change. Doing that in Chrome's bindings would help more than anything else here.

@mgol

This comment has been minimized.

Copy link
Author

commented Aug 28, 2015

@bzbarsky Thanks for clarifying, I'm not that familiar with Web IDL.

@domenic

especially as Edge seems to have purposefully regressed to match Chrome/Safari

Actually, I should've included IE in this matrix as it just doesn't implement the second parameter to the toggle method so it obviously ignores undefined. So they haven't regressed, they've just implemented a non-compliant behavior.

BTW, I found the Chrome report about that: https://crbug.com/489665. I've reported it to WebKit: https://bugs.webkit.org/show_bug.cgi?id=148582 and Edge: https://connect.microsoft.com/IE/feedbackdetail/view/1725606/ as well.

@mgol

This comment has been minimized.

Copy link
Author

commented Aug 28, 2015

I guess we can close the issue since the spec is OK and non-Firefox browsers should update their behavior? Or do we want to first check if this is mobile-web-compatible or something?

(BTW, after today I can recognize each of Chrome, Firefox, Safari, Edge & IE 11 just by checking results of various classList/className-related tests...)

@annevk

This comment has been minimized.

Copy link
Member

commented Aug 29, 2015

@mzgol hah, yes, if you test a given feature enough, you can use it to detect all browsers, and often different versions of browsers too!

I think I'm closing this and if a browser finds out this is not compatible they can file a new focused issue.

Thank you all for your efforts.

@annevk annevk closed this Aug 29, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.