Remove Ctrl + Shift + S keyboard shortcut

VERIFIED FIXED in Firefox 66

Status

P2
normal
VERIFIED FIXED
5 months ago
9 days ago

People

(Reporter: _6a68, Assigned: pdahiya, NeedInfo)

Tracking

(Depends on: 1 bug, {dev-doc-needed})

unspecified
Firefox 66
dev-doc-needed
Dependency tree / graph

Firefox Tracking Flags

(firefox66 verified, firefox67 verified)

Details

Attachments

(1 attachment)

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.
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.
Priority: -- → P2
(Assignee)

Comment 2

4 months ago
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!
(Assignee)

Comment 3

4 months ago
Github link to track Screenshots keyboard shortcut pull request

https://github.com/mozilla-services/screenshots/pull/5229
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
Flags: needinfo?(pdahiya)
Flags: needinfo?(hkirschner)
(Assignee)

Comment 5

4 months ago
(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
Flags: needinfo?(pdahiya)
(Assignee)

Comment 6

4 months ago
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!
(Assignee)

Comment 9

4 months ago
Remove Ctrl + Shift + S ( Cmd + Opt + S for Mac) key board shortcut
Forwarding my ni to Jason, who can actually help.
Flags: needinfo?(hkirschner) → needinfo?(jlaster)

Comment 11

3 months ago
Pushed by pdahiya@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f66935a5c80d
Remove Ctrl + Shift + S keyboard shortcut r=jlast,_6a68
(Assignee)

Updated

3 months ago
Blocks: 1514055
(Assignee)

Comment 12

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

Comment 13

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f66935a5c80d
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Assignee: nobody → pdahiya
Keywords: dev-doc-needed

Updated

2 months ago
Depends on: 1518804

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]

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.

Status: RESOLVED → VERIFIED
status-firefox66: fixed → verified
status-firefox67: --- → verified

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

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

Flags: needinfo?(jlaster) → needinfo?(hkirschner)

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.

You need to log in before you can comment on or make changes to this bug.