Opened 8 years ago

Closed 8 years ago

#3783 closed Bug (fixed)

Indenting commands in table cells creates collapsed paragraphs.

Reported by: Martin Kou 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 8 years ago.
3783_2.patch (11.5 KB) - added by Garry Yao 8 years ago.
3783_3.patch (8.7 KB) - added by Garry Yao 8 years ago.

Download all attachments as: .zip

Change History (10)

Changed 8 years ago by Garry Yao

Attachment: 3783.patch added

comment:1 Changed 8 years ago by Garry Yao

Keywords: Review? added
Owner: changed from Martin Kou to Garry Yao
Status: newassigned

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 8 years ago by Frederico Caldeira Knabben

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 8 years ago by Garry Yao

Attachment: 3783_2.patch added

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

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 8 years ago by Garry Yao

Attachment: 3783_3.patch added

comment:5 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:6 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:7 Changed 8 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [3930].

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