Ticket #5436 (closed Bug: fixed)

Opened 4 years ago

Last modified 4 years ago

[[IE]] Cursor goes to next Table Cell after we insert a Smiley in the Table Cell

Reported by: satya Owned by: fredck
Priority: Normal Milestone: CKEditor 3.4
Component: General Version: 3.0
Keywords: IBM Cc: damo,joek

Description

To reproduce the defect

  1. Open Ajax sample.
  1. Insert a Table with some no of rows and Columns
  1. Go in to one of Table Cell and Click on Smiley Icon

Actual Result:

See that the Smiley is inserted in to the Current Table Cell and the Cursor goes to the next Table Cell..If it is last Table Cell Cursor goes out of the Table

Expected Result:

Smiley should be inserted in to the Current Table Cell and the Cursor should remain in the same Table Cell.

I have tested against IE 6,7 & 8

This is a regression issue.

Attachments

5436.patch (863 bytes) - added by brooks 4 years ago.
5436_2.patch (993 bytes) - added by tobiasz.cudnik 4 years ago.
5436_3.patch (815 bytes) - added by tobiasz.cudnik 4 years ago.
5436_4.patch (830 bytes) - added by tobiasz.cudnik 4 years ago.
5436_5.patch (867 bytes) - added by tobiasz.cudnik 4 years ago.
5436_6.patch (1.5 KB) - added by fredck 4 years ago.

Change History

comment:1 Changed 4 years ago by satya

Same behaviour happens when we insert a Horizontal Line,or an Anchor,or a New Table inside the Current Cell

comment:2 Changed 4 years ago by Saare

Similar with #5348.

comment:3 in reply to: ↑ description Changed 4 years ago by fredck

  • Keywords Confirmed added
  • Version changed from 3.2 to 3.0
  • Milestone changed from CKEditor 3.3 to CKEditor 3.4

This is a regression issue.

No, it's not a regression. I can confirm it with CKEditor 3.0, 3.1 and 3.2.

comment:4 Changed 4 years ago by brooks

  • Owner set to brooks
  • Status changed from new to assigned

Changed 4 years ago by brooks

comment:5 Changed 4 years ago by brooks

  • Keywords review? added; Confirmed removed

when we insert a inline element,no cursor jump strategy adopted. In my opinion, we can cancel the jump behavior when a block element inserted, coz little profit a user could get from this behavior.

comment:6 Changed 4 years ago by garry.yao

  • Keywords Confirmed Review- added; review? removed

This issue still remains for block element after the proposed patch.
I believe L123-L127 exists only for the reason that it could prevent empty block creation (our fixForBody logic) when element is inserted in body, e.g. Insert <hr> in body. But it's better to receive comment from the author to properly understand if there's other situation that need this part of codes.

comment:7 Changed 4 years ago by tobiasz.cudnik

  • Status changed from assigned to new
  • Owner changed from brooks to tobiasz.cudnik

I'm taking over this ticket after Brooks.

comment:8 Changed 4 years ago by fredck

I've done some investigation around this issue. In fact, the problem is the piece of code going from line 124 to 126 in the wysiwygarea plugin, which has been introduced because of #3100.

The issue fixed by those lines is that when, for example, inserting a horizontal line in the middle of a paragraph, we may have this selection, which is wrong:

<p>...</p>
<hr />^
<p>...</p>

This can be easily confirmed if you comment that code and try it with Firefox. So, we need to fix it in that case.

But, the fix is totally ignoring other cases. So, what we need to understand here is the following: are we inserting a non inline element followed by a block element? In that case, fix it.

Changed 4 years ago by tobiasz.cudnik

comment:9 Changed 4 years ago by tobiasz.cudnik

  • Keywords Review? added; Review- removed
  • Status changed from new to assigned

Second patch implements non inline element followed by a block element solution, which seems to resolve the issue for smile insertion and still allows the HR fix.

Additionally patch fixes an assignment in an IF condition in same plugin, which was changing the node type.

comment:10 Changed 4 years ago by fredck

  • Keywords Review+ added; Review? removed

