Autoparagraphing not working in nested editable and enter duplicating nested editable

[AlexW]

Steps, 1: Add a simplebox widget to the editor. 2: Delete the "Content..." text. 3: Hit the return key to get a new line. 4: Hit the backspace key twice. 5: The p element has now been removed.

Any presses of the return/enter key now duplicates the "simplebox-content" div.

Can only reproduce on FireFox (I'm on v30)

[Piotrek Koszuliński]

Can be reproduce on plugins/widget/dev/nestedwidgets.html. Confirmed on FF only.

There are at least two problems:

1. auto paragraphing does not work
2. enter is not limited to nested editable.

The second issue is most likely reproducible on all browsers, but the first one can only be reproduced on FF. Both need to be fixed.

[Artur Delura]

First problem resolved. In Firefox for content:

<p>^<br></p>

Method getRangeAt(0) for selection returned object with properties startOffset and endOffset set to 1. Proper values should be 0.

​https://github.com/cksource/ckeditor-dev/commit/9fdc4a288e239a356842a2a357c5a732de5ed180

[Piotrek Koszuliński]

1. Please don't wrap existing single-line conditions in {}. We are in a transition moment, but this means that we can use {} in the new code. Not that we can change existing code.
2. If Firefox natively yields selection in <p><br>^</p> this means that the selection is there. The fix is incorrect because our API will return untrue selection. What needs to be fixed is code that makes the invalid selection, because it's likely that it's ours code. You can watch selectRanges() calls to see whether this is true.

[Piotrek Koszuliński]

The 4.4.3 milestone was shortened, so the remaining tickets must be postponed.

[Artur Delura]

1. auto paragraphing does not work

Method shouldAutoParagraph didn't care about nested editable at all. I have written test for case, and fixed it.

2. enter is not limited to nested editable.

Problem is with browser which reset selection range when we switch between nested editable. I reported it on bugzilla: ​https://bugzilla.mozilla.org/show_bug.cgi?id=1039217

Anyway, use case described in ticket summary. Works well. I think this ticket is related to #11687 Because problem source is the same. Changes on ​branch:t/12162.

[Piotrek Koszuliński]

1. Code style in editable.js.
2. ( pathBlock && pathBlock.equals( pathBlockLimit ) )this is too broad - check whether pathBlockLimit.getAttribute( 'contenteditable' ) == 'true'. Though, I'm not sure whether you'll be able to write a test for this addition - hard to find a matching case.
3. Add a comment pointing to this ticket.
4. 'autop' => 'autoparagraphing'
5. You don't need to fire selectionChange in tests. At least you should not need to do that. It should happen automatically when you change selection.
6. Remove autop tags from unrelated tests files - namely all plugins.
7. I don't think that this bug (​https://bugzilla.mozilla.org/show_bug.cgi?id=1039217) has anything to do with incorrectly working enter key. Please create a test in which you set config.autoParagraph to false, and selection right in nested editable (<div>) and exec enter command. See what happens - nested editable should not be duplicated.

[Artur Delura]

Replying to Reinmar:

2. ( pathBlock && pathBlock.equals( pathBlockLimit ) )this is too broad - check whether pathBlockLimit.getAttribute( 'contenteditable' ) == 'true'. Though, I'm not sure whether you'll be able to write a test for this addition - hard to find a matching case.

I've changed to ( pathBlock && pathBlock.getAttribute( 'contenteditable' ) == 'true' )

5. You don't need to fire selectionChange in tests. At least you should not need to do that. It should happen automatically when you change selection.

Unfortunatelly I have to, I spend a lot of time to make it work, it was long time ago, but as far as i remember some condition wasn't meet to fire event (Maybe old selection was same as new one?).

7. I don't think that this bug (​https://bugzilla.mozilla.org/show_bug.cgi?id=1039217) has anything to do with incorrectly working enter key. Please create a test in which you set config.autoParagraph to false, and selection right in nested editable (<div>) and exec enter command. See what happens - nested editable should not be duplicated.

You were right. I added new test and fixed auto paragraphing in nested editable on enter key. By the way: here is use case which fail because of firefox bug: ​http://dev.ckeditor.com/ticket/11687#comment:3

Changes in ​branch:t/12162

[Piotrek Koszuliński]

1. To fix this issue in a clean way we need to close #12279 first. We'll be able to remove the getClosestEditable function which you needed to introduce.
2. You may have problems with selectionChange in tests because you implemented this new test based on existing domfix.js tests which are bad. They mock editable what may distort editor's behaviour. Instead, please revert changes in domfix.js and create domfixnestededitable.js with a test using normal editor.
3. You haven't created a test for bug in enterkey plugin.

These changes will take more time than we have in 4.4.4, so most likely we'll need to postpone this ticket.

[Artur Delura]

Replying to Reinmar:

1. To fix this issue in a clean way we need to close #12279 first. We'll be able to remove the getClosestEditable function which you needed to introduce.

Done.

2. You may have problems with selectionChange in tests because you implemented this new test based on existing domfix.js tests which are bad. They mock editable what may distort editor's behaviour. Instead, please revert changes in domfix.js and create domfixnestededitable.js with a test using normal editor.

Done.

3. You haven't created a test for bug in enterkey plugin.

I have, but it was hidden :)

changes in same ​branch:t/12162.

[Piotrek Koszuliński]

There are couple of red tests when running ​http://tests.ckeditor.dev:1030/#tests/autoparagraphing,enter

[Piotrek Koszuliński]

Most likely you try to call getAttribute on DOM fragment which may be one of the ancestors. Either we must fix getAncestor() or the function in enterkey plugin - I would do the latter.

[Artur Delura]

I think we should not modify getAncestor() method to filter DOM fragments. It limit functionality somehow. What we could do is add extra parameter which skip DOM fragments. But do we really need this? I think the best way is to leave responsibility for developer to filter such cases. That's why we introduce function as a first argument, to give developer more flexibility. To be clear - not doing nothing in this field.

I pushed additional commit to ​branch:t/12162.

[Artur Delura]

Rebased to current master as well.

[Piotrek Koszuliński]

The branch needed many improvements (see my commits), but the dev code was ok. Fixed on master with ​git:3cf4f01.