Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[17009] Keep ID of inlined element while insertText is used. #430

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@msamsel
Copy link
Contributor

commented May 31, 2017

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

yes

This PR contains

  • Unit tests
  • Manual tests

What changes did you make?

  • Change in method mergeSibling to always combine elements with higher index to element with lower index.
  • Create mechanism to preserve id:
    -- firstly id with reference to element is kept in array
    -- because merging siblings was change, I have guarantee that reference to element will remain for entire process
    -- then id is removed from elements
    -- now process of insert is going as usual. If there will be possibility to merge inline element, this should happen
    -- id is restored to elements. If there will be inserted non-inline elements, then id will apply to first element.

Before change:

  • <p><span id="abc">te^st</span></p> + insertText('FOO');
  • <p><span id="abc">te</span><span>FOOst</span></p>

After change:

  • <p><span id="abc">te^st</span></p> + insertText('FOO');
  • <p><span id="abc">teFOOst</span></p>

Closes #697.

@f1ames f1ames self-requested a review Jul 17, 2017

@f1ames
Copy link
Member

left a comment

The code itself works fine, however there are some code styling/formatting issues. Remember about:

  • Proper code style, especially spaces, e.g. if ( test === true ) {, } );.
  • Use single quotes only (do not use them in object keys if not needed).
  • Proper commenting:
    • Proper tickets referencing (full link on the comment end if linking to Trac).
    • Recheck for any grammar mistakes.

There are also some tests failing on IE8
image

@@ -0,0 +1,15 @@
@bender-tags: 4.7.1, tc, 17009

This comment has been minimized.

Copy link
@f1ames

f1ames Jul 24, 2017

Member

Use bug tag instead of tc.
Trac number tag should be prefixed with trac, like trac17009.

@@ -0,0 +1,15 @@
@bender-tags: 4.7.1, tc, 17009
@bender-ui: collapsed
@bender-ckeditor-plugins: wysiwygarea, divarea, toolbar, sourcearea, undo, elementspath, justify, specialchar

This comment has been minimized.

Copy link
@f1ames

f1ames Jul 24, 2017

Member

The divarea plugin is in extraPlugins so it is not needed here. Also justify, elementspath and undo are not needed (looking on a scenario). However, I would remove justify and elementspath and leave undo (so someone may check if it works).

### Insert text.
1. Open `Source` in editor. Make sure there is `span` with `id` attribute.
1. Put carret in editor.
1. Insert text, by usinng `Insert Special Character` button.

This comment has been minimized.

Copy link
@f1ames

f1ames Jul 24, 2017

Member
  1. Insert text using Insert Special Character button.

### Insert text.
1. Open `Source` in editor. Make sure there is `span` with `id` attribute.
1. Put carret in editor.

This comment has been minimized.

Copy link
@f1ames

f1ames Jul 24, 2017

Member

Maybe something like:

  1. Switch to Source mode and make sure there is a span with id attribute.
  2. Switch back to WYSIWYG mode and put caret in the editor.
1. Open `Source` in editor. Make sure there is `span` with `id` attribute.
1. Put carret in editor.
1. Insert text, by usinng `Insert Special Character` button.
1. Check `Source` again

This comment has been minimized.

Copy link
@f1ames

f1ames Jul 24, 2017

Member

Missing dot.

temporaryElement = element;
element = sibling;
sibling = temporaryElement;
isNext = !isNext;

This comment has been minimized.

Copy link
@f1ames

f1ames Jul 24, 2017

Member

I was trying to find a case which fails without updating isNext flag, inserting special character in
<p><strong><em>12345^6</em><em>abcd</em></strong><strong><em>789</em></strong></p> does. Maybe we could add unit test covering this tc (probably here tests/core/dom/element/element.js) so this changes are covered?

This comment has been minimized.

Copy link
@msamsel

msamsel Jul 25, 2017

Author Contributor

Test is added in tests/core/dom/element/element.js

