738925 - A few new keyboard shortcuts for opening and closing popups in TBPL

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, enhancement)

Description

[Matt Brubeck (:mbrubeck)]

Attachment: 1/3: "?" -> toggle help pane

Adding some keyboard shortcuts for some of my frequent actions in TBPL.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: 2/3: Control+Enter -> submit comment

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: 3/3: Escape -> close any open dropdown or popup

Comment 3

[Phil Ringnalda (:philor)]

Comment on attachment 609001 [details] [diff] [review]
1/3: "?" -> toggle help pane

LGTM

Comment 4

[Phil Ringnalda (:philor)]

Comment on attachment 609002 [details] [diff] [review]
2/3: Control+Enter -> submit comment

DO WANT!

Comment 5

[Phil Ringnalda (:philor)]

Comment on attachment 609003 [details] [diff] [review]
3/3: Escape -> close any open dropdown or popup

I'm torn about this, so I'm punting the review to Arpad. It's one of my most-frequent frustrations, trying to find one of the parts that you *can* click on to close things, and it's the key that you would expect to work, and about the third time I tried it out, it was during a tbpl refresh, and Esc both closed the popup and cancelled the refresh (since it's the shortcut for the Stop button), which is not at all nice.

Comment 6

[Matt Brubeck (:mbrubeck)]

(In reply to Phil Ringnalda (:philor) from comment #5)
> Comment on attachment 609003 [details] [diff] [review]
> 3/3: Escape -> close any open dropdown or popup
>
> and about the third time I tried it out, it was during a tbpl refresh, and
> Esc both closed the popup and cancelled the refresh (since it's the shortcut
> for the Stop button), which is not at all nice.

I guess stopping the "keyup" event for the escape key is not enough to prevent the default action.  I'll see if I can find a way to do it, maybe by stopping the keypress event too.

Comment 7

[Arpad Borsos [:Swatinem]]

Comment on attachment 609002 [details] [diff] [review]
2/3: Control+Enter -> submit comment

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

::: js/AddCommentUI.js
@@ +54,5 @@
>      });
>  
> +    $("#logNoteText").bind("keyup", function logNoteTextKeypress(e) {
> +      // Control+Enter submits the form
> +      if (e.which == 13 && (e.ctrlKey || e.metaKey)) {

Please use KeyEvent.DOM_VK_RETURN here, thats much easier to read than a magic number.

Comment 8

[Matt Brubeck (:mbrubeck)]

Attachment: 3/3: Escape -> close any open dropdown or popup

(In reply to Matt Brubeck (:mbrubeck) from comment #6)
> I guess stopping the "keyup" event for the escape key is not enough to
> prevent the default action.  I'll see if I can find a way to do it, maybe by
> stopping the keypress event too.

Switching from "keyup" to "keydown" fixed this problem.

(In reply to Arpad Borsos (Swatinem) from comment #7)
> Please use KeyEvent.DOM_VK_RETURN here, thats much easier to read than a
> magic number.

Unfortunately that isn't portable (won't work in WebKit browsers, for example), unless we provide our own fallback KeyEvent object with hard-coded magic numbers...

Comment 9

[Arpad Borsos [:Swatinem]]

Comment on attachment 609121 [details] [diff] [review]
3/3: Escape -> close any open dropdown or popup

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

Hm there must be some portable way, maybe event.DOM_VK_RETURN (as in the argument to the callback)? Or event.originalEvent when using jQuery, or something.

Comment 10

[Matt Brubeck (:mbrubeck)]

event.DOM_VK_RETURN does not seem to work in WebKit either.  I haven't found any sort of built-in constants that work there, so I'm just pushing this as-is for now:

https://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/3510a775fc18
https://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/9ab5fc84e8b6
https://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/8ef613c912f6