[Blink] New page does not trigger autoparagraping

[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

[Jakub Ś]

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

[Piotr Jasiun]

If you create a new page the paragraph is created in it. The problem is that selection in 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.

[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).

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

[Piotr Jasiun]

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.

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

[Piotr Jasiun]

Yes, they does.

[Piotrek Koszuliński]

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

[Piotr Jasiun]

Replying to Reinmar:

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

Done.

2. 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).

[Piotr Jasiun]

Changes in ​t/12630d.

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

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

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

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

[Piotrek Koszuliński]

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

[Piotrek Koszuliński]

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