Allow list-only indentation

[Piotrek Koszuliński]

Currently indent plugin allows two things:

• indenting block elements like <p>, <h1>, etc. either with margin-left style or classes defined in config.indentClasses,
• creating nested (indented) lists structures (e.g ul > li > ol > li).

These features should be split (to two separate plugins?) or option allowing to disable one of them should be introduced (e.g. config.indentOnly='lists|blocks'?).

[Olek Nowodziński]

Extracted from #10192:

• #10192 introduces p || li as a requiredContent for indent plugin.
• Indent is a dependency of the list plugin.

The above are quick patches to let enterkey close lists correctly. The reasoning is as follows:

1. Enterkey depends on indent to close lists (it uses outdent command).
2. Indent has requiredContent of p{margin-left}.
3. ...so setting config.allowedContent = 'p ul ol li' disables indent (no margin-left)
4. ...and disables closing lists as well (1.)

Although #10192 solves the issue, indent's requiredContent = [ 'p{margin-left}', 'li' ] brings some troubles.

Since #10192, setting config.allowedContent = 'p li' enables p indentation without explicit config.allowedContent = '...p{margin-left}...'. This is why indent plugin must be smarter to consider ACF rules when enabling/disabling commands. This implies a major refactorization of indent's refresh and exec methods to handle several new cases.

The main inconvenience is that list indentation and block indentation should be handled separately by list/indent and share the same button set. The idea is as follows:

1. Create separate commands: indent|outdentList and indent|outdentBlock in separate plugins (list, indent).
2. Create "core" commands and events: indent|outdent.
3. Let the indent|outdent commands control toolbar buttons.
4. Clicking toolbar button would fire indent|outdent events which, passed or canceled by the listeners, would do the work. List and indent plugins would listen on indent|outdent events and decide (considering ACF) whether any action can be performed:
   • Action possible: do the action (exec indent|outdentList|Block), cancel the event.
   • Action impossible: just pass the event.
5. indent|outdentList and indent|outdentBlock would fire indent|outdentRefresh event which, handled by indent|outdent commands in core, would enable or disable them (+buttons). indent|outdentRefresh would basically provide information about indent|outdent states as seen from indent|outdentList and indent|outdentBlock commands.

