Last Comment Bug 683172 - Source Editor should automatically set up Undo/Redo key bindings
: Source Editor should automatically set up Undo/Redo key bindings
Status: RESOLVED FIXED
[sourceeditor][good first bug][mentor...
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: P3 normal (vote)
: Firefox 11
Assigned To: Mihai Sucan [:msucan]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-30 08:50 PDT by Cedric Vivier [:cedricv]
Modified: 2012-05-04 12:49 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Source Editor should automatically set up Undo/Redo key bindings - b=683172 r=msucan (1.34 KB, patch)
2011-08-30 08:57 PDT, Cedric Vivier [:cedricv]
no flags Details | Diff | Review
Source Editor should automatically set up Undo/Redo key bindings - b=683172 r=msucan (2.04 KB, patch)
2011-08-30 09:04 PDT, Cedric Vivier [:cedricv]
mihai.sucan: review-
Details | Diff | Review
proposed patch (4.65 KB, patch)
2011-12-12 13:33 PST, Mihai Sucan [:msucan]
rcampbell: review+
Details | Diff | Review

Description Cedric Vivier [:cedricv] 2011-08-30 08:50:42 PDT
Of the following basic editing features that should be default:

cut/copy/past/select all/undo/redo  

The two latter (Undo and Redo) are not supported automatically by SourceEditor currently.
Orion Key bindings should probably be registered in _initEditorFeatures... it might be worth to expose a disableEditorFeatures boolean to config object so that a consumer of SourceEditor can get full control if needed.
Comment 1 Cedric Vivier [:cedricv] 2011-08-30 08:57:31 PDT
Created attachment 556863 [details] [diff] [review]
Source Editor should automatically set up Undo/Redo key bindings - b=683172 r=msucan
Comment 2 Cedric Vivier [:cedricv] 2011-08-30 09:04:01 PDT
Created attachment 556868 [details] [diff] [review]
Source Editor should automatically set up Undo/Redo key bindings - b=683172 r=msucan
Comment 3 Mihai Sucan [:msucan] 2011-08-30 09:10:39 PDT
Comment on attachment 556868 [details] [diff] [review]
Source Editor should automatically set up Undo/Redo key bindings - b=683172 r=msucan

Thanks for your bug report and quick patch! Quick comments:

- key bindings need to be localizable.
- please use "undo" and "redo" as action names (standard in upstream Orion).
- we need a test as well.
- please update scratchpad accordingly. I assume you need to remove the <key> xul elements from scratchpad.xul.

Giving r- for now. Looking forward for the updated patch. Thanks again!
Comment 4 Mihai Sucan [:msucan] 2011-08-30 09:12:21 PDT
Not sure, but maybe we need to use the system default undo/redo keys. Rob, any thoughts?
Comment 5 Rob Campbell [:rc] (:robcee) 2011-09-02 05:05:56 PDT
System default, you mean OS defaults or Gecko defaults? If you mean Gecko defaults for whichever locale it's in, then yes.
Comment 6 Rob Campbell [:rc] (:robcee) 2011-09-02 05:06:49 PDT
little book-keeping here so we know who's working on this.
Comment 7 Mihai Sucan [:msucan] 2011-10-07 12:47:25 PDT
Rob: I meant Gecko defaults.
Comment 8 Rob Campbell [:rc] (:robcee) 2011-10-11 11:43:01 PDT
yes, should use gecko defaults.
Comment 9 Dave Camp (:dcamp) 2011-10-27 09:14:28 PDT
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.
Comment 10 Mihai Sucan [:msucan] 2011-12-12 13:33:09 PST
Created attachment 581033 [details] [diff] [review]
proposed patch

Proposed patch. Things to note:

- I looked into how I can use the Gecko default keybindings and it seems that the textarea implementation is pretty much hard coded (ctrl/cmd-z and ctrl/cmd-shift-z).

- in scratchpad.xul we need to keep the <key> element, so we can have the shortcuts visible in the menus.

Please let me know if this is fine. Looking forward to your review. Thanks!


I looked into adding an nsIController for the SourceEditor, but it didn't work well, so I did bail out for now. It would be "over-engineered" for this bug alone. Anyhow, I think it might make sense, for the future, to have an nsIController that calls the Orion API for the most common cmd_* stuff. Is this worth a separate bug, what do you think?


(Cedric has agreed with me taking this bug.)
Comment 11 Mihai Sucan [:msucan] 2011-12-15 05:03:19 PST
Thank you Rob!
Comment 12 Rob Campbell [:rc] (:robcee) 2011-12-16 06:03:45 PST
https://hg.mozilla.org/integration/fx-team/rev/c135571daddf
Comment 13 Rob Campbell [:rc] (:robcee) 2011-12-16 11:22:03 PST
https://hg.mozilla.org/mozilla-central/rev/c135571daddf
Comment 14 Mihai Sucan [:msucan] 2012-02-09 11:24:11 PST
The addition of the default Undo/Redo keyboard shortcuts in Firefox 11 to the Source Editor should be documented. Thank you!
Comment 15 Eric Shepherd [:sheppy] 2012-05-04 12:49:21 PDT
Documented:

https://developer.mozilla.org/en/Tools/Using_the_Source_Editor#Keyboard_commands

Note You need to log in before you can comment on or make changes to this bug.