Closed Bug 738925 Opened 8 years ago Closed 8 years ago

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

Categories

(Tree Management Graveyard :: TBPL, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

Attachments

(3 files, 1 obsolete file)

Adding some keyboard shortcuts for some of my frequent actions in TBPL.
Attachment #609001 - Flags: review?(philringnalda)
Attachment #609002 - Flags: review?(philringnalda)
Attachment #609003 - Flags: review?(philringnalda)
Comment on attachment 609001 [details] [diff] [review]
1/3: "?" -> toggle help pane

LGTM
Attachment #609001 - Flags: review?(philringnalda) → review+
Comment on attachment 609002 [details] [diff] [review]
2/3: Control+Enter -> submit comment

DO WANT!
Attachment #609002 - Flags: review?(philringnalda) → review+
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.
Attachment #609003 - Flags: review?(philringnalda) → review?(arpad.borsos)
(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.
Attachment #609003 - Flags: review?(arpad.borsos)
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.
(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...
Attachment #609003 - Attachment is obsolete: true
Attachment #609121 - Flags: review?(arpad.borsos)
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.
Attachment #609121 - Flags: review?(arpad.borsos) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 743595
Product: Webtools → Tree Management
Product: Tree Management → Tree Management Graveyard
You need to log in before you can comment on or make changes to this bug.