Code snippet plugin

[Olek Nowodziński]

A brand-new widgets-based plugin to make insertion of rich code snippets into editor contents nice and easy.

Key features:

1. Syntax highlighting.
   • Server-side?
     • A generic architecture working with different server-side libraries (e.g. configurable URL and POST data).
     • Geshi as default?
     • Note: Server-side highlighting requires AJAX POST (see CKEDITOR.ajax.post in linked mockup).
   • Client-side syntax highlighting support (unlikely)?
2. Dialog-based editing.
   • Mostly due to lack of focusmanager for widgets. Widgets lose focus if <select> is clicked.
3. Support for different programming languages.
   • A select field to switch between languages.
4. Downcast to

   <pre><code [class="language-*"]>...</code></pre>
   

5. Simple and fast testing
   • The architecture of the plugin should be independent and possible to test without any coloring library.
6. Pretty sample :)
7. KISS. Code-size matters.

---

​Existing mockup created during DrupalCon Prague 2013 sprint.

[Marek Lewandowski]

Pushed to ​snippet.

Config variables in snippet plugin:

• snippet_langs - Object allowing to override languages available in highlighter
• snippet_template - (client-side only) allows to specify custom template for the plugin, see ​dev sample
• snippet_class - allows to specify custom class which will be given to <pre/> tag inside editable (while editing)

Knwon issues:

• no icon in toolbar
• if code snippet will be empty, it will be removed after going to source mode and back to wysiwyg - we decided that snippet can not be empty
• tab handling within code edit dialog - it goes to next focusable item. For code snippets tabs are _crucial_ so i belive w should figure out way to provide both - keyboard navigation, and ability to put tabs - we added a hotkey: ctrl+alt+t
• plugin should be renamed from snippet to codesnippet
• tests needs to be created for both snippet, and snippetgeshi plugins

[Marek Lewandowski]

@todo: Since code snippets plugin already has its ticket, its branch should be renamed to t/11480

[Marek Lewandowski]

You can use a ​sample to test plugins functionality.

I've created a ticket branch ​t/11480, and removed old snippet branch.

[Piotrek Koszuliński]

Force pushed rebased branches.

[Piotrek Koszuliński]

WOW. This is a really cool plugin :) As soon as it will be released we should use it on our forum.

I've made a quick review. I'll continue it next week. For now:

1. "Note: you may use CTRL + ALT + T hotkey to insert a tab character."

On my OS this opens a terminal :D And I started to think that we need to override TAB's navigation behaviour. To not conflict with dialog too much we can make it this way:

• When you focus <textarea> the screen reader says "In this field TAB inserts indentation characters. To unblock navigational role of TAB press ESC (or some shortcut)". From now on, until ESC is pressed, TAB inserts tabs (or other configurable characters).
• When user presses ESC once, the tab is unblocked until he enters textarea again. Subsequent ESC press may also block TAB again, but not necessarily.
• We can change the tip below <textarea> so it says whether TAB is locked or not.
• Note that SHIFT+TAB should not be locked so if users gets stuck in the <textarea> (he could forget which key unblocks TAB), he can escape by backing away.
• It's pretty easy to implement, because we may block TAB keystroke with high priority listener removing itself on ESC press.

2. Why height:440 in the samples? :D

3. Sample is not added to samples index.

4. The code snippet dialog is not very resizable. But it's not critical - it can be improved in the future. Now we should only fix dialog size on smaller resolutions, because it is affected by viewport height (only on page load though), but the minimal height is too big IMO.

5. [Chrome at least] CTRL+R is locked when in the dialog.

6. CKEDITOR.plugins.codesnippet lacks docstring.

7. ​https://github.com/cksource/ckeditor-dev/blob/9a5bcfecfc353c5bf47761316eff500f8d3d15e6/plugins/codesnippet/plugin.js#L219-L223 - better to use parent.add( child ).

8. ​https://github.com/cksource/ckeditor-dev/blob/9a5bcfecfc353c5bf47761316eff500f8d3d15e6/plugins/codesnippet/plugin.js#L201 - you cannot use classes - it's a garbage after ACF. It may not be available (e.g. if ACF was disabled).

9. ​https://github.com/cksource/ckeditor-dev/blob/9a5bcfecfc353c5bf47761316eff500f8d3d15e6/plugins/codesnippet/plugin.js#L145-L146 - is it ok? They look as not compatible and not precise.

