Closed Bug 1597046 Opened 5 months ago Closed 1 month ago

Make toggle() function take second parameter to force state

Categories

(DevTools :: Inspector, enhancement)

enhancement
Not set

Tracking

(firefox75 fixed)

RESOLVED FIXED
Firefox 75
Tracking Status
firefox75 --- fixed

People

(Reporter: sebo, Assigned: Tanny_m, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(4 files)

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

Hi Sebastian,

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

Thank you

Flags: needinfo?(sebastianzartner)

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

Flags: needinfo?(sebastianzartner)

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

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

Assignee: nobody → tanmaya.march

Hi Gabriel and Sebastian,

Requesting review as I have submitted the patch.

Thank you

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

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

Flags: needinfo?(sebastianzartner)

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

Flags: needinfo?(sebastianzartner)

Hi Gabriel,

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

Thanks,

Tanmaya

Flags: needinfo?(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.

Flags: needinfo?(gl)

(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

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

Flags: needinfo?(pbrosset)

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

Flags: needinfo?(pbrosset)
Status: NEW → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 75
See Also: → 1619109
You need to log in before you can comment on or make changes to this bug.