1502924 - Remove Ctrl + Shift + S keyboard shortcut

Status: VERIFIED FIXED

(DevTools :: General, enhancement, P2)

Description

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

This bug tracks removing the Ctrl + Shift + S shortcut from devtools, so that Screenshots can use it.

Background:

Firefox Screenshots has been interested in taking over the Ctrl + Shift + S keyboard shortcut from Devtools.

Bug 1456984 measured usage of various keyboard shortcuts, and based on conversation with :digitarald, we are good to proceed with Screenshots taking the shortcut.

Comment 1

[Patrick Brosset <:pbro>]

I've taken a look at bug 1456984 but didn't find the conversation about this. There is a mention about a github issue where that conversation likely took place, but the link is missing.
It would be great to have the decision captured here (or a link to it) so we can later refer back to it if we ever find ourselves confused as to why this shortcut doesn't exist anymore.

Comment 2

[Punam Dahiya [:pdahiya]]

Hi Patrick, Updating bug with GitHub link capturing discussion and devtools ok with Screenshots taking Ctrl + Shift + S shortcut

https://github.com/mozilla-services/screenshots/issues/2491#issuecomment-420012054

Thanks!

Comment 3

[Punam Dahiya [:pdahiya]]

Github link to track Screenshots keyboard shortcut pull request

https://github.com/mozilla-services/screenshots/pull/5229

Comment 4

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Maybe the Screenshots team could just take this bug, since it looks pretty straightforward, and since the 65 soft freeze is right around the corner.

If I'm skimming the devtools code correctly, we'd just need to:
- remove the jsdebugger shortcut definition from devtools/startup/devtools-startup.js[1],
- update the tooltip for the jsdebugger Tool definition in devtools/client/definitions.js[2] (what should the new tooltip be?),
- and remove the 'debugger.commandkey' l10n key[3].

I'm not sure what tests would be impacted, but we could just make the change, see what breaks, and update the broken tests.

Digitarald, does this seem right for a first pass? Punam, any interest in taking a swing here?



[1] https://searchfox.org/mozilla-central/source/devtools/startup/devtools-startup.js#137-141
[2] https://searchfox.org/mozilla-central/source/devtools/client/definitions.js#144
[3] https://searchfox.org/mozilla-central/source/devtools/startup/locales/en-US/key-shortcuts.properties#43

Comment 5

[Punam Dahiya [:pdahiya]]

(In reply to Jared Hirsch [:_6a68] [:jhirsch] from comment #4)
> Maybe the Screenshots team could just take this bug, since it looks pretty
> straightforward, and since the 65 soft freeze is right around the corner.
> 
> If I'm skimming the devtools code correctly, we'd just need to:
> - remove the jsdebugger shortcut definition from
> devtools/startup/devtools-startup.js[1],
> - update the tooltip for the jsdebugger Tool definition in
> devtools/client/definitions.js[2] (what should the new tooltip be?),
> - and remove the 'debugger.commandkey' l10n key[3].
> 
> I'm not sure what tests would be impacted, but we could just make the
> change, see what breaks, and update the broken tests.
> 
> Digitarald, does this seem right for a first pass? Punam, any interest in
> taking a swing here?
> 

Sure, will take a pass at it and share a patch.Thanks!

> 
> 
> [1]
> https://searchfox.org/mozilla-central/source/devtools/startup/devtools-
> startup.js#137-141
> [2]
> https://searchfox.org/mozilla-central/source/devtools/client/definitions.
> js#144
> [3]
> https://searchfox.org/mozilla-central/source/devtools/startup/locales/en-US/
> key-shortcuts.properties#43

Comment 6

[Punam Dahiya [:pdahiya]]

Link to try server run with recommended changes in #comment 4

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f762636d7a721d4fe8fc3fcc53f24f8e3b0bb7f3

Patch is returning empty for jsdebugger tooltip getter, it will be good to get input from devtools on whats alternative tooltip that can be used for jsdebugger. Thanks!

Comment 7

[Punam Dahiya [:pdahiya]]

Try server link with running build

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ace67b9186adc98935a61fb397035b926f7e45a9

Comment 8

[Punam Dahiya [:pdahiya]]

Try server run with updated patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97adeb6bced59ece7871b0a49b48b98f8905d2da

Comment 9

[Punam Dahiya [:pdahiya]]

Attachment: Bug 1502924 - Remove Ctrl + Shift + S keyboard shortcut r=jhirsch,jlast ***

Remove Ctrl + Shift + S ( Cmd + Opt + S for Mac) key board shortcut

Comment 10

[:Harald Kirschner :digitarald]

Forwarding my ni to Jason, who can actually help.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f66935a5c80d
Remove Ctrl + Shift + S keyboard shortcut r=jlast,_6a68

Comment 12

[Punam Dahiya [:pdahiya]]

Created bug 1514055 to update mdn to reflect removal of debugger keyboard shortcut. Thanks!

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 14

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 65.0a1 (2018-10-29) on Windows 7, 64 Bit. This bug's fix is verified with latest Beta!

Build ID : 20190218131312
User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:66.0) Gecko/20100101 Firefox/66.0

[bugday-20190213]

Comment 15

[Mihai Boldan, Desktop QA [:mboldan]]

I'm also confirming that the keyboard shortcut is opening the Screenshots feature on Firefox 67.0a1(2018-02-26) and on Firefox 66.0b11 it doesn't do anything.
Tests were executed under Ubuntu 18.04x64 and under macOS X 10.12.6.
Based on this and on Comment 14, I'm marking this issue as Verfied.

Comment 16

[Oriol Brufau [:Oriol]]

I frequently use the debugger and I have screenshots disabled. This totally breaks my workflow.

Comment 17

[Jason Laster [:jlast]]

CC Harald, do you think we could find a new shortcut?

Comment 18

[:Harald Kirschner :digitarald]

I totally understand the issue of breaking muscle memory. Optimizing for screenshots disabled is a hard one.

The 2 options I see would be adding a modifier to Ctrl-Shift-S or making keyboard shortcuts customizable.

Comment 19

[:Harald Kirschner :digitarald]

Continued in the new bug.