Opened 4 years ago

Closed 4 years ago

#12448 closed New Feature (fixed)

insertHtmlIntoRange

Reported by: Piotr Jasiun Owned by: Piotr Jasiun
Priority: Normal Milestone: CKEditor 4.5.0 Beta
Component: General Version:
Keywords: Cc:

Description (last modified by Piotr Jasiun)

When image is uploaded (#11461) we need to replace uploading image with the final one, so we need to insert html in the given range (not in the editors selection). Because of this situation we need to introduce insertHtmlIntoRange methods.

Change History (10)

comment:1 Changed 4 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: newassigned

comment:2 Changed 4 years ago by Piotr Jasiun

Summary: insertHtmlIntoRange and insertHtmlIntoSelectioninsertHtmlIntoRange

comment:3 Changed 4 years ago by Piotr Jasiun

Description: modified (diff)

comment:4 Changed 4 years ago by Piotr Jasiun

Status: assignedreview

There were some conceptual problems with this ticket. The main reason is that insertHtml is high-level method: it fixes selection, scroll viewport and can make snapshots if widget was inserted. The insertHtmlIntoRange, in contrast, is low-level method: it just insert HTML without any additional operations. But in both cases we need to make some operations after insertion (ex. checkWidgets). To solve the problem I introduced afterInsert event with the intoRange parameter which is true if this is low level method.

Changes in t/12448.

comment:5 Changed 4 years ago by Piotrek Koszuliński

Status: reviewreview_failed

I rebased branch:t/12448 and pushed additional commit.

  • insertElement and insertElementIntoRange don't need to fire the afterInsert event. These methods are very different from insertHtml and insertText, because they can't trigger data processor. Also the insertion algorithm is much different.
  • Since the behaviour is so different, I think that it will be better to rename the event to afterInsertHtml so in the future we can have one specific for insertElement. The goal is to have a total separation between methods inserting elements and methods inserting HTML/text.
  • data.intoRange should carry the range.
  • Please complete editor.insertHtml/Text and editable.insertHtmlIntoRange docs with information that these methods fire the editor.afterInsertHtml event. In editor.inserHtml/Text/Element you can also mention the editor.insertHtml/Text/Element events because I see that they are not mentioned there.

comment:6 Changed 4 years ago by Piotrek Koszuliński

I also noticed a difference between:

  • insertElement(element, range) chooses between insertElementIntoRange and insertElementIntoSelection based on the range argument.
  • For HTML there's only insertHtml and insertHtmlIntoRange.

I find the two methods better than three, but wouldn't it be better to do it the same way as element methods for consistency reasons?

Last edited 4 years ago by Piotrek Koszuliński (previous) (diff)

comment:7 in reply to:  6 ; Changed 4 years ago by Piotr Jasiun

Status: review_failedreview

Replying to Reinmar:

I also noticed a difference between:

  • insertElement(element, range) chooses between insertElementIntoRange and insertElementIntoSelection based on the range argument.
  • For HTML there's only insertHtml and insertHtmlIntoRange.

I find the two methods better than three, but wouldn't it be better to do it the same way as element methods for consistency reasons?

We agreed that these methods only look similar, but work differently, so there is no reason do make then look event more similar. It may suggest that then have something common. Especially when the third method is useless.

I fixed rest of issues. Changes in t/12448.

Last edited 4 years ago by Piotr Jasiun (previous) (diff)

comment:8 in reply to:  7 Changed 4 years ago by Piotr Jasiun

Replying to pjasiun:

Replying to Reinmar:

I also noticed a difference between:

  • insertElement(element, range) chooses between insertElementIntoRange and insertElementIntoSelection based on the range argument.
  • For HTML there's only insertHtml and insertHtmlIntoRange.

I find the two methods better than three, but wouldn't it be better to do it the same way as element methods for consistency reasons?

We agreed that these methods only look similar, but work differently, so there is no reason do make then look event more similar. It may suggest that then have something common. Especially when the third method is useless.

I fixed rest of issues. Changes in t/12448.

After the discussion we decided to organized insertHtml methods the same way insertElement is organized. I also fixed and added test and rebased branch on the newest major.

comment:9 Changed 4 years ago by Piotrek Koszuliński

Status: reviewreview_passed

comment:10 Changed 4 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed

Merged to major with git:18d651ad.

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