Opened 3 years ago

Closed 3 years ago

#13453 closed Bug (fixed)

D&D whole selected content cause error

Reported by: Artur Delura Owned by: Szymon Cofalik
Priority: Normal Milestone: CKEditor 4.5.3
Component: General Version: 4.5.0 Beta
Keywords: Cc:

Description

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

Change History (15)

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

Status: newconfirmed
Version: 4.5.0 Beta

Remember to set version.

comment:2 Changed 3 years ago by Szymon Cofalik

Owner: set to Szymon Cofalik
Status: confirmedassigned

comment:3 Changed 3 years ago by 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 ;).

Last edited 3 years ago by Szymon Cofalik (previous) (diff)

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

See also #13465.

comment:5 Changed 3 years ago by 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.

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

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

comment:8 Changed 3 years ago by 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.

comment:9 Changed 3 years ago by 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.

Last edited 3 years ago by Szymon Cofalik (previous) (diff)

comment:10 Changed 3 years ago by Szymon Cofalik

Status: assignedreview

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.

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

Status: reviewassigned

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.

comment:12 Changed 3 years ago by 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).

comment:13 Changed 3 years ago by Szymon Cofalik

Status: assignedreview

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

Milestone: CKEditor 4.5.2CKEditor 4.5.3

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

Resolution: fixed
Status: reviewclosed

Fixed on master with git:5386e97.

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