Range#createBookmark2 incorrectly normalizes offsets when element is selected

[Piotrek Koszuliński]

Unit test can be found in t/10812.

Manual TCs:

TC1 (IE and sometimes FF)

1. Load this HTML: <h1>Apollo 11<img alt="Saturn V carrying Apollo 11" src="assets/sample.jpg" /></h1>
2. Select image and press align right.
3. Undo.

• Expected: image is selected after undoing (although selection may differ visually from the old one)
• Actual: selection is lost.
• Depending on the HTML I was sometimes able to reproduce this on FF too, but usually it only works on IE, because it keeps DOM clean.

TC2 (all browsers)

<table border="1" cellpadding="1" cellspacing="1" style="width:500px">
	<tbody>
		<tr>
			<td>a</td>
			<td>b</td>
		</tr>
		<tr>
			<td>c</td>
			<td>d</td>
		</tr>
	</tbody>
</table>

1. Load the above HTML.
2. Select table (using elements path).
3. Apply bold.
4. Try to undo.

• Expected: entire table is selected after undoing.
• Actual: selection is lost.

[Piotrek Koszuliński]

Pushed t/10812 with a test.

[Olek Nowodziński]

Created a simple fix for the issue in ​t/10812. It makes all tests green and fixes the manual case. Hopefully it won't break anything not covered with tests ;)

[Frederico Caldeira Knabben]

I'm not reviewing it... just to let you guys know that the removed code has been introduced with [6461] for #1272, which makes a fire alarm to fire in my head.

[Piotrek Koszuliński]

This line was first introduced in this patch ​http://dev.ckeditor.com/ticket/1272#comment:37 as a response for ​http://dev.ckeditor.com/ticket/1272#comment:36. Let's see how much has changed in last 3 years ;>

[Piotrek Koszuliński]

TC which I mentioned in comment:8 is not reproducible on t/10812 or master.

However, I've got a really mixed feelings about createBookmark2. I was reading its code and I'm not sure yet, but I think that it misses a lot of cases. Cases which we assume that are not possible for ranges extracted from real selection, but as this ticket proves... life can be surprising.

I also don't think that proposed patch is correct. These lines seem to be required and most likely patch passes automated tests because they are incomplete and manual tests because testing things related to text nodes normalization and selection are really hard and also browser specific.

My idea is that createBookmark2 should assume that everything is possible, because we are not able to anticipate every case possible in real life. It also shouldn't do unnecessary normalization. For example ​here it's moving container down the tree when it could just normalize offset to the last child's normalized index + 1.

[Olek Nowodziński]

Related ticket: #10850.

[Piotrek Koszuliński]

Perhaps other related tickets: #10858, #10411, #10869.

[Piotrek Koszuliński]

Another one: #10893.

[Piotrek Koszuliński]

Pushed a solution to ​t/10812b on dev and tests.

Patch fixes also: #10850, #10847 and #10842.

[Olek Nowodziński]

I added several tests, and fixed minor mistakes in code. Still:

• 34 failed/50 passed in IE8 in dt/core/dom/rangebookmarks.html
• Mixed CKEditor API with jQuery in createPlayground. As jQuery may be not available in the future due to some different testing environment, this should not happen.

[Piotrek Koszuliński]

Replying to a.nowodzinski:

I added several tests, and fixed minor mistakes in code. Still:

• 34 failed/50 passed in IE8 in dt/core/dom/rangebookmarks.html

Fixed it with a funny hack.

• Mixed CKEditor API with jQuery in createPlayground. As jQuery may be not available in the future due to some different testing environment, this should not happen.

Then we'll replace this with something else like we'll have to change many other things. Come on - it's the quickest and cleanest solution.

[Olek Nowodziński]

Replying to Reinmar:

Replying to a.nowodzinski:

I added several tests, and fixed minor mistakes in code. Still:

• 34 failed/50 passed in IE8 in dt/core/dom/rangebookmarks.html

Fixed it with a funny hack.

• Mixed CKEditor API with jQuery in createPlayground. As jQuery may be not available in the future due to some different testing environment, this should not happen.

Then we'll replace this with something else like we'll have to change many other things. Come on - it's the quickest and cleanest solution.

I can't see anything new in branches.

[Piotrek Koszuliński]

Pushed once again.

[Piotrek Koszuliński]

Fixed with ​git:5f51c3f on dev and 194136f on tests.