Opened 6 years ago

Closed 6 years ago

#10812 closed Bug (fixed)

Range#createBookmark2 incorrectly normalizes offsets when element is selected

Reported by: Piotrek Koszuliński Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.2.2
Component: General Version: 4.2.1
Keywords: Cc:

Description (last modified by 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.

Change History (19)

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

Status: newconfirmed

Pushed t/10812 with a test.

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

Description: modified (diff)
Version: 4.2.1 (GitHub - master)

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

Description: modified (diff)

comment:4 Changed 6 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 4.2.2

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

Summary: Range#createBookmark2 incorrectly normalize offsets when element is selectedRange#createBookmark2 incorrectly normalizes offsets when element is selected

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

Owner: set to Olek Nowodziński
Status: confirmedreview

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 ;)

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

comment:8 Changed 6 years ago by 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 ;>

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

Owner: changed from Olek Nowodziński to Piotrek Koszuliński
Status: reviewassigned

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.

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

Related ticket: #10850.

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

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

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

Another one: #10893.

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

Status: assignedreview

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

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

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

Status: reviewreview_failed

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.

comment:15 in reply to:  14 ; Changed 6 years ago by Piotrek Koszuliński

Status: review_failedreview

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.

comment:16 in reply to:  15 Changed 6 years ago by 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.

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

Pushed once again.

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

Status: reviewreview_passed

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

Resolution: fixed
Status: review_passedclosed

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

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