Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#11393 closed Bug (fixed)

Regression in link/link.html test

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

Description (last modified by Piotrek Koszuliński)

First bad commit git:f905c2d3.

Test named "test create link on a non-editable inline element" failed with error: "Unexpected error: Index or size was negative, or greater than the allowed value.".

The problem is caused by DOM being overwritten in the test (with editable.setHtml()). It does not trigger any selection change event, so editor._.hiddenSelectionContainer still exists, however, container is not present in DOM.

In this case it's more test issue than real bug. But making this bulletproof will not harm anything.

Change History (4)

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

Owner: set to Piotrek Koszuliński
Status: newassigned

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

Description: modified (diff)
Status: assignedreview

Pushed t/11393 on dev and tests.

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

Status: reviewreview_failed

I rebased both branches on latest master and pushed a small fix to tests.

This change requires a word of comment since it's hard to figure out that hiddenSelectionContainer is no longer in DOM:

       if ( range.endContainer.equals( root ) )
 -        range.endOffset = Math.min( range.endOffset, hiddenSelectionContainerIndex );
 +        range.endOffset = Math.min( range.endOffset, root.getChildCount() );
      }

Regarding tests

assert.isMatching(
	// getHtmlWithRanges processes selections anchored on blocks incorrectly.
	/^(<p>\[<\/p>)?<p>\[?abc\]?<\/p>(<p>\]<\/p>)?$/i,
	html,
	'Selection contains entire editable contents'
);

// Be sure the selection is really there - regexp doesn't check it.
assert.isTrue( !!html.match( /\[/ ) && !!html.match( /\]/ ), 'Selection exists' );

Oh dude, come on :P

Use:

assert.isMatching(
	// getHtmlWithRanges processes selections anchored on blocks incorrectly.
	/(^<p>\[abc\]</p>$)|(^<p>\[</p><p>abc</p><p>\]</p>$)/i,
	html,
	'Selection contains entire editable contents'
);

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

Resolution: fixed
Status: review_failedclosed

Added comment and simplified comparisons in tests.

Fixed on master with git:40be3ec on dev and 0557518 on tests.

Last edited 5 years ago by Piotrek Koszuliński (previous) (diff)
Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy