Definitely open to opposing opinions, but I personally think we should deprecate the option to turn off/on the "Pick an element from the page" in the devtools settings. It's such an integral part of devtools that I think it's a little bizarre that it's optional. (Plus, freeing up some space in Settings might not be the worst thing.)

I agree. Related: in bug 1171939 I'm arguing that it should be possible to disable *all* panels. If we did allow that, then disabling the inspector panel should also turn off the element picker button.

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #1) > Related: in bug 1171939 I'm arguing that it should be possible to disable > *all* panels. If we did allow that, then disabling the inspector panel > should also turn off the element picker button. This makes sense that these go together.

Agreed we should remove this from the options panel

Unsure if I was supposed to add some sort of test—fairly clueless when it comes to testing in general, really...

Comment on attachment 8745308 [details] [diff] [review] command-pick.patch Sorry, clearing review—looks like there's a ton of stuff in this patch file I didn't intend to be reviewed. I'll upload a new one shortly.

Comment on attachment 8745310 [details] [diff] [review] command-pick.patch Review of attachment 8745310 [details] [diff] [review]: ----------------------------------------------------------------- Looks like this should work, but I'm wondering if we shouldn't just remove this tool from the list of toolbox buttons instead. If we agree that this is not a toolbox buttons like the others and that it will never be toggleable, then there's no reason for it to be in the list of ToolboxButtons at the top of devtools\client\framework\toolbox.js at all. Also, the _buildPickerButton in devtools\client\framework\toolbox.js shouldn't hide the button by default, and we should remove the devtools.command-button-pick.enabled pref from devtools\client\preferences\devtools.js ::: devtools/client/framework/toolbox-options.js @@ +185,4 @@ > if (!tool.isTargetSupported(this.toolbox.target)) { > continue; > } > + if (tool.id==='command-button-pick') { This is inconsistent with our coding guidelines and eslint rules: https://wiki.mozilla.org/DevTools/CodingStandards Needs spaces around === and double-quotes.

(In reply to Patrick Brosset <:pbro> from comment #7) > Comment on attachment 8745310 [details] [diff] [review] > command-pick.patch > > Review of attachment 8745310 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks like this should work, but I'm wondering if we shouldn't just remove > this tool from the list of toolbox buttons instead. If we agree that this is > not a toolbox buttons like the others and that it will never be toggleable, > then there's no reason for it to be in the list of ToolboxButtons at the top > of devtools\client\framework\toolbox.js at all. > Also, the _buildPickerButton in devtools\client\framework\toolbox.js > shouldn't hide the button by default, and we should remove the > devtools.command-button-pick.enabled pref from > devtools\client\preferences\devtools.js Various toolbox modes won't show it - like worker debugging or addon debugging. If I'm reading the code right, it's right now not building the button at all in the case of addon debugging, but for workers it's relying on setToolboxButtonsVisibility to do so, which relies on it being part of ToolboxButtons. We should change the isAddon condition here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.js#980 to instead check for target.getTrait("highlightable"). If that happened then I think we could remove it from ToolboxButtons.

Round 2! I tried to adhere to Patrick's suggestions. Everything /seems/ like it's working as intended.

Comment on attachment 8748728 [details] [diff] [review] command-pick.patch Review of attachment 8748728 [details] [diff] [review]: ----------------------------------------------------------------- Almost there. Just one change to make sure this works fine with worker toolboxes. ::: devtools/client/framework/toolbox-options.js @@ +187,5 @@ > } > + // if (tool.id === "command-button-pick") { > + // // We don't want the command-pick appearing in the toolbox options > + // continue; > + // } Why did you add this commented out code here? It looks like this was added by mistake maybe. Please remove it. ::: devtools/client/framework/toolbox.js @@ -1028,4 @@ > this._pickerButton.id = "command-button-pick"; > this._pickerButton.className = "command-button command-button-invertable"; > this._pickerButton.setAttribute("tooltiptext", toolboxStrings("pickButton.tooltip")); > - this._pickerButton.setAttribute("hidden", "true"); These 2 changes in this file look good, but as Brian said in comment 8, we have several "modes" for our toolboxes: normal toolbox, browser toolbox, addon toolbox, worker toolbox (maybe more), and the buttons we show in each one of them is different. If you look at the top of _buildButtons, you'll see that we have a condition block around this._buildPickerButton(). So, if we're in an addon toolbox, then we don't try and build the picker button, which makes sense. But it means that it will be built for a worker toolbox for instance, which doesn't make sense. There is no DOM to be inspected in a worker. So, please change this IF condition from: if (!this.target.isAddon) to if (this.target.getTrait("highlightable")) (I think this is the right trait to use, but you should double-check by starting a worker toolbox from about:debugging).

(In reply to Patrick Brosset <:pbro> from comment #11) > ::: devtools/client/framework/toolbox-options.js > @@ +187,5 @@ > > } > > + // if (tool.id === "command-button-pick") { > > + // // We don't want the command-pick appearing in the toolbox options > > + // continue; > > + // } > > Why did you add this commented out code here? It looks like this was added > by mistake maybe. Please remove it. Removed! 'twas an accident. > These 2 changes in this file look good, but as Brian said in comment 8, we > have several "modes" for our toolboxes: normal toolbox, browser toolbox, > addon toolbox, worker toolbox (maybe more), and the buttons we show in each > one of them is different. I've made the change you suggested and all seems well, but I was unable to see if I'd done it properly for the addon toolbox—is that different from the browser toolbox? How should I doublecheck that that's been done correctly?

Comment on attachment 8750419 [details] [diff] [review] command-pick.patch Review of attachment 8750419 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me! (In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #12) > I've made the change you suggested and all seems well, but I was unable to > see if I'd done it properly for the addon toolbox—is that different from the > browser toolbox? How should I doublecheck that that's been done correctly? You can open about:debugging, and in the add-ons category click on the debug button for any of the add-ons you have currently installed.

(In reply to Patrick Brosset <:pbro> from comment #13) > You can open about:debugging, and in the add-ons category click on the debug > button for any of the add-ons you have currently installed. Seems like everything is shipshape locally, but I'll wait until the trees open again to do a try-push before checking this in.

I've added a note for this button to https://developer.mozilla.org/en-US/docs/Tools/Tools_Toolbox#Extra_tools. Once it's remove, a version note needs to be added there. Sebastian

Looks like we forgot about this bug. I rebased Helen's patch and pushed it to TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=adf4335cc80f9ba1fa915abad76b0c721b97f732 Let's see if this works, and then land it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=889f54dc59e8b6363043c86fe10a5011a0aa50bb Forgot to update one test.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/03fde9f8b44a Don't allow to disable the element picker icon. r=pbro

(In reply to Sebastian Zartner [:sebo] from comment #15) > I've added a note for this button to > https://developer.mozilla.org/en-US/docs/Tools/Tools_Toolbox#Extra_tools. > Once it's remove, a version note needs to be added there. I've removed this. I didn't add a version note, because that section actually refers to the array of buttons on the right-hand side of the toolbar: https://developer.mozilla.org/en-US/docs/Tools/Tools_Toolbox#Toolbar ...and the picker hasn't lived there for ages.

(In reply to Will Bamberg [:wbamberg] from comment #20) > (In reply to Sebastian Zartner [:sebo] from comment #15) > > I've added a note for this button to > > https://developer.mozilla.org/en-US/docs/Tools/Tools_Toolbox#Extra_tools. > > Once it's remove, a version note needs to be added there. > > I've removed this. I didn't add a version note, because that section > actually refers to the array of buttons on the right-hand side of the > toolbar: > > https://developer.mozilla.org/en-US/docs/Tools/Tools_Toolbox#Toolbar > > ...and the picker hasn't lived there for ages. Good point. Users might still wonder where that option is gone, though. Anyway, it's fine for me to leave it out. Sebastian

> Users might still wonder where that option is gone, though. Fair enough. I added a note here: https://developer.mozilla.org/en-US/docs/Tools/Settings?document_saved=true#Available_Toolbox_Buttons and updated the screenshot for the "Settings" page.

I have reproduced this bug with Nightly 45.0a1 (2015-11-21) on Windows 7 , 64 Bit ! This bug's fix is verified with latest Developer Edition (Aurora) Build ID 20170118004007 User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0 [bugday-20170118]

As of comment 23. Sebastian