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

RESOLVED DUPLICATE of bug 1528608

Status

defect
P3
trivial
RESOLVED DUPLICATE of bug 1528608
11 months ago
3 months ago

People

(Reporter: scook0+bugzilla, Unassigned)

Tracking

({regression})

62 Branch

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox62 wontfix, firefox64 wontfix)

Details

Attachments

(1 attachment)

(Reporter)

Description

11 months ago
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.
(Reporter)

Comment 1

11 months ago
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.
(Reporter)

Comment 2

11 months ago
I imagine this only affects non-Mac platforms.

Comment 3

11 months ago
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0
20180704220810
Blocks: 1014090
Severity: normal → trivial
Status: UNCONFIRMED → NEW
Has STR: --- → yes
Component: Untriaged → Inspector
Ever confirmed: true
Flags: needinfo?(pbrosset)
Keywords: regression
Product: Firefox → DevTools

Comment 4

11 months ago
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.
Attachment #8990075 - Flags: review?(pbrosset)

Updated

11 months ago
Assignee: nobody → chris
Status: NEW → ASSIGNED
Thanks very much Chris for taking a look at this. I will review the patch and test it locally today.
Flags: needinfo?(pbrosset)
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.
Attachment #8990075 - Flags: review?(pbrosset) → feedback+

Updated

11 months ago
Priority: -- → P3

Comment 7

4 months ago

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

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.

Assignee: chris → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(chris)

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

Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1528608
You need to log in before you can comment on or make changes to this bug.