Rich combos reflects whole selection

[Comandeer]

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

• Unit tests
• Manual tests

What changes did you make?

I've introduced CKEDITOR.plugins.richcombo namespace, which currently holds only one method – createSelectionListener. It contains common logic for creating selectionChange listeners for rich combos (Font, Font Size, Styles and Format). These listeners ensures that when determining the displayed value of rich combo state from the whole, not only the beginning, of selection is considered.

There are however some issues with selecting new text, starting from the same element as previous selection. Such selection does not refresh the state of rich combo.

I'm also not 100% sure about createSelectionListener. It simplifies a logic and put it into one place, but at the same time I feel it somehow illogical. Probably it's a place that could be further improved.

closes #525.

[f1ames]

The idea itself is nice as it simplified a lot of code and extracts the common logic, however as you mentioned it looks a little out of place in a richcombo plugin. It looks more like some kind of helper/utility which could be totally separated from richcombo (looking at the createSelectionListener function itself, it is executed in the combo scope, however I don't see any references to this or combo object itself).

I was thinking of moving it somewhere up the abstract chain in CKEDITOR.ui namespace, but haven't found the proper place. So there are two ideas:

1. Make richcombo context/selection aware

What I mean here is to bound the logic from createSelectionListener into richcombo plugin itself. For example there could be onSelectionChange/onContextChange method (working same as onRender, onClick, etc). It will be called (if defined by plugin using richcombo) when selection changes.
Looking on how all combos works now it makes sense as all of them listen to selectionChange event. The problem is richcombo is basically defined as UI element (CKEDITOR.ui.richCombo) so it may still look wrong from the conceptual point of view.

The other issue is that createSelectionListener executes callback for every element found in selection/path and with the above change it will be more simple to just pass a list of all element/nodes when calling onSelectionChange.

2. Extract createSelectionListener to more suitable place
That's the tough one, as mentioned earlier, the createSelectionListener function does not fit very well inside richcombo namespace. Apart from getNewValue function which is "combo-specific" it is a quite generic code. From the other hand I'm not sure if it is generic enough to put it somewhere in the CKEDITOR.tools or any more suitable place.

@Comandeer WDYT? Any of those two makes more sense than the current approach?

---

1.

There are however some issues with selecting new text, starting from the same element as previous selection. Such selection does not refresh the state of rich combo.

AFAIR, selectionChange is fired only if start of the selection (startContainer) changes. Have you consider using selectionCheck? The problem with it could be that it is fired before selectionCheck on which richcombo is refreshed and its value cleared.

2.
I added one unit test as for selection like:

<p><span style="' + ffArial + '">f{oo</span>ba}r</p>

Arial is shown in a combo, while it should be just default value from what I understand. You may also add similar tests for other richcombo plugins. Remember also to put non-styled text in manual tests so this case is also covered there.

3.
Could you create one manual (or unit if possible) test with styles inside a table without tableselection plugin? For most browser there should be no difference, but it will also cover Firefox native table selection.

[f1ames]

Would it be possible to gather all nodes (as an Array/NodeList), pass it at once to a callback and then check if all elements which passes checkElementMatch have the same styling? Is the performance (less loops) or order of processing nodes the reason for using above approach with running callback for each node?

[Comandeer]

This PR needs a proper refactor and more testing.