Opened 4 years ago

Closed 4 years ago

#12162 closed Bug (fixed)

Autoparagraphing not working in nested editable and enter duplicating nested editable

Reported by: AlexW Owned by: Artur Delura
Priority: Normal Milestone: CKEditor 4.4.5
Component: UI : Enter Key Version:
Keywords: Cc:

Description

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)

Change History (19)

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

Component: UI : WidgetsUI : Enter Key
Milestone: CKEditor 4.4.3
Status: newconfirmed
Version: 4.4.2

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.

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

Summary: simplebox widget, content div duplicatedAutoparagraphing not working in nested editable and enter duplicating nested editable

comment:3 Changed 4 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

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

comment:5 Changed 4 years ago by 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.

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

Milestone: CKEditor 4.4.3CKEditor 4.4.4

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

comment:7 in reply to:  1 Changed 4 years ago by Artur Delura

Status: assignedreview
  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.

  1. 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.

Last edited 4 years ago by Artur Delura (previous) (diff)

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

Status: reviewreview_failed
  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.

comment:9 in reply to:  8 Changed 4 years ago by Artur Delura

Replying to Reinmar:

  1. ( 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' )

  1. 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?).

  1. 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

Last edited 4 years ago by Artur Delura (previous) (diff)

comment:10 Changed 4 years ago by Artur Delura

Status: review_failedreview

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

Status: reviewreview_failed
  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.

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

Milestone: CKEditor 4.4.4CKEditor 4.4.5

comment:13 Changed 4 years ago by Artur Delura

Status: review_failedassigned

comment:14 in reply to:  11 Changed 4 years ago by Artur Delura

Status: assignedreview

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.

  1. 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.

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

I have, but it was hidden :)

changes in same branch:t/12162.

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

Status: reviewreview_failed

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

comment:16 Changed 4 years ago by 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.

comment:17 Changed 4 years ago by 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.

comment:18 Changed 4 years ago by Artur Delura

Status: review_failedreview

Rebased to current master as well.

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

Resolution: fixed
Status: reviewclosed

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

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