Opened 6 years ago

Closed 6 years ago

#11493 closed Bug (fixed)

Selection#getRanges(true) overrides cached ranges

Reported by: Michael Koza Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.3.3
Component: Core : Selection Version:
Keywords: Cc:

Description

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.

Change History (6)

comment:1 Changed 6 years ago by 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.

comment:2 Changed 6 years ago by Piotrek Koszuliński

Component: GeneralCore : Selection
Keywords: link widget ranges removed
Milestone: CKEditor 4.3.3
Status: newconfirmed
Version: 4.3.2

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.

comment:3 Changed 6 years ago by Piotrek Koszuliński

Owner: set to Piotrek Koszuliński
Status: confirmedassigned
Summary: Using getRanges(true) appears to result in a destructive actionSelection#getRanges(true) overrides cached ranges

comment:4 Changed 6 years ago by Piotrek Koszuliński

Status: assignedreview

Pushed t/11493 branches.

comment:5 Changed 6 years ago by Olek Nowodziński

Status: reviewreview_passed

Rebased branches on latest master.

comment:6 Changed 6 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed

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

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy