Opened 15 years ago
Closed 14 years ago
#5436 closed Bug (fixed)
[[IE]] Cursor goes to next Table Cell after we insert a Smiley in the Table Cell
Reported by: | Satya Minnekanti | Owned by: | Frederico Caldeira Knabben |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 3.4 |
Component: | General | Version: | 3.0 |
Keywords: | IBM | Cc: | Damian, joek |
Description
To reproduce the defect
- Open Ajax sample.
- Insert a Table with some no of rows and Columns
- 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 (6)
Change History (32)
comment:1 Changed 15 years ago by
comment:3 Changed 15 years ago by
Keywords: | Confirmed added |
---|---|
Milestone: | CKEditor 3.3 → CKEditor 3.4 |
Version: | 3.2 → 3.0 |
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 15 years ago by
Owner: | set to brooks |
---|---|
Status: | new → assigned |
Changed 15 years ago by
Attachment: | 5436.patch added |
---|
comment:5 Changed 15 years ago by
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 15 years ago by
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 14 years ago by
Owner: | changed from brooks to Tobiasz Cudnik |
---|---|
Status: | assigned → new |
I'm taking over this ticket after Brooks.
comment:8 Changed 14 years ago by
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 14 years ago by
Attachment: | 5436_2.patch added |
---|
comment:9 Changed 14 years ago by
Keywords: | Review? added; Review- removed |
---|---|
Status: | new → 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 14 years ago by
Keywords: | Review+ added; Review? removed |
---|
comment:13 Changed 14 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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 14 years ago by
Attachment: | 5436_3.patch added |
---|
comment:14 Changed 14 years ago by
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 14 years ago by
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 14 years ago by
Attachment: | 5436_4.patch added |
---|
comment:16 Changed 14 years ago by
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 14 years ago by
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:
- Load this HTML:
<ul> <li> Item <ul> <li>Sub-item</li> </ul> </li> </ul>
- Put the caret at the end of "Item".
- 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 14 years ago by
Attachment: | 5436_5.patch added |
---|
comment:18 Changed 14 years ago by
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 14 years ago by
Keywords: | Confirmed removed |
---|---|
Status: | reopened → confirmed |
comment:20 Changed 14 years ago by
Keywords: | Review? removed |
---|
comment:21 Changed 14 years ago by
Status: | review → 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 14 years ago by
Attachment: | 5436_6.patch added |
---|
comment:22 Changed 14 years ago by
Owner: | changed from Tobiasz Cudnik to Frederico Caldeira Knabben |
---|---|
Status: | review_failed → review |
comment:23 Changed 14 years ago by
Status: | review → 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 14 years ago by
Status: | review_failed → 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 14 years ago by
Status: | review → review_passed |
---|
comment:26 Changed 14 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Fixed with [5736].
Same behaviour happens when we insert a Horizontal Line,or an Anchor,or a New Table inside the Current Cell