D&D whole selected content cause error

[Artur Delura]

Browser independent.

1. Open ​http://presets.ckeditor.dev/4.5.0/full-all/ckeditor/samples/
2. Focus editor
3. Click body in the elements path (whole content should be selected in the editor content)
4. Try D&D selected content somewhere.

There is an error in the console ​under

Uncaught TypeError: Cannot read property 'getParent' of null

[Piotrek Koszuliński]

Remember to set version.

[Szymon Cofalik]

This bug is a bit bigger: when you make selection that will have all contents to the end and drag it "below" there is the same bug. It is also true for selection containing first elements of editable and draging "above".

Edit: also if you will try to select a line of text and drag it to the left ;).

[Piotrek Koszuliński]

See also #13465.

[Szymon Cofalik]

The bug is caused because dropRange is inside dragRange and when we extract dragRange from editable's contents dropRange is invalid because it was saved by bookmark type 1.

But why browser lets us drag something into the selection? Normally this is impossible, browser shows cursor with crossed circle and nothing happens. But when we drop something on editable's space which is not covered by other element (mostly margin/padding) browser tries to find the closest possible drop place. And it may find it inside our drag range.

The fix will be to make a check inside internalDrop() if the dropRange is not inside dragRange. I tried to fix it inside isDropRangeAffectedByDragRange() method but it can't be done properly - no matter if the method returns true or false, exception is thrown.

[Olek Nowodziński]

Please start with ATs in ​branch:t/13453, if possible. When some reliable AT(s) exist, we can debate to what extent things must get refactored to fix the issue.

[Piotrek Koszuliński]

It must be a separate check and if drop range is detected inside drag range then whole algorithm must abort. It has little to do with isXAffectedByY, becuase that method only checks the order, but not the edge case. I would add the additional check before it.

[Artur Delura]

I think the fix should be to move drop range into proper place i.e. out of the drag range. Function isDropangeAffectedByDragRange has nothing to do with such checking. It assume that drop is out of drag range. It should be fixed at the very beginning.

[Szymon Cofalik]

My approach was the one suggested by Reinmar.

1) As he said, this is a different case so we won't include it into range affecting.

2) Changing isDropRangeAffectedByDragRange() would require writing special tests for that function and some cases that are now passing might fail (this was my experience when I started to mess with isDropRangeAffectedByDragRange() even though I was pretty sure that my change is actually fixing real-world scenarios). Things we do with isDropRangeAffectedByDragRange() are pretty complicated so it is better to leave the function and the tests alone.

3) Aborting the script is most similiar to what browser do when we drag onto selection.

4) @a.delura: we would have to change drop position to either start or end of drag range which would result in same content as when aborting script, but we would additional fire events and create snapshots, which are not needed.

[Szymon Cofalik]

Fix and tests pushed to ​branch:t/13453b

I introduced three methods in CKEDITOR.dom.range API as I think these were and will be needed. This is one of a few tickets where range intersections would be useful to me, so I wrote a method that checks if ranges intersect: CKEDITOR.dom.range.intersects.

The theory after checking intersection is this: if any range boundary of range A is inside range B the ranges does intersect. So we check all range boundaries against the other range. These are all cases how ranges can interact with themselves:

{ } [ ]
{ [ } ]
[ { ] }
[ ] { }
[ { } ]
{ [ ] }
{[   ]} (same ranges)

The method uses CKEDTIOR.dom.range.containsRangeBoundary that checks if "a place" in DOM specified by container and offset (or just a range boundary) is inside the range. This is mostly helper function for CKEDITOR.dom.range.intersects.

The checks are soft, meaning that if a range starts in exactly same place as the other range ends - they do not intersect. They just have the same range boundary. There is a special case, when the ranges are identical - none of boundaries are in fact inside the other range. Then, we check if ranges are collapsed: if not - ranges intersect (wholly!). If they are collapsed - the ranges are treated like they are next to each other - so they do not intersect.

If you need hard check, you can use CKEDITOR.dom.range.touches which checks if ranges have any boundary identical. Something like: rangeA.intersects( rangeB ) || rangeA.touches( rangeB ) will do a hard check.

All three methods are tested, I tried to do my best when thinking of test cases but you may suggest other.

Important note:

CKEDITOR.dom.range.containsRangeBoundary uses internal method compareAddress() to get position-relation between nodes. This function is very similiar to ​CKEDITOR.dom.node.getPosition - see the loop at the end of the method. I tried to use ​CKEDITOR.dom.node.getPosition but since it does not operate on offsets I needed to create some boilerplate code in CKEDITOR.dom.range.containsRangeBoundary and it still didn't pass all the tests. You can check my attempts on ​branch:t/13453.

However, since ​CKEDITOR.dom.node.getPosition almost "includes" compareAddress() I propose moving compareAddress() to a space that is visible both by ​CKEDITOR.dom.node.getPosition and CKEDITOR.dom.range.containsRangeBoundary and make both those functions use it.

[Piotrek Koszuliński]

The code in ​branch:t/13453b is very interesting and from time to time I would love to see these methods in the core. However, there are couple of problems:

• Those methods could be a polyfill for native methods. That's a better practice because they are then easier to understand for others, they are faster on browsers which do support them and we could even eventually avoid coding them if they are only needed on certain browsers.
• It would be good to somehow merge compareAddress with node.comparePosition.
• This problem can be perhaps solved in 2LOC instead of 40SLOC.

My idea for the fix is that we transform drag/dropRanges to bookmarks. And comparing positions of bookmarks is much easier, because we can use node.comparePosition and we do not have to worry about identical positions.

[Szymon Cofalik]

Pushed ​branch:t/13453c with proposed solution.

Important note: To fix the described bug completly, #13465 needs to be fixed too (issues are related).

[Piotrek Koszuliński]

Fixed on master with ​git:5386e97.