Summary

Support "ctrl-s" shortcut to save text.

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

Information

Owner

hongbin

Repository

Djblets

Branch

Bugs

1958

Depends On

Reviewers

Groups

djblets

People

Description

Support "ctrl-s" shortcut to save text.

* Support "ctrl-s" to save pending input text in review description, comments, etc.

Testing Done

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

Files

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?

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.

djblets/media/js/jquery.gravy.js (Diff revision 1)
```
Blank line between these.
```

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.

Change Summary:

Thanks Christian. Here is the fix. Worked in 5 main browsers.

Diff:

Revision 2 (+22 -6)

Show changes

djblets/media/js/jquery.gravy.js

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?

hongbin June 18, 2011, 11:48 p.m.

In some browsers such as IE, keypress just captures the actual key. The keys such as "ctrl", "esc", and "enter" won't be captured.

hongbin June 19, 2011, 1:42 p.m.

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.

chipx86 June 27, 2011, 3:06 p.m.

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.

hongbin June 28, 2011, 1:41 a.m.

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.

hongbin June 28, 2011, 1:44 a.m.

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.

hongbin June 28, 2011, 2 a.m.

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.

chipx86 June 28, 2011, 7:29 p.m.

Someone recently reported that bug. I'll look into it.

chipx86 June 28, 2011, 7:39 p.m.

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.

chipx86 June 28, 2011, 7:39 p.m.

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.

chipx86 June 28, 2011, 7:40 p.m.

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.

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:

+	1958

Diff:

Revision 3 (+27 -4)

Show changes

djblets/media/js/jquery.gravy.js

Added Files:

Ship it!

```
Thanks! Committed to master (0fe4a2f)
```