Opened 12 years ago

Closed 12 years ago

#8950 closed Bug (fixed)

Webkit: insertElement() not working as expected for inline elements

Reported by: Teresa Monahan Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.6.4
Component: General Version: 3.0
Keywords: IBM Cc: Damian, Satya Minnekanti

Description

The formatting of elements changes when inserting multiple elements into the editor in Webkit browsers.

To reproduce:

  • Save the attached file into the _samples directory and open it in Chrome or Safari.
  • Click the 'Insert Elements' button. This should add a paragraph of text to the editor and an image below it.

Problem: The image is inserted after the text, rather than below it in webkit browsers.

The generated HTML in webkit browsers is:

<p>
	Text goes here<img src="http://a.cksource.com/c/1/inc/img/demo-little-red.jpg" /></p>

Other browsers correctly add a new paragraph for the image:

<p>
	Text goes here</p>
<p>
	<img src="http://a.cksource.com/c/1/inc/img/demo-little-red.jpg" /></p>

This only seems to occur when an inline element is inserted after a block level element. We can reproduce this when the first element is a <p> or <div> and the 2nd element is a <img>, <iframe>, <a>, <span> etc.

This seems like it could be a webkit issue, however ticket #8412 describes an issue in Opera which could be related.

Attachments (8)

Webkit insertElements issue.html (1.9 KB) - added by Teresa Monahan 12 years ago.
webkit.html (2.1 KB) - added by Jakub Ś 12 years ago.
8950.patch (5.4 KB) - added by Garry Yao 12 years ago.
8950_2.patch (1.0 KB) - added by Garry Yao 12 years ago.
8950_3.patch (4.1 KB) - added by Garry Yao 12 years ago.
8950_4.patch (5.0 KB) - added by Garry Yao 12 years ago.
8950_5.patch (5.2 KB) - added by Garry Yao 12 years ago.
8950_6.patch (6.4 KB) - added by Garry Yao 12 years ago.

Download all attachments as: .zip

Change History (21)

Changed 12 years ago by Teresa Monahan

comment:1 Changed 12 years ago by Jakub Ś

Status: newconfirmed
Version: 3.6.4 (SVN - trunk)3.0

This is a little bit strange. I have console logged "temp.getFirst()" and this is how browsers see it:

Safari:

Outer1: <p>Text goes here<img src="http://a.cksource.com/c/1/inc/img/demo-little-red.jpg"></p>
Outer2: <img src="http://a.cksource.com/c/1/inc/img/demo-little-red.jpg">

FF without delay:

Outer1: <p>Text goes here</p>
Outer2: <img src="http://a.cksource.com/c/1/inc/img/demo-little-red.jpg">

FF with delay:

Outer1: "<p>Text goes here<img src="http://a.cksource.com/c/1/inc/img/demo-little-red.jpg"><br></p>"
Outer2: "<img src="http://a.ckso...g/demo-little-red.jpg">"
  1. Despite different results of "how browser sees", browsers first insert Text than the Image.
  2. Please see webkit.html. All I have done was adding some delay between two inserts. It turned out that this has changed how Firefox sees the code and in result how it inserts it (Firefox started behaving like Webkit).
Last edited 12 years ago by Jakub Ś (previous) (diff)

Changed 12 years ago by Jakub Ś

Attachment: webkit.html added

comment:2 Changed 12 years ago by Garry Yao

Milestone: CKEditor 3.6.4
Owner: set to Garry Yao
Status: confirmedreview

First we shall not expect insertElement to insert element side by side as specified, because this method is designed to have a new cursor position established for each insertion, thus the proper way to insert multiple element is to use instead the ::insertHtml method.

But the ticket reveals a defect of the cursor position after insertion, where cursor position should be placed at the end of the inserted block, instead of "after end", it's better for the following reasons:

  • Place cursor between blocks are ambitious at the selection level, as shown in this case
  • Cursor placed at the "last editable position" of inserted matches native browser impl.

Changed 12 years ago by Garry Yao

Attachment: 8950.patch added

comment:3 Changed 12 years ago by Garry Yao

Note that the change to range.js are for testing purpose.

http://ckeditor.t/tt/8950/1.html

comment:4 in reply to:  2 Changed 12 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

Replying to garry.yao:

... the ticket reveals a defect of the cursor position after insertion, where cursor position should be placed at the end of the inserted block, instead of "after end"

This is a wrong assumption.

insertElement gives more "precision" on the insertion, because we're dealing with a single well defined element.

insertHtml instead is very generic and we know nothing about the html structure passed to it, so in this case it is more acceptable to have the editor doing some correction and place the selection inside the insertion.

There are two ways for this ticket to get fixed, when inserting <p>Text</p> at the end of the document:

  1. Move the caret inside the inserted paragraph, as there is no acceptable position for it after that.
  1. Create a new <p> after the inserted paragraph and move the selection into this new one.

Probably "2" is the right one, as it is more consistent with the expected behavior.

Changed 12 years ago by Garry Yao

Attachment: 8950_2.patch added

comment:5 Changed 12 years ago by Garry Yao

Status: review_failedreview

Ok, if the above approach is to be taken, it's a matter of performing a selection change fix to reach the consistency across all browsers, for the above case.

comment:6 Changed 12 years ago by Garry Yao

It looks I was wrong with the above patch, it's still required to be fixed inside of the insertElement method.

Changed 12 years ago by Garry Yao

Attachment: 8950_3.patch added

Changed 12 years ago by Garry Yao

Attachment: 8950_4.patch added

comment:7 Changed 12 years ago by Garry Yao

After a while of hesitation, I consider 1. of 4 is still the best way to go, as for 2. results in a basis violation - the insertion should introduce no new element other than the request one, so from the suface level we'd rather align other browsers to the current webkit result instead.

comment:8 Changed 12 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

I would insist on "2" for this issue right now, so we'll give the developer an option (controlled by using either insertElement or insertHtml).

Changed 12 years ago by Garry Yao

Attachment: 8950_5.patch added

comment:9 Changed 12 years ago by Garry Yao

Status: review_failedreview

comment:10 Changed 12 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

After patch:

  1. Insert a table.
  2. Put the caret inside one of the cells.
  3. Insert an <hr>.
  4. Type something.

An extra paragraph will be appended after the typed text.

Changed 12 years ago by Garry Yao

Attachment: 8950_6.patch added

comment:11 Changed 12 years ago by Garry Yao

Status: review_failedreview

The above test's failure comes from plugin and is out the scope of this ticket, but considering the new behavior could simply the plugin code, taking it into the patch as well.

comment:12 in reply to:  11 Changed 12 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

Replying to garry.yao:

The above test's failure is out the scope of this ticket

If changes made by this ticket impact on other parts of the code, this is definitely to be fixed here. It's good to see that things could be simplified there.

comment:13 Changed 12 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [7538].

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy