Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#12630 closed Bug (fixed)

[Blink] New page does not trigger autoparagraping

Reported by: Piotrek Koszuliński Owned by: Piotrek Koszuliński
Priority: Must have (possibly next milestone) Milestone: CKEditor 4.4.6
Component: General Version: 4.0
Keywords: Blink Cc:

Description (last modified by Piotrek Koszuliński)

  1. Open replacebycode sample.
  2. Press the new page page.
  3. Elements path shows only the body tag.
  4. Press bold button.
  5. Elements path shows body>strong

Change History (20)

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

Description: modified (diff)

comment:2 Changed 4 years ago by Jakub Ś

Status: newconfirmed
Version: 4.0

Problem can be reproduced in Blink browsers from CKEditor 4.0 (works fine in 4.0 beta).

comment:3 Changed 4 years ago by Piotr Jasiun

Owner: set to Piotr Jasiun
Status: confirmedassigned

comment:4 Changed 4 years ago by Piotr Jasiun

If you create a new page the paragraph is created in it. The problem is that selection is before that paragraph, not in it:

<body...>[]<p><br></p></body>

We could move it into that paragraph in fixDom (ugly hack in t/12630a), but the problem is not here.

fixDom is called multiple times and if we print content of the editable and range.startContainer in it, it turns out that the selection is good as some point, but changed to be bad later.

The other solution is to use focus and blur instead of DOMFadeIn and DOMFadeOut in selection.js (t/12630b). It turns out that it is enough to use focus in selection.js:L699 or comment out this line, so not call selection change. What is interesting this line change the selection form body to p element so it seems that fixing problem at this stage is the problem in the future.

It is hard to debug, because everything works fine when debugger runs. Also everything is fine if I call editor.execCommand('newpage'); from the console.

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

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

The reason of the problem is the fixInitialSelection (selection.js) call at the very beginning after loading data (straight from dataReady -> getSelection()).

At this point there's no paragraph yet, because this happens while checking if selection has changed (and fixDom listens on selectionChange). The code in the fixInitialSelection tries to find the first editable space in the body element, but since it's empty, it places selection there. Then some odd things happen with the engine, but basically it looks that even though fixDom moves the selection to the created paragraph it is somehow ignored and on the next DOMFocusIn selection moves back to the body. This may be a browser bug, but it's hard to tell due to the events mess that the editor causes itself.

The strangest thing is that the data processor does not create the paragraph itself. If it does that it would help, because when I change the new page plugin so it sets data to '<p></p>' everything starts to work fine (because fixInitialSelection can place the selection inside the paragraph).

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

In branch:t/12630c I added code which checks in the parser whether the root element is empty and fills it with a <p><br /></p>. That passes all tests (there's one obvious red in htmlDP) and fixes the issue.

It was always strange for me that we don't fix the empty root situation while parsing or processing content. Even if that fix cannot be in the parser (because it has too wide effect then), it can be placed in the htmlDP.toHtml() method.

comment:7 Changed 4 years ago by Piotr Jasiun

Status: assignedreview

As you said I put the fix in htmlDP.toHtml() and write tests for it. It seems to fix the problem. Because general case works fine when I use editor.execCommand('newpage'); there is no point to write plugin level test for new page, they would work anyway. Changes in t/12630d.

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

Asking before I checked - does the test fail when run with dev from the master branch? I know that this was a pretty fragile case.

comment:9 Changed 4 years ago by Piotr Jasiun

Yes, they does.

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

Status: reviewreview_failed
  1. The patch should also fix the behaviour on inline editor and in editor with the divarea.
  2. There's no need to create editorP since it's identical with editor1.

BTW. I understood that this patch will fix one more thing. There is bug that setting empty data in inline editor makes it invisible (because the height is 0px).

comment:11 in reply to:  10 Changed 4 years ago by Piotr Jasiun

Replying to Reinmar:

  1. The patch should also fix the behaviour on inline editor and in editor with the divarea.

Done.

  1. There's no need to create editorP since it's identical with editor1.

I don't see any editor1 there. There is editor with enterMode=BR and editor2 which is inline.

BTW. I understood that this patch will fix one more thing. There is bug that setting empty data in inline editor makes it invisible (because the height is 0px).

comment:12 Changed 4 years ago by Piotr Jasiun

Status: review_failedreview

Changes in t/12630d.

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

I pushed two commits to branch:t/12630d with small improvements.

Unfortunately, I realised then that this patch would need a serious improvement in the editable-selection layer. TC:

  1. Open inlineall sample on FF or IE.
  2. Focus the top right hand editor.
  3. Click new page.
  4. Type.
  5. Notice that the text was added before the paragraph. Directly in the body.

This means that we would need a good initial selection fix. We always had problems with that but it's not easy to fit the fix into all the code run during the editor initialisation. From the long-term perspective, I have to say that we should have this (this is also related to cases where there's a block widget in the content), but I don't see this possible now.

So the question is whether we want to work on both these problems in 4.5.0 or we should better fix this the current issue ASAP.

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

Status: reviewreview_failed

comment:15 Changed 4 years ago by Frederico Caldeira Knabben

I've just appended a commit to t/12630d, which fixed the FF case. The problem is that the check for fixInitialSelection in Gecko was not taking in consideration inline editors.

I didn't have the chance to check IE because my vm has just broken but hopefully the fix for that will be similar.

comment:16 in reply to:  15 Changed 4 years ago by Piotrek Koszuliński

Replying to fredck:

I've just appended a commit to t/12630d, which fixed the FF case.

... and 5 long days later I'm pushing branch:t/12630d with pretty deep refactoring of the initial selection fixing mechanism. It turned out to be rather impossible to discover inside the selection constructor whether we are dealing with situation in which we must fix the selection or not. Therefore, we decided to move the fix to a new editable.fixInitialSelection method and call it right after the editable's innerHTML is changed.

This simplified the whole situation, because now it's unlikely that we'll break an existing selection, since it cannot exist yet. Although, this is a big change in terms of the execution flow and has hard to predict consequences. I needed to tune up many grumbling tests, workaround a very ugly Firefox issue and perform a lot of tests. Fortunately, everything looks very good and is surprisingly stable.

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

BTW. This patch fixes also #12104 - I restored all tests that we needed to mark as regressions. This is also a result of simplification - we don't hit some edge bugs in the browsers.

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

Owner: changed from Piotr Jasiun to Piotrek Koszuliński
Status: review_failedreview

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

Resolution: fixed
Status: reviewclosed

Fixed on master with git:9fc1a76026. Fixes #12104 as well.

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

It turned out that this patched fixed also #12257 :).

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