1476873 - Use Element.toggleAttribute instead of our custom handling for setAttribute vs. removeAttribute

Status: RESOLVED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[Matthew N. [:MattN]]

Assuming that the logic is what we want and won't cause regressions…

https://dxr.mozilla.org/mozilla-central/search?q=regexp%3A%22removeAttribute%5C((key%7Cname)%5C)%22+path%3Apayments&redirect=false

Comment 1

[Diego Pino]

Attachment: Bug-1476873-Use-Element.toggleAttribute-instead-of-c.patch

Comment 2

[Diego Pino]

I run the tests. It doesn't seem to be regressions https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3cf25219244e71423a4783c4d14dbcc5357a0b5

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 9029594 [details] [diff] [review]
Bug-1476873-Use-Element.toggleAttribute-instead-of-c.patch

Review of attachment 9029594 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I think when I filed this I thought toggleAttribute allowed specifying the value to toggle too, not just the name. Let's only switch to toggleAttribute for boolean attributes like the ones in *-form.js and @changes-prevented then revert the rest.

I don't think the
```js
if (el.toggleAttribute(…)) {
  el.setAttribute(…);
}
```
pattern is common enough that I think it hinders readability.

::: browser/components/payments/res/containers/payment-dialog.js
@@ +415,5 @@
>      for (let page of this._mainContainer.querySelectorAll(":scope > .page")) {
>        page.hidden = state.page.id != page.id;
>      }
>  
> +    this.toggleAttribute("changes-prevented", state.changesPrevented);

This is a good replacement

::: browser/components/payments/res/mixins/ObservedPropertiesMixin.js
@@ +33,5 @@
>            get() {
>              return this.getAttribute(name);
>            },
>            set(value) {
> +            if (this.toggleAttribute(name, !!value)) {

This is a behaviour change for "" btw.

Comment 4

[Diego Pino]

Attachment: Bug-1476873-Use-Element.toggleAttribute-instead-of-c.patch

I agree the "if toggle/set" pattern is less readable. Updated patch with suggested changes.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 9031672 [details] [diff] [review]
Bug-1476873-Use-Element.toggleAttribute-instead-of-c.patch

Review of attachment 9031672 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/099c54d22608
Use Element.toggleAttribute instead of custom handling for setAttribute vs. removeAttribute. r=Mattn

Comment 7

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/099c54d22608