comment:11 Changed 4 years ago by tobiasz.cudnik

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

Fixed with [5676].

comment:12 Changed 4 years ago by tobiasz.cudnik

Committed into correct branch with [5698].

comment:13 Changed 4 years ago by satya

  • Status changed from closed to reopened
  • Resolution fixed deleted

The issue is not fully fixed, Cursor still going to the next Cell when we insert a Horizontal Line or a new Table inside the Current Cell.

Changed 4 years ago by tobiasz.cudnik

comment:14 Changed 4 years ago by tobiasz.cudnik

  • Keywords Review? added; Review+ removed

I'm proposing alternative approach, which seems a bit more logic to me.

We simply check if we're still legal to edit in the point of insertion and if not, we're jumping to next block.

comment:15 Changed 4 years ago by fredck

  • Keywords Review- added; Review? removed

There are some valid elements that are not being considered, like inline elements.

Let's review the situation, so we can have a better understanding.

We may have these situations:

A:
<block_limit>
    <inserted_element>
    (inline node)
</block_limit>

B:
<block_limit>
    <inserted_element>
</block_limit>

C:
<block_limit>
    <inserted_element>
    <block element>
</block_limit>

We'll fix the selection "only" in case C, and only if the inserted element is a block element. This is the very first check to be done.

Then, the next check is: is the inserted elements "directly followed" by another block element? In that case, move to that element. For that, we should instead use insertedElement.getNext( evaluator ).

Btw, we may fix the code a bit, so "next" is retrieved only when necessary.

Changed 4 years ago by tobiasz.cudnik

comment:16 Changed 4 years ago by tobiasz.cudnik

  • Keywords Review? added; Review- removed

Last patch implements Frederico's solution and seems to work fine for nested table, smile inside a table and also horizontal line inside body.

comment:17 Changed 4 years ago by fredck

  • Keywords Review- added; Review? removed

By using getNextSourceNode, you may go outside the limits and get an element which is not appropriate. Those are edge cases, but the issue can be reproduced after patch:

  1. Load this HTML:
<ul>
	<li>
		Item
		<ul>
			<li>Sub-item</li>
		</ul>
	</li>
</ul>
  1. Put the caret at the end of "Item".
  2. Insert an Horizontal Rule.

Note that the selection goes to "Sub-item".

As indicated in my previous comment, this may be solved by using getNext( evaluator ) instead, which will limit the search into the same parent tree only.

Changed 4 years ago by tobiasz.cudnik

comment:18 Changed 4 years ago by tobiasz.cudnik

  • Keywords Review? added; Review- removed

Fifth patch uses getNext() method, although selection still goes into <li>Sub-item</li>, as UL is a block element.

comment:19 Changed 4 years ago by fredck

  • Status changed from reopened to confirmed
  • Keywords Confirmed removed

comment:20 Changed 4 years ago by fredck

  • Keywords Review? removed

comment:21 Changed 4 years ago by fredck

  • Status changed from review to review_failed
  • The evaluator function used in this patch is wrong, as it will jump "any" text node, returning the first element found.
  • It's quite simple to fix it to make it work with the tc defined in comment:17.

I'll provide a new patch for it.

Changed 4 years ago by fredck

comment:22 Changed 4 years ago by fredck

  • Owner changed from tobiasz.cudnik to fredck
  • Status changed from review_failed to review

comment:23 Changed 4 years ago by Saare

  • Status changed from review to review_failed

Use this TC, just instead of hr insert an inline element (e.g. an image or an anchor). The cursor goes beneath the new element instead of after it.

comment:24 Changed 4 years ago by fredck

  • Status changed from review_failed to review

Believe me or not, this new TC has nothing to do with the reported issue as it's related to the incapacity of IE to change selections in that position of the document, and it's not introduced by the patch. It deserves a dedicated ticket.

The issue the patch is trying to fix is related to table cells. The TC at comment:17 was there just to confirm that block elements are handled properly. Please review it again considering this.

comment:25 Changed 4 years ago by Saare

  • Status changed from review to review_passed

comment:26 Changed 4 years ago by fredck

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

Fixed with [5736].

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