1763961 - Keyboard shortcut still works when the "Pick an element from the page" feature is disabled.

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Xu Che]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:100.0) Gecko/20100101 Firefox/100.0

Steps to reproduce:

1. disable "Pick an element from the page" in "Web Developer Tools" -> "Settings"
2. Press the keyboard shortcut for "Pick an element from the page"(Ctrl+Shift+C)

Actual results:

The disabled picker still pops out.

Expected results:

It should not respond to the keyboard shortcut.

If this behaviour is working as intended, please add a more detailed configuration allowing the user to either:

• Completely turn off the picker
• or, disable shortcut or toolbox button

Comment 1

[Julian Descottes [:jdescottes]]

Thanks for filing, it makes sense to disable the picker when the button is disabled.

Note that the shortcut for the picker doubles as the shortcut for the inspector panel. This means that the shortcut would still open the inspector, even if it doesn't start the node picker.

Happy to mentor, for the implementation this is mostly about checking Services.prefs.getBoolPref("devtools.command-button-pick.enabled", false) at https://searchfox.org/mozilla-central/rev/d34f9713ae128a3138c2b70d8041a535f1049d19/devtools/client/definitions.js#162

The preference devtools.command-button-pick.enabled is the one controlling the visibility of the button, so simply checking it before invoking the picker should work. The inspector will still get selected via https://searchfox.org/mozilla-central/rev/d34f9713ae128a3138c2b70d8041a535f1049d19/devtools/client/framework/toolbox.js#1225

Making this a mentored bug, implementation should be easy, but a test will be a must here :)

Comment 2

[Artem Manushenkov]

Attachment: Bug 1763961 [devtools] Make unchecking "Pick an element from the page" in "Available Toolbox Buttons" disable hotkey for opening element picker/inspector r=jdescottes

Comment 3

[Artem Manushenkov]

Hola! I'd like to fix this!

I'm submitting a patch. For the inspector part i added the check in devtools-browser.js since i couldn't get it to work in toolbox.js

The toolbox.js function selectToolCommand i edited is "triggered from the WebDeveloper menu and keyboard shortcuts" according to the description, so i tested Tools->Browser Tools->Web developer tools in the top bar and it worked as intended (macOS)

As for the adding the test, I need a bit of guidance:

I found this test https://searchfox.org/mozilla-central/source/devtools/client/framework/test/browser_keybindings_01.js
it confirms that the picker responded to the hotkey by checking for the emitted event toolbox.nodePicker.once("picker-started"), what is a good approach to confirming that it never starts? a timeout?

Comment 4

[Artem Manushenkov]

Attachment: Bug 1763961 [devtools] Make unchecking "Pick an element from the page" in "Available Toolbox Buttons" disable hotkey for opening element picker/inspector r=jdescottes

Comment 5

[Julian Descottes [:jdescottes]]

(In reply to Artem Manushenkov from comment #3)

Hola! I'd like to fix this!

I'm submitting a patch. For the inspector part i added the check in devtools-browser.js since i couldn't get it to work in toolbox.js

The toolbox.js function selectToolCommand i edited is "triggered from the WebDeveloper menu and keyboard shortcuts" according to the description, so i tested Tools->Browser Tools->Web developer tools in the top bar and it worked as intended (macOS)

As for the adding the test, I need a bit of guidance:

I found this test https://searchfox.org/mozilla-central/source/devtools/client/framework/test/browser_keybindings_01.js
it confirms that the picker responded to the hotkey by checking for the emitted event toolbox.nodePicker.once("picker-started"), what is a good approach to confirming that it never starts? a timeout?

Thanks for the patch! I added a comment on phabricator, we need to make sure that the inspector still gets selected when the shortcut is used.
As for the test, browser_keybindings_01 is probably a good test to use as a basis. You can add new test cases for the inspector that will also set the preference. You can use await pushPref("devtools.command-button-pick.enabled", value) to set the value of the preference in the test. And then yes, I think it's a good idea here to use a timeout to check that the picker-started event was not emitted. You can use await wait(X); to avoid having to write the timeout code (the linter would complain about it otherwise).

Comment 6

[Julian Descottes [:jdescottes]]

I have looked a bit more at the test and it's really fragile.

We listen to the "picker-started" event, but there is no guarantee that the event listener (in the test) will be attached before the event is fired. This is only going to work if the toolbox is not created yet, or if we are switching from another tool.

So at the minimum, I suggest that you add the test cases for the inspector with the preference set to false not directly after the basic inspector test case. And ideally, we would update the test so that we create onPickerStart before we call synthesizeKey if toolbox already exists.

Comment 7

[Artem Manushenkov]

I made a compromise for the test, since the disabled hotkey still should lead to the inspector, i made the test switch to another tool tab to test that it both

1. doesn't trigger the picker
2. still opens the inspector tab

What do you think about it?

I also added a makeshift Promise.race to check that the condition times out, since i couldn't find an existing function that does so. It feels like there should be a helper for it but i couldn't find anything like that even in existing testing libraries. let me know if there's a better way to check for it

Comment 8

[Julian Descottes [:jdescottes]]

That sounds good, will review shortly.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/163f397f8ee2
[devtools] Make unchecking "Pick an element from the page" in "Available Toolbox Buttons" disable hotkey for opening element picker/inspector r=jdescottes,devtools-reviewers

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/163f397f8ee2

Comment 11

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Reproducible on a 2024-03-10 Nightly build on Windows 10.
Verified as fixed on Firefox Nightly 126.0a1 and Firefox 125.0 on macOS 12, Windows 10, Ubuntu 22.