10. ​https://github.com/cksource/ckeditor-dev/blob/9a5bcfecfc353c5bf47761316eff500f8d3d15e6/plugins/codesnippet/plugin.js#L185-L189 - do you have a TC for that?

11. ​https://github.com/cksource/ckeditor-dev/blob/9a5bcfecfc353c5bf47761316eff500f8d3d15e6/plugins/codesnippet/plugin.js#L191-L197 - it will be much better to use regexp to scan classes string ;).

12. Should we handle somehow pasted <pre><code> elements? What'd happen if someone pasted a code snippet from another page?

13. Code style:

• ​https://github.com/cksource/ckeditor-dev/blob/9a5bcfecfc353c5bf47761316eff500f8d3d15e6/plugins/codesnippet/plugin.js#L105-L113 - indentation and missing new line before defaults.
• ​https://github.com/cksource/ckeditor-dev/blob/9a5bcfecfc353c5bf47761316eff500f8d3d15e6/plugins/codesnippet/plugin.js#L76 - too long.

[Frederico Caldeira Knabben]

Replying to Reinmar:

1. "Note: you may use CTRL + ALT + T hotkey to insert a tab character."

On my OS this opens a terminal :D And I started to think that we need to override TAB's navigation behaviour. To not conflict with dialog too much we can make it this way:

• When you focus <textarea> the screen reader says "In this field TAB inserts indentation characters. To unblock navigational role of TAB press ESC (or some shortcut)". From now on, until ESC is pressed, TAB inserts tabs (or other configurable characters).
• When user presses ESC once, the tab is unblocked until he enters textarea again. Subsequent ESC press may also block TAB again, but not necessarily.
• We can change the tip below <textarea> so it says whether TAB is locked or not.
• Note that SHIFT+TAB should not be locked so if users gets stuck in the <textarea> (he could forget which key unblocks TAB), he can escape by backing away.
• It's pretty easy to implement, because we may block TAB keystroke with high priority listener removing itself on ESC press.

Ok, let's have TAB to insert tabs... but we should avoid breaking yet another key. So ESC is not a good option. Let's leave its default behavior to happen and instead have an additional keystroke to move out of the field. I would go with CTRL+. (period).

[Marek Lewandowski]

1. Yea i agree that we might rethink tabulation / focus thing. In addition to that i think that it would be great idea to provide an access key for at least soude code textarea.
2. It is ment to be only for testing phase - having it set to at least 440 i can see few widget instances without scrolling.
3. -- will do --
4. Then we'll need to put some extra attention over there. Currently i've simply copied sourcedialog code there. : )
5. Hmmm chrome@Win7 does handle ctrl + R in default way - that's it will refresh the page. Please clarify behaviour that you'd expect.
6. -- will do --

regarding 12. That's actually a good question, i've checked it with chrome atm, and it does handle <pre><code>(...)</code></pre> elements out of a box, thought i did not test other browsers so far.

As for further code requests I will apply them, and ask for further feedback if will be needed.

Followup to point 1. We decided to:

• allow tab key to insert tabulation character
• bind next focus to ctrl + . (period)
• leave shift + tab for browser default function (move focus to previous element)

[Marek Lewandowski]

Another part of feedback:

7. better to use parent.add( child ) - done
8. you cannot use classes - it's a garbage after ACF. It may not be available (e.g. if ACF was disabled). - done
9. is it ok? They look as not compatible and not precise. - could you elaborate a little on that?
10. do you have a TC for that? - pending
11. it will be much better to use regexp to scan classes string ;). - done

13. Code style: indentation and missing new line before defaults. - done

too long. - done

In addition to that i've noticed that undo of textarea gets messed up after tab insertion in a way that's currently done. We'll have to try do something about it.

[Marek Lewandowski]

Important info: for time being we dropped idea for enhanced tab support for textarea, coz initial implementation had bad integration with builtin undo (this within textarea).

There were more-or-less no problems with implementing that for Chrome/IEs, but FF was hard to fix, so we dropped it, to develop it later. All the code went to t/11480b branch.

[Marek Lewandowski]

This issue also depends on #11727

[Marek Lewandowski]

Plugin is ready for review, the only issue is FF problem, described in #11727, after it's being merged to major, we'll rebase onto it.

[Marek Lewandowski]

Tab handling issue was create in #11735.

[Piotrek Koszuliński]

Missing icon extracted to #11736.

