1597046 - Make toggle() function take second parameter to force state

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement)

Description

[Sebastian Zartner [:sebo]]

In bug 1152321 comment 10 Matteo asked me to change the classList.toggle() function to take a second parameter to force the toggle state like it's done in the normal DOM API.

Though I think it's better to create a separate issue to improve this as it's unrelated to the changes in that bug.

The code for it is here:
https://searchfox.org/mozilla-central/rev/b58e44b74ef2b4a44bdfb4140c2565ac852504be/devtools/server/actors/highlighters/utils/markup.js#79

It's a little change, which I think would be good as a first bug.

Sebastian

Comment 1

[Tanmaya [:Tanny_m]]

Hi Sebastian,

I would like to contribute to this bug if I can get some guidance as it to what is required.

Thank you

Comment 2

[Sebastian Zartner [:sebo]]

Hi Tanmaya,

what needs to be done is to add a second parameter force to the toggle() function in markup.js linked to in the description.
This parameter is meant to work like in the DOM API DOMTokenList.toggle(). I.e. that parameter is an optional boolean that, if included, enforces the addition or removal of a CSS class by calling this.add(token) or this.remove(token). If that parameter is not provided when calling the function, it should toggle the class like it's doing now.

Also, it would be good if you created a test case for it. One that tests parts of the markup.js is here:
https://searchfox.org/mozilla-central/rev/652014ca1183c56bc5f04daf01af180d4e50a91c/devtools/server/tests/browser/browser_canvasframe_helper_01.js

Hope that makes it clear what needs to be done. If you need any further info, let me know.

Sebastian

Comment 3

[Rick Hallett]

@sebo Unless Tanmaya has already made progress with this, I would like to look into this. I have started attempting a fix and will update.

Comment 4

[Tanmaya [:Tanny_m]]

Hi Rick,

Thanks for asking, I have already attempted a fix and am only yet to submit the patch. So I would want to continue looking into this if that is okay :)

Thanks,

Tanmaya

Comment 5

[Tanmaya [:Tanny_m]]

Attachment: Bug 1597046 - Implementing toggle() function to take second boolean parameter to force state. r=gl

Comment 6

[Tanmaya [:Tanny_m]]

Hi Gabriel and Sebastian,

Requesting review as I have submitted the patch.

Thank you

Comment 7

[Tanmaya [:Tanny_m]]

Attachment: Bug 1597046 - addressing the revision. r=gl,sebo

Comment 8

[Tanmaya [:Tanny_m]]

Attachment: Bug 1597046 - test cases for implementing toggle(token,force). r=gl,sebo

Comment 9

[Rick Hallett]

Hi Tanmaya, but all means - good luck with the fix!

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

@sebo, is there a use case where we would need to use this force argument? While I see the argument for conforming to normal DOM API, I would want to avoid adding unused code.

Comment 11

[Sebastian Zartner [:sebo]]

At least the patch in bug 1152321 could profit from it. (That's why Matteo initially requested it).
And I am sure there are other places that could use it, but I'd need to search for them.

Sebastian

Comment 12

[Tanmaya [:Tanny_m]]

Attachment: Bug 1597046 - addressing the revision. r=gl,sebo

Comment 13

[Tanmaya [:Tanny_m]]

Hi Gabriel,

Can you please review and approve the changes? :-)

Thanks,

Tanmaya

Comment 14

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Tanmaya [:Tanny_m] from comment #13)

Hi Gabriel,

Can you please review and approve the changes? :-)

Thanks,

Tanmaya

You aren't actually blocked on my review since it is already approved by sebo. Personally, I don't think I am satisfied with the argument on needing to add this change yet without any usage of it. The simple reasoning for this is that we don't want to add any code that isn't used. I can see an argument made when Bug 1152321 is closer to landing, but until then, I think we should hold off from landing.

Comment 15

[Sebastian Zartner [:sebo]]

(In reply to Gabriel [:gl] (ΦωΦ) from comment #14)

I can see an argument made when Bug 1152321 is closer to landing, but until then, I think we should hold off from landing.

I am actively working on bug 1152321 (hopefully finishing it this weekend) and Patrick asked again for this function in his review. So we should land this soon. I don't have enough rights to land this changes, though.

Sebastian

Comment 16

[Sebastian Zartner [:sebo]]

Patrick, this change here was initially meant for the changes of bug 1152321. They could land independently, but I'm wondering whether this should land first and the patches of bug 1152321 changed to make use of them. Since you already asked for it, I'll let you decide.
More info to this is in the description above.

Sebastian

Comment 17

[Patrick Brosset <:pbro>]

I'm landing this now. Thanks for the ping. Let's use it in bug 1152321!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c25ed6f634e7
addressing the revision. r=sebo

Comment 19

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/c25ed6f634e7