The Ace editor will be way better than a textarea for editing code. We should integrate Ace but make sure that the same keyboard shortcuts and context menu for code evaluation/inspection and such work.

For reference, there's already on-going work to integrate Ace into Workspace. Here is the pull request: https://github.com/robcee/workspace/pull/22

The first WIP patch that uses the SourceEditor component from bug 660784. Things TODO: - add comments to the new methods. - use the insertText() API from SourceEditor, once that's available. - update/fix the Scratchpad mochitests. Concerns: - Notice how the configuration for keyboard shortcuts works. This is not ideal in my opinion. If I am not mistaken Orion does too much of event.preventDefault() which breaks some of our XUL-level keyboard shortcuts (ctrl-z, ctrl-i, and so on). I needed to add these to Orion via config. That's nice, but it's not nice when we have our own code that handles keyboard shortcuts. If you also consider the current solution is not ideal/nice, and that we need to change this to work "our way", then I can dive into the Orion source code and look into fixes that would allow us to override "their" keyboard shortcuts. Thoughts? - Also note that the textbox is hidden by the SourceEditor and we never really use it. I think we should really not have any textbox at all in Scratchpad. We shall leave it all up to the SourceEditor to add whatever it wants. Thoughts? Looking forward to your feedback. Thanks!

(In reply to comment #2) > Created attachment 536275 [details] [diff] [review] [review] > first WIP > > The first WIP patch that uses the SourceEditor component from bug 660784. > > Things TODO: > > - add comments to the new methods. > - use the insertText() API from SourceEditor, once that's available. > - update/fix the Scratchpad mochitests. I'll keep these in mind. > > Concerns: > > - Notice how the configuration for keyboard shortcuts works. This is not > ideal in my opinion. If I am not mistaken Orion does too much of > event.preventDefault() which breaks some of our XUL-level keyboard shortcuts > (ctrl-z, ctrl-i, and so on). I needed to add these to Orion via config. > That's nice, but it's not nice when we have our own code that handles > keyboard shortcuts. > > If you also consider the current solution is not ideal/nice, and that we > need to change this to work "our way", then I can dive into the Orion source > code and look into fixes that would allow us to override "their" keyboard > shortcuts. Thoughts? That sounds a little scary. It becomes a difficult proposition to know what keybindings Orion has "stolen" from our xul widgets. Anytime we embed this someplace, we'll need to dig deep to figure those out. > - Also note that the textbox is hidden by the SourceEditor and we never > really use it. I think we should really not have any textbox at all in > Scratchpad. We shall leave it all up to the SourceEditor to add whatever it > wants. Thoughts? Sounds removeable to me. More comments after I look at your patch.

(In reply to comment #3) > > Concerns: > > > > - Notice how the configuration for keyboard shortcuts works. This is not > > ideal in my opinion. If I am not mistaken Orion does too much of > > event.preventDefault() which breaks some of our XUL-level keyboard shortcuts > > (ctrl-z, ctrl-i, and so on). I needed to add these to Orion via config. > > That's nice, but it's not nice when we have our own code that handles > > keyboard shortcuts. > > > > If you also consider the current solution is not ideal/nice, and that we > > need to change this to work "our way", then I can dive into the Orion source > > code and look into fixes that would allow us to override "their" keyboard > > shortcuts. Thoughts? > > That sounds a little scary. It becomes a difficult proposition to know what > keybindings Orion has "stolen" from our xul widgets. Anytime we embed this > someplace, we'll need to dig deep to figure those out. Yeah. It's rather "ugly". I should take the time and see if we can disable the Orion keyboard shortcuts all-at-once, or something. I also expect we might be hit here by the problem that Orion uses (1) an iframe and (2) contentEditable. The former perhaps causes XUL key bindings to not work (maybe they don't work in the iframe). The latter can also cause issues - AFAIK the browser has default key bindings for, say, italic (Ctrl-I) or bold (Ctrl-B), and so on. Further investigation needed, anyhow. Thanks for your comment!

This is the latest WIP patch. This includes work I did about a week ago, when I switched to other higher priority work. Changes since the first patch: - API improvements. - bug fixes. - updated to the latest SourceEditor JSM. - removed the textbox. Still in TODO: - add comments to the new methods. - update/fix the Scratchpad mochitests. - address feedback comments.

I pushed this patch and the dependencies today to the try server. Here are the builds: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mihai.sucan@gmail.com-6e429b873c06/ (windows build is still running) David: please let us know your comments about the Orion accessibility in the Scratchpad tool. Thank you!

Proposed patch. Changes: - updated to the latest Orion integration patch. See attachment 540421 [details] [diff] [review] from bug 660784. - using native XUL keybindings only, no more Orion-based keybindings, thanks to an upstream fix. - added comments to the code as needed. - updated/fixed the Scratchpad tests.

New try builds: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mihai.sucan@gmail.com-bf4f5f31e258 Test results: http://tbpl.mozilla.org/?tree=Try&rev=bf4f5f31e258 All fine, as it seems.

Comment on attachment 540423 [details] [diff] [review] proposed patch > display: function SP_display() > { >- let selectionStart = this.textbox.selectionStart; >- let selectionEnd = this.textbox.selectionEnd; >- if (selectionStart == selectionEnd) { >- selectionEnd = this.textbox.value.length; >+ let selection = this.getSelectionRange(); >+ if (selection.start == selection.end) { >+ selection.end = this.editor.getCharCount(); > } > >- let [selection, result] = this.run(); >+ let [, result] = this.run(); nit: not sure if I have ever seen comma first in a throw-away array before. Maybe this is good form? not sure. This patch looks good, 99% of it seems to be trading our internal API for Orion's internal API. Side note (not sure if this matters, and is probably covered in that bug): will we ever need to also run the Orion test suite in conjunction with devtools tests?

Comment on attachment 540423 [details] [diff] [review] proposed patch adding ehsan for an i18n review

I will try to get to this as soon as possible, but it might take some time. :/

(In reply to comment #9) > >- let [selection, result] = this.run(); > >+ let [, result] = this.run(); > nit: not sure if I have ever seen comma first in a throw-away array before. > Maybe this is good form? not sure. I think I saw this in MDN once, but if it's wrong, please let me know and I'll change it. > This patch looks good, 99% of it seems to be trading our internal API for > Orion's internal API. True. Still, I tried to keep away from the most Orion-internal API. I just abstract it away such that for other editors ... one can use different implementations of the same API. It should work with Ace, for example. > Side note (not sure if this matters, and is probably covered in that bug): > will we ever need to also run the Orion test suite in conjunction with > devtools tests? I doubt. We have our tests that exercise the Orion integration API. We need to rely on upstream to provide a stable, usable releases. I believe that's part of the "deal" when we agree to integrate an upstream project. Thanks for your review+!

Comment on attachment 540423 [details] [diff] [review] proposed patch Asking for additional review from Gavin. Thank you!

(In reply to comment #8) > New try builds: > > http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mihai.sucan@gmail. > com-bf4f5f31e258 > > Test results: > http://tbpl.mozilla.org/?tree=Try&rev=bf4f5f31e258 > > All fine, as it seems. Pressing Enter after typing a reserved word messes up line numbering. Here's a screencast: http://dl.dropbox.com/u/22633148/code%20editor.ogv I'm using Ubuntu 11.04

(In reply to comment #12) > (In reply to comment #9) > > >- let [selection, result] = this.run(); > > >+ let [, result] = this.run(); > > nit: not sure if I have ever seen comma first in a throw-away array before. > > Maybe this is good form? not sure. > > I think I saw this in MDN once, but if it's wrong, please let me know and > I'll change it. It's odd-looking and a little too cute. Change it! :) [...] > > Side note (not sure if this matters, and is probably covered in that bug): > > will we ever need to also run the Orion test suite in conjunction with > > devtools tests? > > I doubt. We have our tests that exercise the Orion integration API. We need > to rely on upstream to provide a stable, usable releases. I believe that's > part of the "deal" when we agree to integrate an upstream project. I do think we should incorporate some set of the orion test-suite in our codebase, as a separate bug. reasons: 1. It'll give us another batch of content-editable tests. 2. It augments our test suite 3. We'll find out if we or anyone else checks something in that breaks our source editor. 3's the big one here. It's possible the editor could break in ways that our own tests don't test. > Thanks for your review+! Indeed. From a usability stand-point, I have a couple of concerns that we need to address: 1. Unix cursor movement keys behave strangely ctrl-a, ctrl-e from anywhere in a line, move the caret to the beginning/end, but typing is from the previous insertion point. 2. Unix editor keys behave strangely ctrl-d, ctrl-k, as above, you don't always get what you expect. 3. Nit: Should have a min-width on the gutter. single digit gutters look funny, imo. 4. Error on the Console: Error: child is undefined Source File: chrome://browser/content/orion.js Line: 4261

Comment on attachment 540423 [details] [diff] [review] proposed patch + self.editor.init(editorPlaceholder, config, self.onEditorLoad.bind(self)); That's a lot of selfs. Not a nit, just a comment. I have no real issues with any of this code, but I'm giving this a feedback- for now until we can figure out what to do about the unix control keys. (note, that problem exists on the mac, not sure about windows or linux yet).

(In reply to comment #14) > (In reply to comment #8) > > New try builds: > > > > http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mihai.sucan@gmail. > > com-bf4f5f31e258 > > > > Test results: > > http://tbpl.mozilla.org/?tree=Try&rev=bf4f5f31e258 > > > > All fine, as it seems. > Pressing Enter after typing a reserved word messes up line numbering. > Here's a screencast: > http://dl.dropbox.com/u/22633148/code%20editor.ogv > I'm using Ubuntu 11.04 Alex, could you try reproducing that on the online demo version of Orion to see if it exists there? http://eclipse.org/orion/ We might also want to do a quick search through their bugzilla to see what we can find about keyboard issues in Firefox.

(In reply to comment #17) > Alex, could you try reproducing that on the online demo version of Orion to > see if it exists there? > > http://eclipse.org/orion/ It's not reproducible in the online version. > > We might also want to do a quick search through their bugzilla to see what > we can find about keyboard issues in Firefox. I did a quicksearch and found nothing in their bugzilla. I'm going to digg through all of their Orion (New, Assigned and Reopened) bugs next, all 374 of them.

(In reply to comment #15) > I do think we should incorporate some set of the orion test-suite in our > codebase, as a separate bug. For sure. In fact, if I were you, I'd make that a requirement to land this patch...

(In reply to comment #19) > (In reply to comment #15) > > I do think we should incorporate some set of the orion test-suite in our > > codebase, as a separate bug. > > For sure. In fact, if I were you, I'd make that a requirement to land this > patch... The situation with their test suite is not very well defined at this point. I agree with Robert that this is something for a follow up bug. It needs more investigation - how we can do it, what subset of their tests we need, and so on.

(In reply to comment #20) > The situation with their test suite is not very well defined at this point. I > agree with Robert that this is something for a follow up bug. Would you care to elaborate please?

(In reply to comment #15) > From a usability stand-point, I have a couple of concerns that we need to > address: > > 1. Unix cursor movement keys behave strangely > ctrl-a, ctrl-e from anywhere in a line, move the caret to the beginning/end, > but typing is from the previous insertion point. Ctrl-A is taken by Select All. What do we do about this? In most editors, except vi and emacs, Ctrl-A is for select all. I think we don't have to make a niche editor here. We want something that works for common users. > 2. Unix editor keys behave strangely > ctrl-d, ctrl-k, as above, you don't always get what you expect. Ctrl-D for delete line, I assume, and Ctrl-K for Goto line. Right? I can add these. > 3. Nit: Should have a min-width on the gutter. > single digit gutters look funny, imo. Will do. > 4. Error on the Console: > Error: child is undefined > Source File: chrome://browser/content/orion.js > Line: 4261 This is a know bug. As far as I know it's fixed in the upcoming patch update. Please recheck this when I update the main patch from bug 660784. Thanks for your testing and comments!

(In reply to comment #21) > (In reply to comment #20) > > The situation with their test suite is not very well defined at this point. I > > agree with Robert that this is something for a follow up bug. > > Would you care to elaborate please? The answer I got from the orion-dev mailing list about Orion editor tests is as follows: > Sorry, we still do not have unit tests to exercise the editor API. The > performance and model API tests are not samples. They run as part of the orion > build. The performance tests also run from the sample page to make it easier > for anyone to try them out. > > All tests are run in the orion build using JSTestDriver. We can send you > a batch file to run them locally if you would like. (dated 27 may 2011, from Silenio Quarti, one of the Orion team members.) The performance tests mentioned are just that: some scripts that instance an Orion TextView and related Orion objects, run stress tests on some of the API... and time the results. These are not unit tests with pass and fail conditions. We can include these, if needed. Please let me know. (Please also note that we may not be able to keep up with upstream test changes once they expand their unit testing infrastructure. Depending on their decisions, it may require having the Orion server to properly exercise the Orion API. Which I wouldn't be surprised.)

(In reply to comment #23) > > The performance tests mentioned are just that: some scripts that instance an > ... erm, correction, that's a single JavaScript file.

(In reply to comment #23) > The performance tests mentioned are just that: some scripts that instance an > Orion TextView and related Orion objects, run stress tests on some of the > API... and time the results. These are not unit tests with pass and fail > conditions. > > We can include these, if needed. Please let me know. Hmm, I was more concerned with the correctness tests, but I think having the performance tests on Talos could also be helpful.

(In reply to comment #22) > (In reply to comment #15) > > 2. Unix editor keys behave strangely > > ctrl-d, ctrl-k, as above, you don't always get what you expect. > > Ctrl-D for delete line, I assume, and Ctrl-K for Goto line. Right? I can add > these. no, Ctrl-D is delete character to right of insertion point (delete next character). Ctrl-K is delete characters to the right of insertion point to end of line (kill-to-eol). These are Emacsisms. If possible, you should probably only do this for OS X. > > 4. Error on the Console: > > Error: child is undefined > > Source File: chrome://browser/content/orion.js > > Line: 4261 > > This is a know bug. As far as I know it's fixed in the upcoming patch > update. Please recheck this when I update the main patch from bug 660784. > > Thanks for your testing and comments! Will do.

Regarding the landing of the Orion test-suite, I agree that it should be a requirement of landing this. Otherwise we'll have no idea if some change breaks our editor. Having the Talos tests running might be helpful, but I wouldn't block on their inclusion. Mihai, can you file the bugs to get these included?

Updated the patch. - Rebased on top of the latest attachment 542833 [details] [diff] [review] from bug 660784 comment 13. - which should fix Mac key bindings and the "child is undefined" error. Robert, please retest. - which also adds debug output for allowing me to track down the problem from comment 14. - added Edit > Go to line... so users can quickly jump to any line number they want. Again, a matter of keyboard shortcuts users expect in an editor (Cmd/Ctrl+K). Looking forward for comments. Thank you!

Just pushed this patch and the dependency to the try server. Results: http://tbpl.mozilla.org/?tree=Try&rev=caf3cc2fde9d Builds and logs: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mihai.sucan@gmail.com-caf3cc2fde9d (when they all become available)

(In reply to comment #27) > Regarding the landing of the Orion test-suite, I agree that it should be a > requirement of landing this. Otherwise we'll have no idea if some change > breaks our editor. > > Having the Talos tests running might be helpful, but I wouldn't block on > their inclusion. > > Mihai, can you file the bugs to get these included? Reported bug 668320.

Comment on attachment 542835 [details] [diff] [review] updated patch On OS X at least, this latest patch works much better. Ctrl keys are doing what I'd expect. The extra spacing in the gutter looks good. I haven't been through the code in this patch fully but the interaction certainly feels imrpoved.

(loltyping)

Thank you for your testing, Rob! Please note that the try server test failures are now fixed in the local patches I have: http://tbpl.mozilla.org/?tree=Try&rev=449f079f988e http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mihai.sucan@gmail.com-449f079f988e I am not updating these patches here yet, because the changes are minor and because I am still waiting for some upstream Orion work.

Updated the patch. Simply rebased on top of the latest fxteam repo and on top of the latest attachment 544476 [details] [diff] [review] from bug 660784 comment 14.

Comment on attachment 544481 [details] [diff] [review] rebased patch giving feedback+ to reflect the patch's previous keyboard handling improvements.

Just a rebase on top of the latest attachment 549908 [details] [diff] [review] from bug 660784.

Try server run: http://tbpl.mozilla.org/?tree=Try&rev=25799973e301 New builds will be available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mihai.sucan@gmail.com-25799973e301 Please note that now the Orion code editor is behind a pref (enabled by default for now): devtools.codeeditor.enabled. See about:config.

Rebased the patch for the Scratchpad move to browser/devtools.

Rebased the patch on top of the latest fxteam, and on top of the latest patch from bug 660784. Also enabled es5 strict mode in scratchpad.js - had only one trivial change to make. Looking forward to your review. We would like to land this patch in time for Firefox 8. Thank you!

Rebased the patch. Looking forward to your review. Thank you!

Comment on attachment 551799 [details] [diff] [review] patch update 9 - rebase >diff --git a/browser/devtools/scratchpad/scratchpad.js b/browser/devtools/scratchpad/scratchpad.js >- get selectedText() >+ getSelectedText: function SP_getSelectedText() Why change this? Seems unnecessary (not really any advantage to matching the SourceEditor API here). >+ /** >+ * Get the entire source code from the editor. >+ * >+ * @return string >+ * The source code. >+ */ >+ /** >+ * Get the editor content, in the given range. If no range is given you get Something looks wrong here. Is there supposed to be a ".text" getter in the between these comments? > display: function SP_display() >+ let selection = this.getSelectionRange(); >+ if (selection.start == selection.end) { >+ selection.end = this.editor.getCharCount(); > } This would be a lot clearer as: let insertionPoint; let selection = this.getSelectionRange(); if (selection.start != selection.end) { // insert after selected text insertionPoint = selection.end; } else { // insert at end insertionPoint = this.editor.getCharCount(); } > onLoad: function SP_onLoad() > { >+ removeEventListener("DOMContentLoaded", SP_onLoad, false); This isn't necessary (this event won't fire twice). >+ gotoLine: function SP_gotoLine(aLine) >+ aLine = window.prompt(this.strings. >+ GetStringFromName("gotoLineCmd.prompt"), oldLine); You should probably use the prompt service for this (will give you a better window title). >-addEventListener("DOMContentLoaded", Scratchpad.onLoad.bind(Scratchpad), false); >+addEventListener("DOMContentLoaded", Scratchpad.onLoad, false); >+addEventListener("unload", Scratchpad.onUnload, false); Why remove the bind()? I think it makes the code easier to read (and you could do the same for onUnload). >diff --git a/browser/devtools/scratchpad/scratchpad.xul b/browser/devtools/scratchpad/scratchpad.xul >+ <key id="key_undo" key="&undoCmd.key;" modifiers="accel" >+ oncommand="Scratchpad.undo();"/> >+ <key id="key_redo" key="&undoCmd.key;" modifiers="accel,shift" >+ oncommand="Scratchpad.redo();"/> Shouldn't these be associated with the commands rather than duplicating oncommand? That also raises the issue that you're only updating these commands' state when the menu shows, when you should be editing it any time the state changes (so that the keys can be disabled accordingly). Is there a way to do that using editor events? >-<textbox id="scratchpad-textbox" >- placeholder="&textbox.placeholder1;" /> This string looks unused now (also maybe unused before...) r=me with these addressed.

(In reply to Gavin Sharp from comment #41) > Comment on attachment 551799 [details] [diff] [review] > patch update 9 - rebase > > >diff --git a/browser/devtools/scratchpad/scratchpad.js b/browser/devtools/scratchpad/scratchpad.js > > >- get selectedText() > >+ getSelectedText: function SP_getSelectedText() > > Why change this? Seems unnecessary (not really any advantage to matching the > SourceEditor API here). Will fix. > >+ /** > >+ * Get the entire source code from the editor. > >+ * > >+ * @return string > >+ * The source code. > >+ */ > >+ /** > >+ * Get the editor content, in the given range. If no range is given you get > > Something looks wrong here. Is there supposed to be a ".text" getter in the > between these comments? Hah, stray/obsolete comment that remained here. Will fix. (I assume this got here in the last patch rebase, when Scratchpad was moved to browser/devtools.) > > display: function SP_display() > > >+ let selection = this.getSelectionRange(); > >+ if (selection.start == selection.end) { > >+ selection.end = this.editor.getCharCount(); > > } > > This would be a lot clearer as: > let insertionPoint; > let selection = this.getSelectionRange(); > if (selection.start != selection.end) { > // insert after selected text > insertionPoint = selection.end; > } else { > // insert at end > insertionPoint = this.editor.getCharCount(); > } Good point. Will update. > > onLoad: function SP_onLoad() > > { > >+ removeEventListener("DOMContentLoaded", SP_onLoad, false); > > This isn't necessary (this event won't fire twice). Will fix. > >+ gotoLine: function SP_gotoLine(aLine) > > >+ aLine = window.prompt(this.strings. > >+ GetStringFromName("gotoLineCmd.prompt"), oldLine); > > You should probably use the prompt service for this (will give you a better > window title). Will update. > >-addEventListener("DOMContentLoaded", Scratchpad.onLoad.bind(Scratchpad), false); > >+addEventListener("DOMContentLoaded", Scratchpad.onLoad, false); > >+addEventListener("unload", Scratchpad.onUnload, false); > > Why remove the bind()? I think it makes the code easier to read (and you > could do the same for onUnload). Will fix. Please note that "DOMContentLoaded" fires more than once because Orion uses an iframe. Nonetheless, I can check the event target. > >diff --git a/browser/devtools/scratchpad/scratchpad.xul b/browser/devtools/scratchpad/scratchpad.xul > > >+ <key id="key_undo" key="&undoCmd.key;" modifiers="accel" > >+ oncommand="Scratchpad.undo();"/> > >+ <key id="key_redo" key="&undoCmd.key;" modifiers="accel,shift" > >+ oncommand="Scratchpad.redo();"/> > > Shouldn't these be associated with the commands rather than duplicating > oncommand? That also raises the issue that you're only updating these > commands' state when the menu shows, when you should be editing it any time > the state changes (so that the keys can be disabled accordingly). Is there a > way to do that using editor events? In Orion we do have the TextChanged event I can listen for. The textbox EditActionListener methods are not invoked when editor transactions are undone/redone, which is unfortunate, so I can't generate TextChanged events. Beyond that, we have EditObservers of some kind in nsIEditor which are very generic, they do not tell what happens, they are just fired... when undo/redo happens (weird). I can't build a TextChanged event based on these, unless I write the JS code that checks what has changed in the textarea value (ugly). I could generate a generic "undo/redo" event, but in Orion we do not have any such events. I would suggest leaving the code as-is, for now. Maybe report a follow up bug, to improve these things. Please let me know if this is acceptable. Thank you! > >-<textbox id="scratchpad-textbox" > > >- placeholder="&textbox.placeholder1;" /> > > This string looks unused now (also maybe unused before...) Will remove. > r=me with these addressed. Thank you very much for the review+!

Updated the patch to address the review comments. Reported follow up bug 678215.

As per updates in bug 660784 comment 552510: removed the gotoLine() method from Scratchpad and removed the associated option from the UI.

erm, that should've been bug 660784 attachment 552510 [details] [diff] [review] (from comment 38).

Comment on attachment 552511 [details] [diff] [review] [checked-in] patch update 11 - remove gotoLine() Landed: http://hg.mozilla.org/integration/fx-team/rev/f85e000407f3

Comment on attachment 552511 [details] [diff] [review] [checked-in] patch update 11 - remove gotoLine() hg.mozilla.org/mozilla-central/rev/f85e000407f3

changed devtools.editor.component from textarea to orion and opened a scratchpad. Orion was there! [09:24:04.061] "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0a1) Gecko/20110814 Firefox/8.0a1"