[Marek Lewandowski]

Ahh, and i think it would be great idea to limit toolbar in sample, coz user will have hard times with finding an icon. Adding following part to config:

toolbar: [ [ 'Source', 'Bold' ], [ 'CodeSnippet' ] ]

should do it.

[Piotrek Koszuliński]

1. No documentation and no tests for ajax.post.
2. The configuration options should be called codeSnippet_*.
3. Code snippet plugin does not reuse the editor.addContentsCss method.
4. The code snippet styles which you add through addCss should land in a stylesheet file and be added using editor.addContentsCss. Additionally, they should not use .cke_* class - they are style for people not our internal thing. So in other words - simply create generic pre element styles.
5. ensurePluginNamespaceExists does not have to exist - just create the namespace in beforeInit or even in init, because specific highlighters have to require codesnippet plugin anyway, so their code will be executed later.
6. ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L120 use function definition instead of function expression.
7. Wrong type of comment: ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L186
8. ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L190 use setHtml. You can also merge this case with the previous one and only process formattedCode conditionally.
9. The code execute this fuctnion for every single element in the editor: ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L214. It should only be called if we're in <pre>. The same applies to accessing the class property.
10. Why do we need div.cke_snipper_wrapper? ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L235-L237
11. The default highlighter code should be pulled from codesnippet code - e.g. you can place it at the end. Now it's intermixed with the rest and it's hard to tell which part is codesnippet's part.
12. There's an addClass method ;) ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L229
13. Again, use function definition: ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L261
14. To be removed: ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L318-L326
15. Remember, remember about not putting empty line after function() { (the only exception may be some huge wrapping IIFE).
16. ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippetgeshi/plugin.js#L42
17. ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippetgeshi/plugin.js#L36 You should use basic writer to ​http://docs.ckeditor.com/#!/api/CKEDITOR.htmlParser.element-method-writeChildrenHtml writeChildrenHtml].
18. Use camel case: ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippetgeshi/plugin.js#L51
19. ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L79 Why is this an CKEDITOR's object? This should be placed in editor.plugins.codesnippet by simply defining these methods in plugin definition. The reason is that those methods modifies editor instance, not CKEDITOR object.
20. In the sample - make the "Remaining part is resposible.." (BTW. typo) more visible, because now it looks too much like editors' startup logic.
21. The toolbars can indeed be simplified.

[Olek Nowodziński]

Rebased branches on major.

[Olek Nowodziński]

Development in ​branch:t/11480b (+tests). I refactorized all plugin files and tests.

What to do with codesnippetgeshi sample? (as the GeSHi library is out of repository).

Replying to Reinmar:

1. No documentation and no tests for ajax.post.

Pending.

2. The configuration options should be called codeSnippet_*.

Done.

3. Code snippet plugin does not reuse the editor.addContentsCss method.

Done.

4. The code snippet styles which you add through addCss should land in a stylesheet file and be added using editor.addContentsCss. Additionally, they should not use .cke_* class - they are style for people not our internal thing. So in other words - simply create generic pre element styles.

Done.

5. ensurePluginNamespaceExists does not have to exist - just create the namespace in beforeInit or even in init, because specific highlighters have to require codesnippet plugin anyway, so their code will be executed later.

Done.

7. ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L120 use function definition instead of function expression.

Done.

6. Wrong type of comment: ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L186

Done.

8. ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L190 use setHtml. You can also merge this case with the previous one and only process formattedCode conditionally.

Done.

9. The code execute this fuctnion for every single element in the editor: ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L214. It should only be called if we're in <pre>. The same applies to accessing the class property.

Done.

10. Why do we need div.cke_snipper_wrapper? ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L235-L237

Done. I changed widget structure to pre > code (just like input HTML).

11. The default highlighter code should be pulled from codesnippet code - e.g. you can place it at the end. Now it's intermixed with the rest and it's hard to tell which part is codesnippet's part.

Done??

12. There's an addClass method ;) ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L229

Done.

13. Again, use function definition: ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L261

Done.

14. To be removed: ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L318-L326

Done.

15. Remember, remember about not putting empty line after function() { (the only exception may be some huge wrapping IIFE).

Done.

16. ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippetgeshi/plugin.js#L42

Done?

17. ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippetgeshi/plugin.js#L36 You should use basic writer to ​http://docs.ckeditor.com/#!/api/CKEDITOR.htmlParser.element-method-writeChildrenHtml writeChildrenHtml].

Done.

18. Use camel case: ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippetgeshi/plugin.js#L51

Done.

19. ​https://github.com/cksource/ckeditor-dev/blob/t/11480/plugins/codesnippet/plugin.js#L79 Why is this an CKEDITOR's object? This should be placed in editor.plugins.codesnippet by simply defining these methods in plugin definition. The reason is that those methods modifies editor instance, not CKEDITOR object.

Done.

20. In the sample - make the "Remaining part is resposible.." (BTW. typo) more visible, because now it looks too much like editors' startup logic.

Done.

21. The toolbars can indeed be simplified.

Done.

[Marek Lewandowski]

Some extra points which i made yesterday with Fred:

• before adding CKEDITOR.ajax.post to public, we must change its internal behavior, it should not perform json serialization on its input data, it should be mediaembed job to do so
• we need to remove geshi sample from codesnippet sample, but we may place an information about it, and a link to codesnipetgeshi at our add-ons repo
• in codesnipetgeshi we need to remove geshi library itself, and provide a config variable (in this plugin) allowing to override library location
• we should create a file readme.md inside codesnippetgeshi with guides for developer, telling how to setup things. We might include php script content, which was initially placed inside that lib directory.

[Frederico Caldeira Knabben]

contents.css

• We should not add styles to <pre>, because it toches all PREs, including those which are not for the codesnippet plugin.

plugins/ajax/plugin.js

• CKEDITOR.ajax.post, should not JSON.stringify data internally before posting. It should instead post data as is. If data must be json, then this should be done before calling post(). (mentioned in comment:21)

[Olek Nowodziński]

​branch:t/11480b (+tests).

[Olek Nowodziński]

Sample to test Code Snippet Geshi

[Piotrek Koszuliński]

1. Open sample, doubleclick snippet, edit code, close, you cannot undo that.
2. Open sample, drag snippet, undo, you cannot redo.
3. I think that both editors in the sample should use the same theme.
4. I don't like the default hjs's theme... What do you think about choosing different one by default in sample? One with a dark bg of course ;). E.g. monokai_sublime?
5. It's not mentioned in codesnippetgeshi's README where to get the GeSHi from.
6. pre and pre>code.hjs are defined in contents.css. While the second style cannot be kept there... why not styling pre by default in editor? Maybe it shouldn't be so drastic (dark bg), but some basic styling would be good and codesnippet would benefit from it too.

[Piotrek Koszuliński]

1. No tests for codesnippetgeshi at all. No sample... How can we be sure that it works.
2. The fact that setHighlighter() changes config object is... rather ugly. The languages are kept in editor._.codesnippet.highlighter so if setHighlighter is used then they should be used if config.codeSnippet_languages is not set. If config is set, then config should be used even if custom highlighter was defined.
3. It's better to add button and widget in init rather than in afterInit.
4. There's no explanation in the inline editor section in the sample that developer needs to add theme's CSS manually.
5. In the plugins.highlighter it's not explained what developer can do with that highlighter instance then. There should be link to setHightligher (and vice versa) and a code sample showing how to use setHighligher (it may be confusing where it is). To avoid duplication you can have sample in one method's doc and link to it from the second one.
6. You're passing also languages property to highlighter constructor but it's not mentioned in the @param def.
7. Typo: ​https://github.com/cksource/ckeditor-dev/blob/55203f99d2f17be13d105b64951a7062c453d1ea/plugins/codesnippet/plugin.js#L186
8. Use ​copy, Luke. ​https://github.com/cksource/ckeditor-dev/blob/55203f99d2f17be13d105b64951a7062c453d1ea/plugins/codesnippet/plugin.js#L304
9. Why do we do this? ​https://github.com/cksource/ckeditor-dev/blob/55203f99d2f17be13d105b64951a7062c453d1ea/plugins/codesnippet/plugin.js#L326 Please add a comment that it's a editable-only class. The config option doc could als explain what's it for.
10. Missing spaces. ​https://github.com/cksource/ckeditor-dev/blob/55203f99d2f17be13d105b64951a7062c453d1ea/plugins/codesnippet/plugin.js#L360
11. Well... this is even funny: ​https://github.com/cksource/ckeditor-dev/blob/55203f99d2f17be13d105b64951a7062c453d1ea/plugins/codesnippet/plugin.js#L363-L366
12. ​https://github.com/cksource/ckeditor-dev/blob/55203f99d2f17be13d105b64951a7062c453d1ea/plugins/codesnippet/plugin.js#L380 why not mention in setHighlighter/highlighter contstructor doc. A developer implementing his highlighter may stumble upon it there, not here.

[Olek Nowodziński]

Changes pushed to ​branch:t/11480b (+tests)

Replying to Reinmar:

1. Open sample, doubleclick snippet, edit code, close, you cannot undo that.

Pending.

2. Open sample, drag snippet, undo, you cannot redo.

Pending.

3. I think that both editors in the sample should use the same theme.

Done.

4. I don't like the default hjs's theme... What do you think about choosing different one by default in sample? One with a dark bg of course ;). E.g. monokai_sublime?

