1473438 - Tooltip for the DOM and Style Inspector incorrectly shows its keyboard shortcut as "(Ctrl+Shift+)C"

Status: RESOLVED DUPLICATE of bug 1528608

(DevTools :: Inspector, defect, P3)

Description

[Stuart Cook]

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

Steps to reproduce:

1. Press F12 to open the developer tools.
2. Hover over the "Inspector" tab to view its tooltip.


Actual results:

The tooltip displays as "DOM and Style Inspector (Ctrl+Shift+)C".

Note that the "C" is outside the (parentheses).


Expected results:

The tooltip should be "DOM and Style Inspector (Ctrl+Shift+C)".

The "C" should be inside the (parentheses), together with its modifiers.

Comment 1

[Stuart Cook]

This seems to have been accidentally introduced by the changes for bug 1014090.

https://dxr.mozilla.org/mozilla-central/rev/987ea0d6a000b95cf93928b25a74a7fb1dfe37b2/devtools/client/definitions.js#83

Presumably the concatenation of "Ctrl+Shift+" with l10n("inspector.commandkey") should take place inside the second argument position of the first l10n call.

Comment 2

[Stuart Cook]

I imagine this only affects non-Mac platforms.

Comment 3

[Gingerbread Man]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0
20180704220810

Comment 4

[Chris Adams]

Attachment: Put the Ctrl+Shift tooltip back inside the parentheses

Hi there,

From what I can tell, this was added during when I was working on Bug 1014090 a few weeks back - oops! Here's the link to the bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=1014090

I think this patch I've attached should resolve it, but I don't have a non OS X machine handy to test this on - sorry about that.

I'm at one of the Mozilla nights, so I'll see if someone else is able to test this, and if so I'll update some more.

Patrick, I've requested you looking over it as you mentioned in slack earlier today, and you were mentoring me when we first looked over this part before - hope that's okay.

Comment 5

[Patrick Brosset <:pbro>]

Thanks very much Chris for taking a look at this. I will review the patch and test it locally today.

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8990075 [details] [diff] [review]
Put the Ctrl+Shift tooltip back inside the parentheses

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

Looks good to me, I've tested locally on Mac and Windows and it works fine on both.
I've made a comment below about changing a variable name, minor stuff, but would be great to fix.
Also, I have the impression that you might have based this patch on an older revision of the mozilla source code, because importing your patch locally did not work well for me. I had to merge manually.
Can you make sure you pull the latest, and then redo your changes on top of that?

::: devtools/client/definitions.js
@@ +79,3 @@
>        return l10n("inspector.mac.tooltip", cmdShiftC, cmdOptC);
>      } else {
> +      const cmdShiftC = "Ctrl+Shift+" + l10n("inspector.commandkey");

This variable name should be ctrlShiftC instead of cmdShiftC to be totally correct.

Comment 7

[Gingerbread Man]

Chris, as you still interested in fixing this? If so, please see comment 6.

Comment 8

[Patrick Brosset <:pbro>]

Unassigning this bug as it hasn't been touched in 8 months and Chris hasn't responded to the needinfo 2 months ago.
Please feel free to pick it up again if you did intend to work on it still.

Comment 9

[Razvan Caliman [:rcaliman]]

I recently fixed this issue in a duplicate: bug 1528608
I'll mark this as duplicate and close it.