Support "ctrl-s" shortcut to save text.

Review Request #2394 — Created June 10, 2011 and submitted

Information

Djblets

Reviewers

Support "ctrl-s" shortcut to save text.

* Support "ctrl-s" to save pending input text in review description, comments, etc.
Manually tested with IE, Firefox, Chrome, Safari and Opera and unit test.

chipx86
  1. 
      
  2. djblets/media/js/jquery.gravy.js (Diff revision 1)
     
     
    I don't think you need to keep custom state like this. Instead, in keypress, you should be able to just check e.ctrlKey. So, keeping what we had, but having a case statement for the S key, and checking for ctrl. Or did you try that and it didn't work?
  3. djblets/media/js/jquery.gravy.js (Diff revision 1)
     
     
    I think all these key codes are provided by jquery.ui. $.ui.keyCode.CONTROL, for instance.
  4. djblets/media/js/jquery.gravy.js (Diff revision 1)
     
     
     
    Blank line between these.
  5. djblets/media/js/jquery.gravy.js (Diff revision 1)
     
     
    No space in the parens.
    
    Actually, no need for this statement. return false should do it.
  6. 
      
HO
chipx86
  1. 
      
  2. djblets/media/js/jquery.gravy.js (Diff revision 2)
     
     
    I think this should be keypress again. Does that work, or is there a reason it'll fail?
    1. In some browsers such as IE, keypress just captures the actual key. The keys such as "ctrl", "esc", and "enter" won't be captured.
    2. Sorry. I take my words back. The "enter" and "esc" works very well in IE. The problem is that 
      1. e.ctrl won't capture anything in IE. 
      2. In FF and other browsers, I have difficulties to overwrite default "ctrl-s" with keypress.
      3. The "esc" is not captured in Google Chrome and Safari.
    3. Esc should be captured in Chrome/Safari, but I know that Chrome had some issues lately with keyboard bindings.
      
      Maybe we can use keydown just for the cases we need it, and keypress for everything else? I'm hoping not to change too much.
    4. Yes. I am fine to use keydown for 'ctrl-s' only.
      For the ESC, have you tried that. It didn't work on my computer no matter demo or localhost. My versions is Chrome(12.0.742.100), Safari(5.0.5). Not sure whether it is due to Windows environment. However, I suggest to put ESC under keydown as well to make sure it worked.
    5. Yes. I am fine to use keydown for needed. Actually, I think that's a good advice to keep RB stable.
      
      For the ESC, have you tried that on your PC. It didn't work on my PC no matter demo or localhost. My versions is Chrome(12.0.742.100), Safari(5.0.5). Not sure whether it is due to Windows environment. However, I suggest to put ESC under keydown as well to make sure it worked.
    6. Oh. Above may show a bug in RB. The step to reproduce:
      1. reply a comment.
      2. CTRL-ENTER
      3. click the reply on 1 to edit.
      4. CTRL-ENTER
      5. publish
      
      Both replies showed up finally. I think only the final reply should showed up.
    7. Someone recently reported that bug. I'll look into it.
    8. Don't worry about Escape in Chrome. It's happening here too, but it's definitely a third-party bug. I wouldn't change any code for it right now.
    9. Don't worry about Escape in Chrome. It's happening here too, but it's definitely a third-party bug. I wouldn't change any code for it right now.
      
      If it works in Firefox, it should be fine. I don't feel I can trust Chrome/Safari at the moment with keyboard shortcuts.
    10. Don't worry about Escape in Chrome. It's happening here too, but it's definitely a third-party bug. I wouldn't change any code for it right now.
      
      If it works in Firefox, it should be fine. I don't feel I can trust Chrome/Safari at the moment with keyboard shortcuts.
      
      I'm having a lot of trouble duplicating the Control-Enter bug. We can talk about this more on IRC, but I'm wondering if it was somehow fixed from another JavaScript change I made tonight.
  3. 
      
HO
Review request changed
Change Summary:
Changed according to Christian's advices.
Description:
   

Support "ctrl-s" shortcut to save text.

   
   
  • Support "ctrl-s" to save pending input text in review description, comments, etc.
-  
  • Fixed the issue, which shortcut "enter" and "esc" didn't work in some browsers.
Testing Done:
~  

Manually tested with IE, Firefox, Chrome, Safari and Opera.

  ~

Manually tested with IE, Firefox, Chrome, Safari and Opera and unit test.

Bugs:
Added Files:
chipx86
  1. Thanks! Committed to master (0fe4a2f)
  2.