Done.

5. It's not mentioned in codesnippetgeshi's README where to get the GeSHi from.

Yes, it is. In the very first paragraph.

6. pre and pre>code.hjs are defined in contents.css. While the second style cannot be kept there... why not styling pre by default in editor? Maybe it shouldn't be so drastic (dark bg), but some basic styling would be good and codesnippet would benefit from it too.

We decided that styles for pre make no longer sense because they may collide with other plugins. As for pre > code.hjs, those styles depend on highlight.js and, if added via CKEDITOR.addCss they would be wrong as there could be some custom highlighter in use. pre > code.hjs would fit only in codesnippethighlightjs plugin, which would hold all the logic related to highlight.js and which, unfortunately, does not exist yet.

So the conclusion is that Code Snippet will not introduce any styles.

---

Replying to Reinmar:

1. No tests for codesnippetgeshi at all. No sample... How can we be sure that it works.

Pending.

2. The fact that setHighlighter() changes config object is... rather ugly. The languages are kept in editor._.codesnippet.highlighter so if setHighlighter is used then they should be used if config.codeSnippet_languages is not set. If config is set, then config should be used even if custom highlighter was defined.

It already works like that ;)

3. It's better to add button and widget in init rather than in afterInit.

Done.

4. There's no explanation in the inline editor section in the sample that developer needs to add theme's CSS manually.

