1469592 - Add support for Element.toggleAttribute()

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Jonathan Kingston [:jkt] he/him]

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.

Comment 1

[Phabricator Automation]

Attachment: Bug 1469592 - Add support for Element.toggleAttribute. r?smaug r?bz!

MozReview-Commit-ID: KkDLMlQgESs

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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 3

[Jonathan Kingston [:jkt] he/him]

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!

Comment 4

[Phabricator Automation]

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

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 6

[Jonathan Kingston [:jkt] he/him]

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.

Comment 7

[Anne (:annevk)]

Comment on attachment 8986269 [details]
Bug 1469592 - Add support for Element.toggleAttribute. r?smaug r?bz!

Thanks!

Comment 8

[Jonathan Kingston [:jkt] he/him]

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

This could use an intent to ship, by the way.

Comment 10

[Jonathan Kingston [:jkt] he/him]

Yup, I should have filed that. I wrote it out will send now :)

Comment 11

[Jonathan Kingston [:jkt] he/him]

Related to this change also: https://github.com/whatwg/dom/pull/663

Comment 12

[Phabricator Automation]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/72cacb766a6f
Add support for Element.toggleAttribute. r=bzbarsky,smaug!

Comment 14

[Boris Zbarsky [:bzbarsky]]

I'm not sure what information you're asking for....

Comment 15

[Jonathan Kingston [:jkt] he/him]

Just a heads up on the spec change that you asked for :)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Ah.  Yes, thank you.  ;)

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/72cacb766a6f

Comment 18

[Florian Quèze [:florian]]

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?

Comment 19

[Jonathan Kingston [:jkt] he/him]

: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.

Comment 20

[Florian Quèze [:florian]]

(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 :-).

Comment 21

[Jonathan Kingston [:jkt] he/him]

> 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);

Comment 22

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11790 for changes under testing/web-platform/tests

Comment 23

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 24

[Estelle Weyl]

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.

Comment 25

[Boris Zbarsky [:bzbarsky]]

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.

Comment 26

[Estelle Weyl]

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.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Thanks!

Comment 28

[Chris Mills [:cmills]]

(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.