"Core" indent|outdent commands and events can be eventually delegated to a separate plugin (let's call it indentUi) which would also create toolbar buttons. This will let us avoid redundant button invokation code in list and indent. This has also some other advantages:

• With #8244, UI buttons might no longer be essential for indentation.
• CKEditor indent becomes scalable. New plugins can also listen on indent|outdent events, do custom job and control UI interface states.

                                                       action                          action
                                                          ^                              ^
                                                          |                              |
+----+    execute   +------------+  in|outdent evt  +-----+-----+  in|outdent evt  +-----+-----+  in|outdent evt
|    |+------------>|            |+---------------->|           |+- - - - - - - - >|           |+- - - - - - - - - . . .
| UI |              |  indentUi  |                  | plugin #1 |                  | plugin #2 |
|    |<------------+|            |                  |           |                  |           |
+----+   set state  +------------+                  +-----------+                  +-----------+
                           ^                              +                              +
                           |    in|outdentRefresh evt     |                              |
                           +-------------------------------------------------------------+------------------------ . . .

[Frederico Caldeira Knabben]

Can't we avoid the core stuff, using the very same proposal. At this point we would have this:

• "indent" plugin that creates the buttons and delivers the "core" part of the indentation stuff, firing events and doing the generic job.
• "indentlist" and "indentblock" plugins, doing what they're suppose to do, using the "indent" plugin API. Both would depend on "indent".

Still one thing is not clear... what abou the ENTER key? Would it simply depend on "indentlist"? Sounds like an ok way for it.

[Piotrek Koszuliński]

Exactly - there's no need to add anything to core. Indent buttons and commands should be added by "indent" plugin required by "indentlist" and "indentblock" plugins.

The enter key IMO needs just a few lines of the current "outdent" command. It uses it just for simplicity. I'm for fully independent commands, because "indentlist" would have to require "indent" too and most likely "list" as well!. So all these plugins would be required by "enter" plugin? Noo...

Especially that we were considering merging "enter" plugin into the core (by creating a class grouping keyboard handlers which are now spread across editable class and enter plugin) and core cannot require a plugin.

[Piotrek Koszuliński]

#7006 case to be fixed in this ticket.

[Olek Nowodziński]

Created branches t/10027 in ​dev and tests.

Dev

t/10027 introduces a new indentation system with three separate plugins: indent, indentlist and indentblock.

• indent plugin: handles UI (buttons), defines generic indent and outdent commands that fire exec and refresh events handled by content-specific commands.

• indentlist plugin: handles list indentation (<ul>, <ol>), defines indentlist and outdentlist commands. This plugin can nest lists and collapse them into paragraphs. It brings TAB key support for indenting and outdenting lists.

• indentblock plugin: handles generic indentation (with CSS margin-left|right) of blocks (also <ul> and <ol> when nesting isn't possible), defines indentblock and outdentblock commands. It brings TAB key support for indenting and outdenting lists with CSS margin-left|right.

t/10027 also brings indent dev sample for easy testing.

In this ticket I focused on porting previous functionality which, even if sometimes buggy (#7006), is still predictable and users don't complain about it.

Tests

Significantly increased test coverage (previously only edge cases), including:

• lists
• various blocks (paragraphs, divs, tables)
• indent classes
• indent units
• initial command states
• ENTER_BR behavior
• keystrokes
• indentlist and indentblock-only cases

Note: two tests will fail until #10439 is resolved - this is intentional.

Issues

I'm still wondering whether indentlist, outdentlist, indentblock and outdentblock should be editor commands or not. They can be independently called and used but i.e. they don't revert selection (indent plugin does) which makes them hard to use. I'm for having them outside of command system to not to confuse users.

Misc

This ticket resolves #8244.

[Wim Leers]

I couldn't find any bugs. I tried every permutation of actions I could think of (which is probably not every possible permutation, but still). I checked the markup, the tabbing behavior and button states.

Finally, this indeed solves #8244.

Wonderful :)

[Olek Nowodziński]

I'm glad to hear that. However I'm pretty sure that Fred is gonna find something odd ;) Some code refactoring is still possible.

[Frederico Caldeira Knabben]

I see that you're bringing changes to the enter key code but no new test for it.

I'm not stating that anything is wrong but it would be good the check the status of the tests and eventually add new ones, if necessary.

[Olek Nowodziński]

Rebased dev and tests branches on latest major.

[Olek Nowodziński]

Replying to fredck:

I see that you're bringing changes to the enter key code but no new test for it.

I'm not stating that anything is wrong but it would be good the check the status of the tests and eventually add new ones, if necessary.

You were right. I created several tests for enterkey in lists and found out that it's broken. I created more tests that revealed that even the original implementation was buggy. E.g.:

• When splitting a list into two separate lists (enter in empty element, neither first or last child), the id attribute was preserved for both of them, bringing two nodes of the same id into DOM.

• Using enter in the first (empty) element of the list, which is inside of another list, moved caret to the beginning of editable (pretty WTFish).

I fixed all the bugs introduced by #10027 along with the bugs of the old implementation.

Question: While in ENTER_BR, what is the correct tag to be created when a block must be created (e.g. inheriting class, style, etc.)? <div> or <p>?

[Frederico Caldeira Knabben]

I'm having 2 reds here:
​http://ckeditor4.t/dt/plugins/indent/indent.html

Additionally, there is a mixture of TABS and SPACEs in the indentation of comments in enterkey/plugin.js.

[Olek Nowodziński]

• I moved red tests to where they belong (#10439).
• Fixed tab/space mess (shame, shame...)
• Fixed code style.
• Made content-specific commands internal for indentation system (no longer editor commands, see comment:9).
• Brought back ol' and naive outdent startupDisabled since command states will be resolved in #10439.

[Frederico Caldeira Knabben]

I'm cancelling the review as additional changes to the code are necessary as discussed in a private call with Olek.

[Olek Nowodziński]

Rebased t/10027 (dev+tests) on latest major.

• Introduced job-based indentation system that completely separates block-based and nesting-based indentation.
• Extended docs.
• Extended tests.
• Major code refactoring and optimization.

[Olek Nowodziński]

Merged fix into master, dev (​​git:41cbd9d), tests (0f6fc5b).

This ticket also resolves #8244.