Done.

5. In the plugins.highlighter it's not explained what developer can do with that highlighter instance then. There should be link to setHightligher (and vice versa) and a code sample showing how to use setHighligher (it may be confusing where it is). To avoid duplication you can have sample in one method's doc and link to it from the second one.

Done.

6. You're passing also languages property to highlighter constructor but it's not mentioned in the @param def.

It has already been mentioned.

7. Typo: ​https://github.com/cksource/ckeditor-dev/blob/55203f99d2f17be13d105b64951a7062c453d1ea/plugins/codesnippet/plugin.js#L186

Done.

8. Use ​copy, Luke. ​https://github.com/cksource/ckeditor-dev/blob/55203f99d2f17be13d105b64951a7062c453d1ea/plugins/codesnippet/plugin.js#L304

Done.

9. Why do we do this? ​https://github.com/cksource/ckeditor-dev/blob/55203f99d2f17be13d105b64951a7062c453d1ea/plugins/codesnippet/plugin.js#L326 Please add a comment that it's a editable-only class. The config option doc could als explain what's it for.

Done. I renamed the property to config.codeSnippet_codeClass in case we wanted to introduce some class for <pre>. Also extended docs.

10. Missing spaces. ​https://github.com/cksource/ckeditor-dev/blob/55203f99d2f17be13d105b64951a7062c453d1ea/plugins/codesnippet/plugin.js#L360

Done.

11. Well... this is even funny: ​https://github.com/cksource/ckeditor-dev/blob/55203f99d2f17be13d105b64951a7062c453d1ea/plugins/codesnippet/plugin.js#L363-L366

Pending.

12. ​https://github.com/cksource/ckeditor-dev/blob/55203f99d2f17be13d105b64951a7062c453d1ea/plugins/codesnippet/plugin.js#L380 why not mention in setHighlighter/highlighter contstructor doc. A developer implementing his highlighter may stumble upon it there, not here.

Done.

[Piotrek Koszuliński]

Merged to major ​git:620a6b4 on dev c3323dd on tests.

There are couple of topics, which we need to check before release - e.g. #11781, #11782, name in the elements path. Additionally, I found that because of limitations created by undo manager, we are forced to do very uncool and potentially dangerous for performance lock&unlock for every code snippet.