Ticket #8950 (closed Bug: fixed)

Opened 2 years ago

Last modified 2 years ago

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

Reported by: tmonahan Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.6.4
Component: General Version: 3.0
Keywords: IBM Cc: damo, satya

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

Webkit insertElements issue.html (1.9 KB) - added by tmonahan 2 years ago.
webkit.html (2.1 KB) - added by j.swiderski 2 years ago.
8950.patch (5.4 KB) - added by garry.yao 2 years ago.
8950_2.patch (1.0 KB) - added by garry.yao 2 years ago.
8950_3.patch (4.1 KB) - added by garry.yao 2 years ago.
8950_4.patch (5.0 KB) - added by garry.yao 2 years ago.
8950_5.patch (5.2 KB) - added by garry.yao 2 years ago.
8950_6.patch (6.4 KB) - added by garry.yao 2 years ago.

Change History

Changed 2 years ago by tmonahan

comment:1 Changed 2 years ago by j.swiderski

  • Status changed from new to confirmed
  • Version changed from 3.6.4 (SVN - trunk) to 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 2 years ago by j.swiderski (previous) (diff)

Changed 2 years ago by j.swiderski

comment:2 follow-up: ↓ 4 Changed 2 years ago by garry.yao

  • Status changed from confirmed to review
  • Owner set to garry.yao
  • Milestone set to CKEditor 3.6.4

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 2 years ago by garry.yao

comment:3 Changed 2 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 2 years ago by fredck

  • Status changed from review to review_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 2 years ago by garry.yao

comment:5 Changed 2 years ago by garry.yao

  • Status changed from review_failed to review

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 2 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 2 years ago by garry.yao

Changed 2 years ago by garry.yao

comment:7 Changed 2 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 2 years ago by fredck

  • Status changed from review to review_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 2 years ago by garry.yao

comment:9 Changed 2 years ago by garry.yao

  • Status changed from review_failed to review

comment:10 Changed 2 years ago by fredck

  • Status changed from review to review_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 2 years ago by garry.yao

comment:11 follow-up: ↓ 12 Changed 2 years ago by garry.yao

  • Status changed from review_failed to review

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 2 years ago by fredck

  • Status changed from review to review_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 2 years ago by garry.yao

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

Fixed with [7538].

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