Opened 7 years ago

Closed 7 years ago

#3783 closed Bug (fixed)

Indenting commands in table cells creates collapsed paragraphs.

Reported by: martinkou Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.0
Component: General Version: SVN (CKEditor) - OLD
Keywords: Firefox Review+ Cc:

Description

To reproduce:

  1. Open replacebyclass.html in Firefox/Safari/Opera.
  2. Create a table.
  3. Move caret to first table cell.
  4. Indent.
  5. Move caret to second table cell.
  6. Try to move caret back to first table cell.
  7. You can't do that (Firefox), or you'll have strange selection positions in other browsers.

Attachments (3)

3783.patch (5.2 KB) - added by garry.yao 7 years ago.
3783_2.patch (11.5 KB) - added by garry.yao 7 years ago.
3783_3.patch (8.7 KB) - added by garry.yao 7 years ago.

Download all attachments as: .zip

Change History (10)

Changed 7 years ago by garry.yao

comment:1 Changed 7 years ago by garry.yao

  • Keywords Review? added
  • Owner changed from martinkou to garry.yao
  • Status changed from new to assigned

We've again tripped by bookmark ignoring logic, and this time is about CKEDITOR.dom.element::getNext/getPrevious, I'm proposing of introducing a guard function to it as we deal with 'getPreviousSourceNode/getNextSourceNode'.

comment:2 Changed 7 years ago by fredck

  • Keywords Review- added; Review? removed

There are a few issues in the getNext and getPrevious changes:

  • The "guard" name for the function is misleading, as it doesn't have the same usage we have in the walker to "stop" the search. It instead acts like a filter, and we have used the "evaluator" name for it in the walker. The same name should be used here.
  • The CKEDITOR.dom.element class is being used to transform the node when passing it to the guard. We could have text nodes also there, so the CKEDITOR.dom.node class should be used.
  • The raw DOM node is being transformed twice, once when passing it to the guard (evaluator) and again when returning the function result. We should find a solution to make this transformation only once (maybe saving the created object into a new variable).

Other than that, we must be sure that the changes in the function are not impacting other parts of the code which are already using it.

Changed 7 years ago by garry.yao

comment:3 Changed 7 years ago by garry.yao

  • Keywords Firefox Review? added; Review- removed

Beside fixing the above problems, now evaluator is symmetrically available on 'CKEDITOR.dom.element.getFist/Last', adding TCs for changed function.

comment:4 Changed 7 years ago by fredck

  • Keywords Review- added; Review? removed

Let's simplify the implementation just a little bit. Something like this:

getNext : function( evaluator )
{
	var next = this.$, retval;
	do
	{
		next = next.nextSibling;
		retval = next && new CKEDITOR.dom.node( next );
	}
	while ( retval && evaluator && !evaluator( retval ) )
	return retval;
}

Also, I understand the theoretical benefit for the getFirst and getLast changes, but we're not using this new feature anywhere, so we should not have it at this time. We may introduce it in the future at first use. Please revert those changes.

Changed 7 years ago by garry.yao

comment:5 Changed 7 years ago by garry.yao

  • Keywords Review? added; Review- removed

comment:6 Changed 7 years ago by fredck

  • Keywords Review+ added; Review? removed

comment:7 Changed 7 years ago by garry.yao

  • Resolution set to fixed
  • Status changed from assigned to closed

Fixed with [3930].

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