Add support for Element.toggleAttribute()

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: jkt, Assigned: jkt)

Tracking

({dev-doc-complete})

63 Branch
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment)

Element.toggleAttribute provides a simple boolean interface the same as classList.toggle for Element.

See https://github.com/whatwg/dom/issues/461

Other browsers are willing to implement.
FYI, phabricator requests don't seem to end up to my bugzilla review queue, so I won't remember them.
One needs to explicitly ask review in bugzilla.
Comment on attachment 8986269 [details]
Bug 1469592 - Add support for Element.toggleAttribute. r?smaug r?bz!

Ah that is annoying, I didn't spot that... it's my first time. Thanks!
Attachment #8986269 - Flags: review?(bugs)
Comment on attachment 8986269 [details]
Bug 1469592 - Add support for Element.toggleAttribute. r?smaug r?bz!

Olli Pettay [:smaug] has approved the revision.

https://phabricator.services.mozilla.com/D1710
Attachment #8986269 - Flags: review+
Comment on attachment 8986269 [details]
Bug 1469592 - Add support for Element.toggleAttribute. r?smaug r?bz!

(and phabricator's flag handling is so broken that one needs to explicitly clear one review? flag)
Attachment #8986269 - Flags: review?(bugs)
Comment on attachment 8986269 [details]
Bug 1469592 - Add support for Element.toggleAttribute. r?smaug r?bz!

:Annevk could you check over the test in this patch and provide any feedback please.

Thanks.
Attachment #8986269 - Flags: feedback?(annevk)
Keywords: dev-doc-needed
Comment on attachment 8986269 [details]
Bug 1469592 - Add support for Element.toggleAttribute. r?smaug r?bz!

Thanks!
Attachment #8986269 - Flags: feedback?(annevk) → feedback+
I moved the wpt test code to: https://github.com/web-platform-tests/wpt/pull/11603 due to the code freeze and Servo has already merged etc.
This could use an intent to ship, by the way.
Yup, I should have filed that. I wrote it out will send now :)
Attachment #8986269 - Attachment description: Bug 1469592 - Add support for Element.toggleAttribute. r?smaug → Bug 1469592 - Add support for Element.toggleAttribute. r?smaug r?bz
Attachment #8986269 - Attachment description: Bug 1469592 - Add support for Element.toggleAttribute. r?smaug r?bz → Bug 1469592 - Add support for Element.toggleAttribute. r?smaug r?bz!
Related to this change also: https://github.com/whatwg/dom/pull/663
Flags: needinfo?(bzbarsky)
Comment on attachment 8986269 [details]
Bug 1469592 - Add support for Element.toggleAttribute. r?smaug r?bz!

Boris Zbarsky [:bz] (no decent commit message means r-) has approved the revision.

https://phabricator.services.mozilla.com/D1710
Attachment #8986269 - Flags: review+
Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/72cacb766a6f
Add support for Element.toggleAttribute. r=bzbarsky,smaug!
I'm not sure what information you're asking for....
Just a heads up on the spec change that you asked for :)
https://hg.mozilla.org/mozilla-central/rev/72cacb766a6f
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Is this ready to use in chrome code? Should we attempt to do a search and replace on the tree?

I guess the pattern to look for is:
  if (foo.hasAttribute("bar"))
    foo.removeAttribute("bar");
  else
    foo.setAttribute("bar", "true");
eg. https://searchfox.org/mozilla-central/rev/403038737ba75af3842ba6b43b6e2fb47eb06609/browser/base/content/tabbrowser.js#4878-4882

Are there other patterns to look for?
:johannh already asked me this and I think this should be safe to use (it's likely good first bug territory also).

The other patterns could be:

- Find this: el.setAttribute("checked", "");
- Replace with this: el.toggleAttribute("checked", true);

- Find this: el.removeAttribute("checked");
- Replace with this: el.toggleAttribute("checked", false);

Potentially another shortcut is removing boolean statements like this (if it exists):

  if (el.toggleAttribute("checked")) {
    other.removeAttribute("checked");
    el.removeAttribute("checked");
  } else {
    other.setAttribute("checked", "true");
    el.setAttribute("checked", "true");
  }

To be:
  other.toggleAttribute("checked", el.toggleAttribute("checked"));

There might be some other patterns also that I haven't thought about too.
(In reply to Jonathan Kingston [:jkt] from comment #19)
> :johannh already asked me this and I think this should be safe to use (it's
> likely good first bug territory also).

I was mostly wondering if this is something we could automatically rewrite with a script.

> The other patterns could be:
> 
> - Find this: el.setAttribute("checked", "");
> - Replace with this: el.toggleAttribute("checked", true);
> 
> - Find this: el.removeAttribute("checked");
> - Replace with this: el.toggleAttribute("checked", false);

Is there a benefit to using toggleAttribute in these 2 cases where it's longer?

> Potentially another shortcut is removing boolean statements like this (if it
> exists):
> 
>   if (el.toggleAttribute("checked")) {
>     other.removeAttribute("checked");
>     el.removeAttribute("checked");
>   } else {
>     other.setAttribute("checked", "true");
>     el.setAttribute("checked", "true");
>   }
> 
> To be:
>   other.toggleAttribute("checked", el.toggleAttribute("checked"));

In this case it's shorter, but I don't think I would want to do this automatically as whether it improves or degrades readability of the code could be debated. This pattern would be in the good first bug territory I guess :-).
> I was mostly wondering if this is something we could automatically rewrite with a script.

Ah your case is certainly the only one I would auto rewrite with scripting (this isn't something I do so would be interested in how you do this :) ).

Also the ternary equiv might be around too:

foo.hasAttribute("bar") ? foo.removeAttribute("bar") : foo.setAttribute("bar", "true");

> Is there a benefit to using toggleAttribute in these 2 cases where it's longer?

- I find the string arguments passed to setAttribute confusing
  - We don't care about their value
  - We pass confusing arguments like "true"
  - I have erroneously written code that checks their value when hasAttribute is fine
- When there is an if statement just surrounding this statement it can be removed:
  - if (x) { el.setAttribute("checked", ""); }
  - el.toggleAttribute("checked", x);
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11790 for changes under testing/web-platform/tests
Updated https://developer.mozilla.org/en-US/docs/Web/API/Element/ to add Element.toggleAttribute.
https://developer.mozilla.org/en-US/docs/Web/API/Element/toggleAttribute already existed.


Web/API/Element is using old browser compatibility. JSON compat file needs to be created.
Estelle, the documentation at https://developer.mozilla.org/en-US/docs/Web/API/Element/toggleAttribute says the "force" argument is a "Boolean" in the sense of https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Boolean but that's not correct.  It's a boolean, not a Boolean object...

I've now seen this issue come up multiple times in MDN docs.  Is there something automatically linking to Boolean that we can fix to not do that?  Boolean pretty much never appears anywhere and definitely never appears in web APIs.
Flags: needinfo?(estelle)
I removed the xref, which I hope is not automatic, in the doc. Raising the issue of Boolean with the team, and will create another ticket on the topic if necessary.
Flags: needinfo?(estelle)
(In reply to Estelle Weyl from comment #26)
> I removed the xref, which I hope is not automatic, in the doc. Raising the
> issue of Boolean with the team, and will create another ticket on the topic
> if necessary.

This came up in the past because of my misunderstanding of what boolean objects are versus boolean values. We'll try to weed these out in future. The domxref values are manually added, not automatic. We just need to not add them.
Version: 62 Branch → 63 Branch
You need to log in before you can comment on or make changes to this bug.