#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 )
- Open replacebycode sample.
- Press the new page page.
- Elements path shows only the body tag.
- Press bold button.
- Elements path shows body>strong
Change History (20)
comment:1 Changed 10 years ago by
Description: | modified (diff) |
---|
comment:2 Changed 10 years ago by
Status: | new → confirmed |
---|---|
Version: | → 4.0 |
comment:3 Changed 10 years ago by
Owner: | set to Piotr Jasiun |
---|---|
Status: | confirmed → assigned |
comment:4 Changed 10 years ago by
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.
comment:5 Changed 10 years ago by
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 10 years ago by
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 10 years ago by
Status: | assigned → review |
---|
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 10 years ago by
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:10 follow-up: 11 Changed 10 years ago by
Status: | review → review_failed |
---|
- The patch should also fix the behaviour on inline editor and in editor with the divarea.
- 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 Changed 10 years ago by
Replying to Reinmar:
- The patch should also fix the behaviour on inline editor and in editor with the divarea.
Done.
- 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:13 Changed 10 years ago by
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:
- Open inlineall sample on FF or IE.
- Focus the top right hand editor.
- Click new page.
- Type.
- 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 10 years ago by
Status: | review → review_failed |
---|
comment:15 follow-up: 16 Changed 10 years ago by
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 Changed 10 years ago by
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 10 years ago by
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 10 years ago by
Owner: | changed from Piotr Jasiun to Piotrek Koszuliński |
---|---|
Status: | review_failed → review |
comment:19 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | review → closed |
Fixed on master with git:9fc1a76026. Fixes #12104 as well.
Problem can be reproduced in Blink browsers from CKEditor 4.0 (works fine in 4.0 beta).