785889 - Make search related keyboard shortcuts discoverable

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Victor Porof [:vporof][:vp]]

Follow up for bug 779732.
Should add them both in the "Filter Scripts" searchbox and the operators popup.

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: v1

Comment 2

[Victor Porof [:vporof][:vp]]

mq:
0 785650 - Make it easier to stop searching for scripts in the Debugger
1 779732 - Make search operations in the debugger more intuitive
2 785883 - Pressing up/down/tab when filtering scripts should have expected behavior
3 780073 - Add keyboard shortcut tooltips across the debugger UI, part 1
4 780073 - Add keyboard shortcut tooltips across the debugger UI, part 2
5 785889 - Make search related keyboard shortcuts discoverable

Comment 3

[Victor Porof [:vporof][:vp]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=eb5e3bd2a1da

Comment 5

[Victor Porof [:vporof][:vp]]

Needs a slight rebase.

Comment 6

[Victor Porof [:vporof][:vp]]

Attachment: v2

Rebased and qfolded on top of 780073.

Comment 7

[Victor Porof [:vporof][:vp]]

Attachment: v2.1

Needed another rebase because of the merge.

Comment 8

[Victor Porof [:vporof][:vp]]

Comment on attachment 656972 [details] [diff] [review]
v2.1

Rob's leaving!

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 656972 [details] [diff] [review]
v2.1

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

Victor, if I understand it correctly, most of the changes in this patch are necessary in order to avoid preprocessing the string files and do it in JS instead?

Axel, am I right to understand that you guys would not like it the other way around? Also, is there anything we need to do after removing strings or reusing (slightly modified) old strings in a new name?

These changes look good to me, except for the regression below. r=me with that fixed.

On a tangential note, the "fullscreen" button is now the only one without a tooltip.

::: browser/devtools/scratchpad/scratchpad.js
@@ -49,5 @@
> -      // XXX bug 779642 Use "Cmd-" literal vs cloverleaf meta-key until
> -      // Orion adds variable height lines
> -      // elemString += keysbundle.GetStringFromName("VK_META_CMD") +
> -      //               keysbundle.GetStringFromName("MODIFIER_SEPARATOR");
> -      elemString += "Cmd-";

You didn't copy this over to LayoutHelpers.jsm, effectively regressing bug 779642.

Comment 10

[Axel Hecht [:Pike]]

Comment on attachment 656972 [details] [diff] [review]
v2.1

To be frank, I don't understand what this patch is actually doing.

You could leave the items in the DTD if you had definitions of the keys as DTD entries, not sure if there are or not.

Nit, I like hg blame to be useful, this patch comes with a host of whitespace changes.

Comment 11

[Victor Porof [:vporof][:vp]]

(In reply to Axel Hecht [:Pike] from comment #10)
> Comment on attachment 656972 [details] [diff] [review]
> v2.1
> 
> To be frank, I don't understand what this patch is actually doing.
> 
> You could leave the items in the DTD if you had definitions of the keys as
> DTD entries, not sure if there are or not.
> 
> Nit, I like hg blame to be useful, this patch comes with a host of
> whitespace changes.

This patch makes sure that shortcuts appear in the tooltips across debugger UI elements. This means, for example, that hovering Step Out shows ⇧F8 and so on. To display the modifier(s) in this case (and some others), moved some strings from .DTD to .properties, to compute these special characters in JS.

(In reply to Panos Astithas [:past] from comment #9)
> Comment on attachment 656972 [details] [diff] [review]
> v2.1
> 
> Review of attachment 656972 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Victor, if I understand it correctly, most of the changes in this patch are
> necessary in order to avoid preprocessing the string files and do it in JS
> instead?
> 

Yes, just like with Scratchpad.

> Axel, am I right to understand that you guys would not like it the other way
> around? Also, is there anything we need to do after removing strings or
> reusing (slightly modified) old strings in a new name?
> 
> These changes look good to me, except for the regression below. r=me with
> that fixed.

Nice catch!

> 
> On a tangential note, the "fullscreen" button is now the only one without a
> tooltip.

I think I'll add this here as well, to avoid a two-liner followup.

Comment 12

[Victor Porof [:vporof][:vp]]

Attachment: v2.2

Addressed comments.

Comment 13

[Victor Porof [:vporof][:vp]]

Attachment: v2.3

Also added tooltips for the expand/collapse panes button.

Comment 14

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/d9313d042bc1

Comment 15

[Victor Porof [:vporof][:vp]]

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