1742793 - Eyedropper spawns multiple eyedroppers instead of destroying the existing one

Status: NEW

(DevTools :: Inspector, defect, P3)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me) PTO July 8 - 26]

This is a follow up for bug 1741797

An existing eyedropper isn't destroyed when executing the "Tools -> Browser Tools -> Eyedropper" menu-item. Instead a new one is created.

The original report also says: "I don't think the menuitem should be turned into a checkbox type menuitem, just that the command it sends to the eyedropper should result in exiting the eyedropper if already active."

But perhaps the menu-item could be turned into checkbox style?

Honza

Comment 1

[Shane Hughes [:aminomancer]]

Thanks!

I agree that giving it type="checkbox" would be great, but will require additional listeners and presumably extra code in more than one module. I didn't want to make a large demand so as not to discourage someone from submitting a patch. I'm willing to work on this actually but I can't submit it myself. But imo a checkbox type menuitem would be ideal.

Right now, its checked property can be set by the oncommand handler which should be able to ascertain whether the inspector has an active eyedropper or not. It also needs, at least, a popupshowing handler to determine the eyedropper state of the active tab. Because the user can enable eyedropper in tab 1, then switch to tab 2 without killing the eyedropper, then switch back to tab 1 and so on.

The reason this seems like a big ask is because there isn't any code for setting up such a popupshowing handler. I think some one-off code would need to be added to browser-menus.js that adds a popupshowing listener that calls a method in the menuitem's definition in menus.js.

I also couldn't tell if any code for actually checking if an eyedropper exists. There are events emitted by inspector.js but only when the color pick ends, and that also doesn't tell you about the browser in which the eyedropper instance exists. So either way it's a bit beyond me, but I figured it would be easier to just keep the menuitem static for now and just focus on making the callback's behavior dynamic, probably further downstream than menus.js but idk