while ( sibling.data( 'cke-bookmark' ) || sibling.isEmptyInlineRemoveable() ) {
pendingNodes.push( sibling );
sibling = isNext ? sibling.getNext() : sibling.getPrevious();
if ( !sibling || sibling.type != CKEDITOR.NODE_ELEMENT )
if ( !sibling || sibling.type != CKEDITOR.NODE_ELEMENT ) {

This comment has been minimized.

Copy link
@f1ames

f1ames Jul 24, 2017

Member

You added parenthesis here, but there are two more places in this function (while and if below) without them. If you correcting the code style you should cover the whole function.

This comment has been minimized.

Copy link
@msamsel

msamsel Jul 25, 2017

Author Contributor

I add parenthesis in missing places in this function.

}

if ( element.isIdentical( sibling ) ) {
// Save the last child to be checked too, to merge things like
// <b><i></i></b><b><i></i></b> => <b><i></i></b>
var innerSibling = isNext ? element.getLast() : element.getFirst();

// #17009 swap elements, to analyse then in this same order (from lower index to higher)

This comment has been minimized.

Copy link
@f1ames

f1ames Jul 24, 2017

Member

Put ticket reference on the end with full link (as it is a Trac reference).

@@ -1703,6 +1704,17 @@
// Split inline elements so HTML will be inserted with its own styles.
path = range.startPath();
if ( ( node = path.contains( isInline, false, 1 ) ) ) {
// #17009 store elements with id. Beacuse siblings are marged always from lower child index to higher, refrence to node will persist after all operations.
for ( i=0; i < path.elements.length; i++ ) {

This comment has been minimized.

Copy link
@f1ames

f1ames Jul 24, 2017

Member

You could use CKEDITOR.tools.array.forEach here.


var pendingNodes = [],
temporaryElement, node;

This comment has been minimized.

Copy link
@f1ames

f1ames Jul 24, 2017

Member

node variable is not used anywhere.

msamsel added some commits May 26, 2017

@f1ames
Copy link
Member

left a comment

There is one case with copy/paste (clipboard plugin included) which behaves different than before fix (tested on tests/core/editable/manual/insertionkeepid):

  1. Set html <p><span id="SomeId1">Fo$o</span></p>.
  2. Copy $.
  3. Put selection like <p><span id="SomeId1">Fo^$o</span></p>.
  4. Paste few times.

Result

Before fix:

<p><span id="SomeId1">Fo$$$$$$$$$$$$$</span><span>$o</span></p>

After fix:

<p><span id="SomeId1">Fo</span><span id="SomeId1">$</span><span id="SomeId1">$</span><span id="SomeId1">$</span><span id="SomeId1">$</span><span id="SomeId1">$</span><span id="SomeId1">$</span><span id="SomeId1">$</span><span id="SomeId1">$</span><span id="SomeId1">$</span><span>$o</span></p>

When fixing it, please also provide tests covering such case.

**Unexpected:** As result you will obtain splited text into 2 span elements.
1. Switch to `Source mode` and make sure there is a `span` with `id` attribute.
1. Switch back to `WYSIWYG mode` and put caret inside `Foo` word.
1. Insert character usinng `Insert Special Character` button.

This comment has been minimized.

Copy link
@f1ames

f1ames Jul 27, 2017

Member

character using ...


**Expected:** There should remain one `span` element with `id`.

**Unexpected:** After specal character insertion `span` element was splited into two: e.g. `<span id="SomeId2">Fo#</span><span>o</span>`.

This comment has been minimized.

Copy link
@f1ames

f1ames Jul 27, 2017

Member

element was splited into

Past tense is split.

'test insert nothing into inline element with id': function() {
var editor = this.editors.paragraph,
bot = this.editorBots.paragraph;
bot.setHtmlWithSelection( '<p style="text-align: right;"><span class="marker" id="abc">te^st</span></p>' );
bot.setHtmlWithSelection( '<p style="text-align: right"><span class="marker" id="abc">te^st</span></p>' );

This comment has been minimized.

Copy link
@f1ames

f1ames Jul 27, 2017

Member

The bot.setHtmlWithSelection uses deprecated bender.tools.setHtmlWithSelection, you should use bender.tools.selection.setWithHtml instead.

@@ -1186,7 +1180,7 @@ CKEDITOR.dom.element.clearMarkers = function( database, element, removeFromDatab
// <b><i></i></b><b><i></i></b> => <b><i></i></b>
var innerSibling = isNext ? element.getLast() : element.getFirst();

// #17009 swap elements, to analyse then in this same order (from lower index to higher)
// Swap elements to return always this same order: from lowest to highest index (https://dev.ckeditor.com/ticket/17009).

This comment has been minimized.

Copy link
@f1ames

f1ames Jul 27, 2017

Member

always the same

or

to return them always in the same

"id": element.getAttribute('id')
});
element.removeAttribute('id');
// Store elements with id, beacuse siblings are marged always from lowest child index to highest,

This comment has been minimized.

Copy link
@f1ames

f1ames Jul 27, 2017

Member

are merged always

@@ -1967,6 +1980,11 @@

range.moveToBookmark( bm );

// #17009 restore id to previously stored elements.

This comment has been minimized.

Copy link
@f1ames

f1ames Jul 27, 2017

Member

Remember about proper ticket referencing (full Trac url on the end).

@msamsel

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2017

Note:
Currently realisation is paused.
Issue requires few decision to be made related to ID behaviour.
<0> - element without ID
<IDx> - element with some ID
^ - cursor

  • <ID1>Some^ text</ID1> + <0>input</0> -> what should be done with right part of ID1 element. What if there will be some classes or inline styles?
  • <ID1>Some^ text</ID1> + <ID2>input</ID2> -> Similar question as above.
  • <ID1 class="some fancy class">Some^ text</ID1> + <ID1>input</ID1> -> what if ID match but styles not?

Problem exists because insertText and insertHtml use this same method for inserting text.

@mlewand mlewand added the review:easy label Jul 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.