Opened 5 years ago

Closed 5 years ago

#12071 closed New Feature (fixed)

New methods to replace set(get)HtmlWithRange(Selection)

Reported by: Olek Nowodziński Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.4.2
Component: General Version:
Keywords: Cc:

Description

The idea is to implement methods, which would allow us to set HTML with ranges (sometimes selection) anchored in elements and text nodes, depending on the case. A corresponding methods to read such HTML with ranges (selections) should also be implemented.

Examples:

<p>{foo}</p> // range anchored in "foo" text node
<p>{}foo</p> // collapsed range anchored in "foo" text node
<p>[]foo</p> // collapsed range anchored in <p>
<p>{foo]</p> // range anchored in "foo" text node (start) and <p> (end)

Required by #11636.

Change History (5)

comment:1 Changed 5 years ago by Olek Nowodziński

Owner: set to Olek Nowodziński
Status: newassigned

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

cc

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

I pushed some changes to benderjs-ckeditor/t/12071.

  1. Methods should not use "this" as bender.tools. Better to refer to bender.tools directly, so you can make a shortcut of such function (var make = bender.tools.setSelection)
  2. I don't think that getRange should automatically use fixHtml. This decision should be left for a caller.
  3. I've still got doubts about method names. For example getRange returns... an HTML. It's silly, but I think that we need to discuss this with the team, because all options seem to be equally bad.
  4. There's no error if there is more than one range in getSelection(). We should make any test using getSelection() in such case fail clearly.
  5. Tests for set/getSelection assumes that selection won't be normalised. I'm not sure you can do that. They fail on Opera (which does not yet have the same patch as Blink recently got), so they will also fail on Safari. I think that you want to know if getRange() matches /<p>[\{\[]foo[\}\]]</p>/ and that's it. You've got getRange() tested, so you can base get/setSelection() tests on it.
  6. I would like to see more tests for setSelection firing selectionChange range. I've got doubt about a case when it doesn't really change selection (e.g. two subsequent identical setSelection calls). I think there should be no selectionChange then, but most importantly - it should be consistent.

comment:4 in reply to:  3 Changed 5 years ago by Olek Nowodziński

Replying to Reinmar:

I pushed some changes to benderjs-ckeditor/t/12071.

  1. Methods should not use "this" as bender.tools. Better to refer to bender.tools directly, so you can make a shortcut of such function (var make = bender.tools.setSelection)

Done.

  1. I don't think that getRange should automatically use fixHtml. This decision should be left for a caller.

Done.

  1. I've still got doubts about method names. For example getRange returns... an HTML. It's silly, but I think that we need to discuss this with the team, because all options seem to be equally bad.

This topic will be discussed once everything's done. It's not a blocker.

  1. There's no error if there is more than one range in getSelection(). We should make any test using getSelection() in such case fail clearly.

Done.

  1. Tests for set/getSelection assumes that selection won't be normalised. I'm not sure you can do that. They fail on Opera (which does not yet have the same patch as Blink recently got), so they will also fail on Safari. I think that you want to know if getRange() matches /<p>[\{\[]foo[\}\]]</p>/ and that's it. You've got getRange() tested, so you can base get/setSelection() tests on it.

That's right. Done.

  1. I would like to see more tests for setSelection firing selectionChange range. I've got doubt about a case when it doesn't really change selection (e.g. two subsequent identical setSelection calls). I think there should be no selectionChange then, but most importantly - it should be consistent.

Pending...

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

Milestone: CKEditor 4.4.2
Resolution: fixed
Status: assignedclosed

Merged to master with git:6775bbee6 in ckeditor-dev (tests) and fe38a3d6 in https://github.com/benderjs/benderjs-ckeditor.

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