Map CMD+Shift+C to 'inspect element'

VERIFIED FIXED in Firefox 62

Status

defect
P2
normal
VERIFIED FIXED
5 years ago
2 months ago

People

(Reporter: canuckistani, Assigned: chris, Mentored)

Tracking

(Blocks 1 bug, {good-first-bug})

30 Branch
Firefox 62
x86
macOS
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed, firefox64 verified, firefox65 verified, firefox66 verified)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Chrome has this, and people have suggested it is important to them:

https://twitter.com/colinorama/status/469161120918290432
filter on CLIMBING SHOES
Priority: -- → P3
Chrome supports cmd+opt+C like we do, but they also support cmd+shift+C, and we don't. 
Another tweet showing the need for us to add support for it too: https://twitter.com/Jon47/status/991338076701118464

(Note: this is only on macOS, on Windows and linux, both we and chrome support ctrl+shift+C).

There doesn't seem to be any browser action mapped to cmd+shift+C, so it shouldn't be a problem adding it.

DevTools shortcuts that are added when new browser windows are created are added in the KeyShortcuts section of the file /devtools/startup/devtools-startup.js
They also need to be synced in /devtools/client/definitions.js because shortcut help tooltips are displayed there.

From what I can understand, we should add something like this at the end of the KeyShorcuts getter in devtools-startup.js:

  // Extra shortcut for Mac users to open the inspector because Chrome has it too so it's
  // good for muscle memory.
  if (isMac) {
    shortcuts.push({
      toolId: "inspector",
      shortcut: KeyShortcutsBundle.GetStringFromName("inspector.commandkey"),
      modifiers: "accel,shift"
    });
  }

