Selection#getRanges(true) overrides cached ranges

[Michael Koza]

I was investigating an issue with a plugin that extended the link plugin (ckeditor_link Drupal project) and found a perhaps unintended side effect of this function. I noticed the plugin was using "getRanges(true)" to obtain the ranges instead of the "getRanges()" that link.js itself uses in the onOk defintion. For the most part this was working.

An except for an example:

[...]
definition.onOk = CKEDITOR.tools.override(definition.onOk, function(original) {
          return function() {
            var process = false;
            if ((this.getValueOf('info', 'linkType') == 'drupal') && !this._.selectedElement) {
              var ranges = editor.getSelection().getRanges(true);
              if ((ranges.length == 1) && ranges[0].collapsed) {
                process = true;
              }
            }
            original.call(this);
[...]

This works fine so long as it's not an image created by the new image2 image widget that the plugin is being applied to. All text and other items appeared to work correctly, but the getRanges(true) call resulted in a length of 0 when used on a new image from the image widget. This is not the case on images created by the original image plugin.

After some investigation I tried the following: I placed a console.log(editor.getSelection().getRanges(true)); prior to the call of original.call(this);

This resulted in breaking the standard functionality of the link plugin. I stepped through the process and found that the getRanges(true) call modifies the original value and any subsequent calls to getRanges() will result in the same value as the initial getRanges(true) call. Is this the intended behaviour of the getRanges function with the optional parameter?

Since this only seems to cause an empty range selection on images created by the new image widget.

[Michael Koza]

I guess I didn't finish that last thought:

Since this only seems to cause an empty range selection on images created by the new image widget, it might be a problem with how ranges are determined for widget objects.

[Piotrek Koszuliński]

First of all I want to clarify one thing. Widgets are non editable parts of content. Therefore calling getRanges(true) on selection containing just the image2 or any other widget will return empty selection. This is correct and intended behaviour. This is also why plugins like link use getRanges() instead of getRanges(true). Otherwise they wouldn't work with widgets, so Drupal plugins for CKEditor have to be updated.

Regarding a bug with cache, I indeed see code which looks incorrectly. ​https://github.com/ckeditor/ckeditor-dev/blob/master/core/selection.js#L1396 this function caches getRanges() result and then uses it for both - getRanges() and getRanges(true) calls. Unfortunately it modifies that array when onlyEditables option is passed, so getRanges() calls are influenced too. Thanks for finding this! :) I guess it wasn't easy.

[Piotrek Koszuliński]

Pushed ​t/11493 branches.

[Olek Nowodziński]

Rebased branches on latest master.

[Piotrek Koszuliński]

Fixed on master with ​git:b3e1f0998 on dev and 4e4b60e on tests.