Alex, you know the startup code like nobody else, do you think that would be the right thing to do here?
Flags: needinfo?(poirot.alex)
Tested on Mac just now, this seems to work fine.
(In reply to Patrick Brosset <:pbro> from comment #3)
> Chrome supports cmd+opt+C like we do, but they also support cmd+shift+C, and
> we don't. 
> Another tweet showing the need for us to add support for it too:
> https://twitter.com/Jon47/status/991338076701118464
> 
> (Note: this is only on macOS, on Windows and linux, both we and chrome
> support ctrl+shift+C).
> 
> There doesn't seem to be any browser action mapped to cmd+shift+C, so it
> shouldn't be a problem adding it.
> 
> DevTools shortcuts that are added when new browser windows are created are
> added in the KeyShortcuts section of the file
> /devtools/startup/devtools-startup.js
> They also need to be synced in /devtools/client/definitions.js because
> shortcut help tooltips are displayed there.
> 
> From what I can understand, we should add something like this at the end of
> the KeyShorcuts getter in devtools-startup.js:
> 
>   // Extra shortcut for Mac users to open the inspector because Chrome has
> it too so it's
>   // good for muscle memory.
>   if (isMac) {
>     shortcuts.push({
>       toolId: "inspector",
>       shortcut: KeyShortcutsBundle.GetStringFromName("inspector.commandkey"),
>       modifiers: "accel,shift"
>     });
>   }
> 
> Alex, you know the startup code like nobody else, do you think that would be
> the right thing to do here?

Yes. You may also want to tweak the tooltip accordingly over here:
https://searchfox.org/mozilla-central/source/devtools/client/definitions.js#75-78
Flags: needinfo?(poirot.alex)
I unfortunately don't have the time to work on this myself, but looking at comment 3 and comment 5 should be helpful for someone else to work on this.
I set myself as a mentor on this bug, which means I can guide anyone interested in fix this.
If you have never worked on this project before, I strongly suggest going through the contribution docs first: http://docs.firefox-dev.tools/
Mentor: pbrosset
Keywords: good-first-bug
Priority: P3 → P2

Comment 7

a year ago
(In reply to Patrick Brosset <:pbro> from comment #6)
> I unfortunately don't have the time to work on this myself, but looking at
> comment 3 and comment 5 should be helpful for someone else to work on this.
> I set myself as a mentor on this bug, which means I can guide anyone
> interested in fix this.
> If you have never worked on this project before, I strongly suggest going
> through the contribution docs first: http://docs.firefox-dev.tools/

Hi Patrick, I want to work on this bug.
Can you assist me setup so i can start working on it ASAP.
Flags: needinfo?(pbrosset)

Comment 8

a year ago
(In reply to Patrick Brosset <:pbro> from comment #6)
> I unfortunately don't have the time to work on this myself, but looking at
> comment 3 and comment 5 should be helpful for someone else to work on this.
> I set myself as a mentor on this bug, which means I can guide anyone
> interested in fix this.
> If you have never worked on this project before, I strongly suggest going
> through the contribution docs first: http://docs.firefox-dev.tools/

Hey Patrick , I have started working on this bug , currently going through the docs you pointed at. Will ask for help if I need any.Thanks
Assignee

Comment 9

a year ago
LOL, I was about to write that I'm up for giving this a go, before I saw these two posts, but I'll likely have a go anyway, and see if what I thought I was gonna do matches what the others have.

I've been looking into how keyboard shortcuts work on Firefox anyway, after wishing there was a native keystroke available for opening a new container tab (something like 'cmd+shift+T' or 'cmd+shift+C'), so this seems like a nice way in.

I'm currently working through the instructions to set up a dev environment (i.e. working through the steps listed in http://docs.firefox-dev.tools), and if I'm successful, I'll update later this afternoon.

If nothing else, there might be some PRs to the docs I end up generating even is this is resolved by one of the other kind souls volunteering 
(In reply to georgeranch31 from comment #7)
> Hi Patrick, I want to work on this bug.
> Can you assist me setup so i can start working on it ASAP.
Please see comment 6, it has a link to our contribution documentation. There should be enough information in there to set up the dev environment and get started on a fix for this.
Next, see comment 3 that has some code example for /devtools/startup/devtools-startup.js, which I think should work.
Flags: needinfo?(pbrosset)
Having said that, please feel free to send any specific questions about the set-up my way. You can also join our public Slack and ask questions live there: https://devtools-html-slack.herokuapp.com/

Also, 3 persons have shown interest in fixing this bug. Let's not all work on the same bug :)
For info, we have other bugs ready to be picked up here: 
http://bugs.firefox-dev.tools/?easy&tool=all&mentored
(you can use the sidebar on this page to filter bugs).
Assignee

Comment 12

a year ago
Hi folks, I think I've got a dev enviroment up now.

I'm going to share my how I see it, as even if I can't get a PR put together it might help another person.

The devtools-startup.js file is where keyboard shortcuts are defined. It fetches the current shortcuts from a properties file based on the locale. In my case, it's at:

https://searchfox.org/mozilla-central/source/devtools/startup/locales/en-US/key-shortcuts.properties#35

This is where I see that 'C' is the key.

On startup these are read by devtools-startup.js, and the shortcuts are defined in KeyShortcuts, which currently returns an array listing all the shortcuts.

https://searchfox.org/mozilla-central/source/devtools/startup/devtools-startup.js#64

You're suggesting adding a check with isMac, to push another entry into that array, subbing in the extra key stroke like so:

{
  toolId: "inspector",
  shortcut: KeyShortcutsBundle.GetStringFromName("inspector.commandkey"),
  modifiers: "shift,accel"
}

So we'd need something like:

let shortCuts = [
// all the shortcuts defined as usual - an array of objects
]

if (isMac) {
    shortCuts.push({
      toolId: "inspector",
      shortcut: KeyShortcutsBundle.GetStringFromName("inspector.commandkey"),
      modifiers: "accel,shift"
    })
  };

return shortCuts


After that, we'd need to update the tooltips, to show this, so we'd need to update this bit here to show both shortcuts presumably:
https://searchfox.org/mozilla-central/source/devtools/client/definitions.js#75-78

I've put a patch together to show this, but I think I need some guidance, as if you run this patch, you'll see that the brackets only wrap the first shortcut declaration and not the second. I think this is because the function for showing these tooltips only ever assume one shortcut. So we'd need to update the call to:

l10n("inspector.tooltip2", "Ctrl+Shift+")

And make it take a second argument for the alternative shortcut combo.

Can you let me know if this is on the right track?

For what it's worth, here's the patch as it stands - I need to dash off for dinner, but hopefully it'll either help shed some light for someone else, or once I have a pointer about the tooltip, I'll be able to share a patch as you've outlined below:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch


FWIW, I've got this working locally on my machine - I just need a pointer on the tooltip, and perhaps some guidance in prepping the patch so it can be merged in.
Flags: needinfo?(pbrosset)
Thanks Chris for the detailed analysis so far. You're definitely on the right track.
For the tooltip, here's what I think we should do:

1. Create a new localized string in startup.properties, something like:
inspector.mac.tooltip=DOM and Style Inspector (%1$S or %2$S)
So it's exactly the same as inspector.tooltip2 but specific for Mac, and has 2 parameters

2. In definition.js, change the tooltip logic so that on non-Mac, it does the same as today, but on Mac it does:
l10n("inspector.mac.tooltip", "Cmd+Opt+" + key, "Cmd+Shift+" + key)
where key is just l10n("inspector.commandkey") (same as before).

3. Now, this will also require the l10n function to be changed because right now it only supports 1 argument. Thankfully, with ES6, it's easy:
function l10n(name, ...args) {
  try {
    return args ? L10N.getFormatStr(name, ...args) : L10N.getStr(name);
  } catch (ex) {
    console.log("Error reading '" + name + "'");
    throw new Error("l10n error with " + name);
  }
}

That should do it.
I should note that the new string in startup.properties should also come with a self-explanatory comment that says that the string is exactly the same as inspector.tooltip2, but only for mac, and has 2 shortcuts, etc... because translators need as much context about strings as we can give them.

So far you're the one who's shown most progress here, let me assign the bug to you. Feel free to submit a patch for review to me, and I'll take a look at it quickly.

Thank you for your work on this!
Assignee: nobody → chris
Status: NEW → ASSIGNED
Flags: needinfo?(pbrosset)
Assignee

Comment 14

a year ago
Hi Patrick - I've made those those changes as you've suggested.

My patch is at the link below, and I've outlined my reasoning there for using a gist.

In short, it wasn't obvious to me how to get a patch attached to this bug, using bugzilla or the `hg bzexport -e` command suggested in the docs, so this was the fastest way I knew to share the changes for feedback as a prelude to doing review 'properly'.

https://gist.github.com/mrchrisadams/fafe28f539151585dc4c8a9ea598aa17


I have a few questions I think I'll need help with before I can finish this up:


### This is not a toggle - it just opens the inspector

Right now, hitting ctrl+cmd+C or cmd+opt+C will open the inspector, but not close it. To do this, you need to use the cmd+opt+I shortcut.

This is the same behaviour as Chrome right now - can you confirm that this is the desired behaviour here?


### Testing this

I'm manually test this by working in a loop along the lines of

1. Run "./mach build && ./mach run"
2. Look at browser behaviour, and check keystroke and tooltip.
3. Make changes in vscode
4. goto step 1, until all is working as expected

This means I haven't written any tests yet - where would I write these, and is there a test harness or similar I might use if I *was* to add some tests?

### Following the patch procedure

I've mentioned I'm having a hard time prepping and submitting a patch as per the docs, as all the ways I tried I wasn't successful with. I think I'll need some help if I am to follow that process with you. (My mercurial is a big hazy - I last used it in 2010…)

Anyway - in the devtools slack as @mrchrisadams, so I'm happy to chat there if it helps me sort the last review steps of this, to get it in shape for you folks to use.
Flags: needinfo?(pbrosset)
(In reply to Chris Adams from comment #14)
> Hi Patrick - I've made those those changes as you've suggested.
Thanks!

> In short, it wasn't obvious to me how to get a patch attached to this bug,
> using bugzilla or the `hg bzexport -e` command suggested in the docs, so
> this was the fastest way I knew to share the changes for feedback as a
> prelude to doing review 'properly'.
> 
> https://gist.github.com/mrchrisadams/fafe28f539151585dc4c8a9ea598aa17
No problem, if you're having problems to attach a patch here (or on mozreview), a gist is fine too. I can always land it for you, making sure your name appears as the commit author.

> ### This is not a toggle - it just opens the inspector
> 
> Right now, hitting ctrl+cmd+C or cmd+opt+C will open the inspector, but not
> close it. To do this, you need to use the cmd+opt+I shortcut.
> 
> This is the same behaviour as Chrome right now - can you confirm that this
> is the desired behaviour here?
Yes it is.

> ### Testing this
> 
> I'm manually test this by working in a loop along the lines of
> 
> 1. Run "./mach build && ./mach run"
> 2. Look at browser behaviour, and check keystroke and tooltip.
> 3. Make changes in vscode
> 4. goto step 1, until all is working as expected
Sounds good, this is the typical workflow.
You can also do ./mach build faster instead of ./mach build since you are only changing static files, you don't need to rebuild the full thing.
As far as I know, on Mac, some files also don't even need a build at all because they are symlinked from the source files.

> This means I haven't written any tests yet - where would I write these, and
> is there a test harness or similar I might use if I *was* to add some tests?
There is definitely a test harness in place. Thanks for asking about that.
I believe this test might need to be changed: \devtools\client\framework\test\browser_keybindings_01.js
You can try to run it with your change, see if it still passes. And then check the code in there to see if changes are needed to account for the new shortcut.
Some docs about running tests: 
http://docs.firefox-dev.tools/tests/
http://docs.firefox-dev.tools/tests/mochitest-devtools.html
Here you would run: 
./mach mochitest devtools/client/framework/test/browser_keybindings_01.js

> ### Following the patch procedure
> 
> I've mentioned I'm having a hard time prepping and submitting a patch as per
> the docs, as all the ways I tried I wasn't successful with. I think I'll
> need some help if I am to follow that process with you. (My mercurial is a
> big hazy - I last used it in 2010…)
> 
> Anyway - in the devtools slack as @mrchrisadams, so I'm happy to chat there
> if it helps me sort the last review steps of this, to get it in shape for
> you folks to use.
Great! Asking for live help there is the best way to solve this. I'll try to get some time today and connect.
In the meantime, you can always save your patch as a diff file on your hard drive, then just use the "add an attachment" feature here on bugzilla, mark the file as a "patch" when doing so, and setting the review (R) flag to ? and typing my name in the input field next to it.
Flags: needinfo?(pbrosset)
Assignee

Comment 16

a year ago
This is an unfinished patch - I still need to add a few tests, and give a last check for typos I might have introduced but it should give an idea of the direction I'm headed in.

Now I know how to add run the tests, I'll check at and add those.
Attachment #8979536 - Flags: review?(pbrosset)
Comment on attachment 8979536 [details] [diff] [review]
tweak-short-cuts-for-mac-no-tests-yet.patch

Review of attachment 8979536 [details] [diff] [review]:
-----------------------------------------------------------------

Definitely on the right track. Thanks!

::: devtools/client/definitions.js
@@ +630,4 @@
>   *        Optional format argument.
>   * @returns A localized version of the given key.
>   */
> +function l10n(name, ...args) {

The jsdoc comment above this function still refers to this argument as "arg", not "args". Could you please update it?

::: devtools/client/locales/en-US/startup.properties
@@ +154,4 @@
>  # Keyboard shortcut for DOM and Style Inspector will be shown inside brackets.
>  inspector.tooltip2=DOM and Style Inspector (%S)
>  
> +# LOCALIZATION NOTE (inspector.tooltip2)

The key name in parens here needs to match the actual key name below. So please change this to inspector.mac.tooltip

@@ +154,5 @@
>  # Keyboard shortcut for DOM and Style Inspector will be shown inside brackets.
>  inspector.tooltip2=DOM and Style Inspector (%S)
>  
> +# LOCALIZATION NOTE (inspector.tooltip2)
> +# On a mac, we support toggling the inspector short with cmd+shift+C and cmd+opt+C

Some suggested rephrasing:
"This is the exact same string than inspector.tooltip2, except that it is shown on mac only, where we support toggling ...[rest of your sentence]..."

::: devtools/startup/devtools-startup.js
@@ +69,4 @@
>  
>    // List of all key shortcuts triggering installation UI
>    // `id` should match tool's id from client/definitions.js
> +  let shortCuts = [

nit: other instances of this same word in this file are non camel-cased, so I suggest writing this shortcuts instead of shortCuts.
Attachment #8979536 - Flags: review?(pbrosset) → feedback+
Assignee

Comment 18

a year ago
Hi Patrick,

Thanks for the review - I'm a bit embarrassed to have missed the string mismatches, but glad to have a second pair of eyes. I've included all your suggested changes in this patch.

On the testing front, I was able to set up the the test harness, but after having a look through the code, there wasn't an obvious place in to me where I would write a test to check for a second set of shortcuts.

This is partly down to what I think the code is doing in both `buildDevtoolsKeysetMap` and `hookKeyShortcuts`.

As far as I can tell, in the test you referred to tests a keyboard shortcut by calling 'keysetMap.inspector.synthesizeKey()', then checking if the inspector is visible a few lines later:

https://searchfox.org/mozilla-central/source/devtools/client/framework/test/browser_keybindings_01.js#63


We set up these keyboard shortcuts by calling `buildDevtoolsKeysetMap`, which relies on shortcuts to be made available by `hookKeyShortCuts` - creating one <key> element for each keyboard shortcut:

https://searchfox.org/mozilla-central/source/devtools/startup/devtools-startup.js#549

This fetches all the keyboard shortcuts, defined in the call to the lazyGetter at:

https://searchfox.org/mozilla-central/source/devtools/startup/devtools-startup.js#64

Because I think we're keying by 'id' in the `buildDevtoolsKeysetMap` function, and our current approach is to push another entry to the array returned by 'Keyshortcuts' lazygetter, we are reusing the id.

As far as I'm aware, the 'id' what we rely on to make the inspector panel appear, so if we wanted a separate test for this other keystroke, I think we'd need to create another id, like 'inspectorMac' to trigger the inspector in *just this case* and replicate much of the code we have for 'inspector' here:

https://searchfox.org/mozilla-central/source/devtools/client/definitions.js#68

I'm not sure how far down the rabbit hole you want to go in this case - I could have a stab at replicating all this just so we have a separate test for this extra keyboard shortcut, but I'd rather check with you first.

What do you think?
Attachment #8979835 - Flags: review?(pbrosset)
(In reply to Chris Adams from comment #18)
> Thanks for the review - I'm a bit embarrassed to have missed the string
> mismatches, but glad to have a second pair of eyes. I've included all your
> suggested changes in this patch.
Hey don't worry about it, that's what reviews are for!

> On the testing front, I was able to set up the the test harness, but after
> having a look through the code, there wasn't an obvious place in to me where
> I would write a test to check for a second set of shortcuts.
> 
> This is partly down to what I think the code is doing in both
> `buildDevtoolsKeysetMap` and `hookKeyShortcuts`.
> 
> As far as I can tell, in the test you referred to tests a keyboard shortcut
> by calling 'keysetMap.inspector.synthesizeKey()', then checking if the
> inspector is visible a few lines later:
> 
> https://searchfox.org/mozilla-central/source/devtools/client/framework/test/
> browser_keybindings_01.js#63
Oh I see, because in devtools-startup.js we used the same toolId (i.e. "inspector"), the second shortcut actually overwrites the first one in the test, and we really just test that second one only.

> As far as I'm aware, the 'id' what we rely on to make the inspector panel
> appear, so if we wanted a separate test for this other keystroke, I think
> we'd need to create another id, like 'inspectorMac' to trigger the inspector
> in *just this case* and replicate much of the code we have for 'inspector'
> here:
> 
> https://searchfox.org/mozilla-central/source/devtools/client/definitions.
> js#68
You're right, we do rely on this id being "inspector". So I think the best solution here is to stick with using this id, but changing the test to use a different data-structure and testing both shortcuts.

1. Rename keysetMap to something more self-explanatory like allKeys, and make it be an array [] instead of an object {}

2. In buildDevtoolsKeysetMap, instead of adding those new objects as properties of the keysetMap object, push them as items of the allKeys array instead:
allKeys.push({
  toolId: key.id.split("_")[1],
  key: key.getAttribute("key"),
  ... rest unchanged ...
});

3. In the actual test (in add_task), change the logic slightly. For the inspector it would be something like:

info("Test the first inspector key (there are 2 of them)")
let inspectorKeys = allKeys.filter(({ toolId }) => toolId === "inspector");

info("The first inspector key should open the toolbox.");
let onToolboxReady = gDevTools.once("toolbox-ready");
inspectorKeys[0].synthesizeKey();
let toolbox = await onToolboxReady;
await inspectorShouldBeOpenAndHighlighting();

4. Then do the other stuff for webconsole, debugger, netmonitor.

5. And then do the second inspector key test. This way, by doing it after some other tools have been tested, we are testing again that the second key actually does switch back to the inspector again.

info("The second inspector key should should switch to the inspector again and highlight.");
onSelectTool = gDevTools.once("select-tool-command");
inspectorKeys[1].synthesizeKey();
await onSelectTool;
await inspectorShouldBeOpenAndHighlighting();

What do you think?
Comment on attachment 8979835 [details] [diff] [review]
tweak-short-cuts-for-mac-review-1.patch

Review of attachment 8979835 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks for addressing my review comments.
See the previous comment for guidance on what to do with the test.

::: devtools/client/definitions.js
@@ +73,5 @@
>    label: l10n("inspector.label"),
>    panelLabel: l10n("inspector.panelLabel"),
>    get tooltip() {
> +    if (osString == "Darwin") {
> +      let ctrlShiftC = "Ctrl+Shift+" + l10n("inspector.commandkey");

I missed that on my first review, but this should be Cmd+Shift+, not Ctrl+Shift+
Attachment #8979835 - Flags: review?(pbrosset) → feedback+
Comment on attachment 8979536 [details] [diff] [review]
tweak-short-cuts-for-mac-no-tests-yet.patch

Quick note: when uploading a new patch here, there's a list of existing patches displayed on the upload page which you can use to mark the older patch(es) as obsolete.
This way the list of attachments on the bug is always showing the latest valid patch.
Attachment #8979536 - Attachment is obsolete: true
Assignee

Comment 22

a year ago
Hi Patrick, thanks for the quick response.

I think I understand what you're going for and it sounds good to me - I'll have a go, and if there are things that I struggle with, I'll drop a message here.

Will have a go at this tonight, upload the patch, marking others as obsolete. 

Ta,

C
Assignee

Comment 23

a year ago
Hi Patrick,


I've implemented the extra tests as you've suggested.

Because we call `inspectorShouldBeOpenAndHighlighting` twice in the tests (once with each 'inspector'), I've adjusted the function to accept us passing in the inspector we want to test, but otherwise I think I was able to follow your instructions and see the passing tests fine.

I've also updated the ctrlshift/cmdshift you caught in the second review.

Ta,
Attachment #8979835 - Attachment is obsolete: true
Attachment #8980101 - Flags: review?(pbrosset)
Comment on attachment 8980101 [details] [diff] [review]
tweak-short-cuts-for-mac-review-with-tests.patch

Review of attachment 8980101 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect!
Just a couple of really really minor details. I'll set this patch as R+ now because it's good. So please fix these minor issues, and when you re-upload your patch, feel free to set the R flag to + yourself. No need for an extra review now.
Next steps:
- push this to our CI environment to see if everything still passes fine (I'll do this later today)
- if it does, land the commit to mozilla-central.

::: devtools/client/framework/test/browser_keybindings_01.js
@@ +43,3 @@
>      });
>  }
> +//

nit: please remove these // character and leave this as an empty line.

@@ +88,4 @@
>    await onSelectTool;
>    await netmonitorShouldBeSelected();
>  
> +  info("The second inspector key should should switch to the inspector again and highlight.");

nit: should is repeated twice here.
Attachment #8980101 - Flags: review?(pbrosset) → review+
Needinfo for me to remember to push this to try later.
Flags: needinfo?(pbrosset)
Assignee

Comment 26

a year ago
Hi Patrick,

Thanks for the review - I've made the last two changes as requested, and attached the patch. Thanks again for your guidance!
Attachment #8980101 - Attachment is obsolete: true
Attachment #8980250 - Flags: review+
Comment on attachment 8980250 [details] [diff] [review]
tweak-short-cuts-for-mac-review-with-tests-2018-05-24-14-15.patch

I pushed the patch as a commit to mozreview. Let me R+ it there and I'll also push it to try (the CI server) from there.
Attachment #8980250 - Attachment is obsolete: true
Flags: needinfo?(pbrosset)

Comment 29

a year ago
mozreview-review
Comment on attachment 8980293 [details]
Bug 1014090 - Toggle inspector on mac with extra cmd+shift+C shortcut;

https://reviewboard.mozilla.org/r/246458/#review252536
Attachment #8980293 - Flags: review?(pbrosset) → review+
And her is the try push URL: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40a60973f45ba7fc6a6afc5f522d15708e5c9431
Let's wait for all builds and tests to complete and we'll then see whether the patch is landable.
Comment hidden (mozreview-request)
Wait, this is the correct URL: https://treeherder.mozilla.org/#/jobs?repo=try&revision=77d9a1ea7fe44925536ec44db6f2c3afedd1ac4d
(I had previously pushed with another local commit before).
This TRY push shows  a few problems:
- Some ESLint formatting issues,
- And the test fails on some platforms.

I investigated and realized that I simply forgot that the test would only pass on Mac! On other platforms, there's only one inspector key.
Let me make those changes and push again.
Comment hidden (mozreview-request)

Comment 36

a year ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/123010192259
Toggle inspector on mac with extra cmd+shift+C shortcut; r=pbro
Backed out for failing on browser_duplicateIDs.js |  key_inspector

backout: https://hg.mozilla.org/integration/autoland/rev/d1e0a3f9e9453bf08e189855706df3dfbd4d7d1f

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=12301019225960647c08823cfc450a396e47a0ae

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=180198933&repo=autoland&lineNumber=2195

02:47:30     INFO - TEST-PASS | browser/base/content/test/general/browser_duplicateIDs.js | key_jsdebugger should be unique - 
02:47:30     INFO - TEST-PASS | browser/base/content/test/general/browser_duplicateIDs.js | key_netmonitor should be unique - 
02:47:30     INFO - TEST-PASS | browser/base/content/test/general/browser_duplicateIDs.js | key_styleeditor should be unique - 
02:47:30     INFO - TEST-PASS | browser/base/content/test/general/browser_duplicateIDs.js | key_performance should be unique - 
02:47:30     INFO - TEST-PASS | browser/base/content/test/general/browser_duplicateIDs.js | key_storage should be unique - 
02:47:30     INFO - TEST-PASS | browser/base/content/test/general/browser_duplicateIDs.js | key_dom should be unique - 
02:47:30     INFO - Buffered messages finished
02:47:30     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_duplicateIDs.js | key_inspector should be unique - 
02:47:30     INFO - Stack trace:
02:47:30     INFO - chrome://mochitests/content/browser/browser/base/content/test/general/browser_duplicateIDs.js:test/<:5
02:47:30     INFO - chrome://mochitests/content/browser/browser/base/content/test/general/browser_duplicateIDs.js:test:3
02:47:30     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1114
02:47:30     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:976
02:47:30     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795
02:47:30     INFO - TEST-PASS | browser/base/content/test/general/browser_duplicateIDs.js | mainKeyset should be unique -
Flags: needinfo?(pbrosset)
Ah, I had no idea such a test existed. We are indeed now generating 2 <key> elements with the same ID.
So we need a different solution for this patch.
I think I have one.
Each key can have either a toolId or an id.
If a toolId is passed, then it needs to match exactly the id of the tool from definition.js (so, "inspector" in that case). Otherwise the shortcut won't do anything.
If, instead, an id is passed, then some code can be added to onKeyShortcut in devtools\client\framework\devtools-browser.js to do whatever is necessary.

So, the proposal is:
- in devtools-startup.js, remove the toolId from this code:
  if (isMac) {
    // Add the extra key command for macOS, so you can open the inspector with cmd+shift+C
    // like on Chrome DevTools.
    shortcuts.push({
      toolId: "inspector",
      shortcut: KeyShortcutsBundle.GetStringFromName("inspector.commandkey"),
      modifiers: "accel,shift"
    });
  }
and pass an id instead. This could be "inspectorMac" for instance.

- and then in devtools-browser.js, add a case statement to the switch in onKeyShortcut like:

case "inspectorMac":
  gDevToolsBrowser.selectToolCommand(window.gBrowser, "inspector", startTime);
  break;
Flags: needinfo?(pbrosset)
Assignee

Comment 39

a year ago
Hi Patrick!

I've updated the code, to avoid having an duplicate 'inspector' ID now, largely as you outlined.

I've kept this as a separate patch from before - I figured two smaller commits was easier. 

If you prefer, I'm happy to fold this into the previous commit, so there's only one commit for this bug.

### Using MozReview

I think in order to push this code using mercurial or git, I'll need to run through the 'Becoming a Mozilla committer' process, listed below:

https://www.mozilla.org/en-US/about/governance/policies/commit/

However, I figured I'm better off sharing this patch to fix the failing test here before I begin a separate request.

Once this is closed I'll start that process so I can use the MozReview process for the next bug (likely the CSS one you mentioned earlier in slack).
Attachment #8981130 - Flags: review?(pbrosset)
Comment on attachment 8981130 [details] [diff] [review]
tweak-short-cuts-for-mac-review-with-tests-2018-05-28-13-29.patch

Review of attachment 8981130 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you Chris. This looks great. Let me R+ it now. And later I'll push it to mozreview folded with the previous patch, and then from there, to TRY.
Attachment #8981130 - Flags: review?(pbrosset) → review+
Comment hidden (mozreview-request)
(In reply to Chris Adams from comment #39)
> ### Using MozReview
> 
> I think in order to push this code using mercurial or git, I'll need to run
> through the 'Becoming a Mozilla committer' process, listed below:
> 
> https://www.mozilla.org/en-US/about/governance/policies/commit/
> 
> However, I figured I'm better off sharing this patch to fix the failing test
> here before I begin a separate request.
> 
> Once this is closed I'll start that process so I can use the MozReview
> process for the next bug (likely the CSS one you mentioned earlier in slack).
Sounds good. I think someone already started looking at that CSS bug though. But let's chat on slack and find you another bug. There's always more ;)
Comment hidden (mozreview-request)
Attachment #8981130 - Attachment is obsolete: true

Comment 45

a year ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4342f8a66345
Toggle inspector on mac with extra cmd+shift+C shortcut; r=pbro
https://hg.mozilla.org/mozilla-central/rev/4342f8a66345
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62

Updated

a year ago
Product: Firefox → DevTools

Updated

11 months ago
Depends on: 1473438
Flags: qe-verify+

Comment 47

5 months ago
Verified on Firefox Nightly 66.0a1 (2018-12-18), Firefox 65.0b5 and Firefox 64 on Mac OS X 10.14.
Status: RESOLVED → VERIFIED
Flags: qe-verify+

Updated

3 months ago
Depends on: 1528608
You need to log in before you can comment on or make changes to this bug.