Opened 9 years ago
Closed 9 years ago
#13142 closed Bug (fixed)
[Edge] CTRL+A, backspace results in an empty div
| Reported by: | Piotrek Koszuliński | Owned by: | Tade0 |
|---|---|---|---|
| Priority: | Normal | Milestone: | CKEditor 4.5.4 |
| Component: | General | Version: | 4.5.0 Beta |
| Keywords: | Cc: |
Description
Follow up of #12858.
- Open any sample with the classic editor.
- Select all with CTRL+A.
- Press backspaces.
Expected: empty paragraph (or heading if it was the first block).
Actual: empty div.
This issue doesn't seem to be reproducible in inline editor. This can be a clue to how to solve it.
Attachments (1)
Change History (44)
comment:1 Changed 9 years ago by
| Status: | new → confirmed |
|---|
comment:2 Changed 9 years ago by
| Milestone: | CKEditor 4.5.0 → CKEditor 4.5.1 |
|---|---|
| Summary: | [Spartan] CTRL+A, backspace results in an empty div → [Edge] CTRL+A, backspace results in an empty div |
We are still unable to work on such issues due to Edge's instability.
comment:3 Changed 9 years ago by
| Owner: | set to Tade0 |
|---|---|
| Status: | confirmed → assigned |
comment:4 Changed 9 years ago by
This seems to be a vendor issue.
I filed a bug report: https://connect.microsoft.com/IE/feedbackdetail/view/1501285/edge-unexpected-empty-div-left-when-removing-whole-content-from-body-with-contenteditable-true and I'll be updating this ticket as soon as I gain any new information on its status.
comment:5 Changed 9 years ago by
How does IE11 and other browsers handle CTRL+A;DEL natively? It isn't so obvious that this is a bug unfortunately. Also, we may be already manually handling CTRL+A;DEL on some browsers. This should be checked.
comment:6 Changed 9 years ago by
Other browsers that I tested have the following behavior:
IE11 @ Win10: Empty <p>
Chrome @ Ubuntu: Single <br>
What's important is that if it's not the <body> element that's editable but something else the result is different only on Edge.
For example a single editable <div> on Edge has a <br> element inside after deleting the text - which is the expected behavior.
comment:8 Changed 9 years ago by
I guess we should create at least a temporary fix.
We'll have to watch out for regressions in case the Edge devs change something again to make this behavior consistent in their browser(that is if they recognize it as a bug).
comment:9 Changed 9 years ago by
Here is the sequence of events that leads to this bug:
- When the user hits backspace (or delete) on the whole selected content then it gets deleted and the browser inserts an empty, superfluous <div> into the empty editable.
- A selectionchange event is fired that in turn evokes the fixDom function from core/editable.js.
- Normally at this point we would have an editable that's either empty or contains a bogus(<br>) or a <p> element(like in IE11). fixDom is supposed rectify the state of the editable so that depending on the enterMode the editable contains either a <p>, <div> or just a <br>(enter mode - br). Having encountered an empty <div> it breaks and only inserts a bogus.
My original approach (available in branch:t/13142) was to recognize this state as a one that requires adding a paragraph and delete the superfluous <div> right before adding the missing paragraph element, but that didn't take into account the situation when the enterMode is <br> and no paragraph should be added.
Now I will handle deleting that <div> as a separate step done before any other fixes. Tests will have to check if this doesn't break anything concerning the 'div' plugin.
comment:10 Changed 9 years ago by
| Status: | assigned → review |
|---|
Changes pushed to branch:t/13142a.
I had to put deleting the div after adding the bogus, because node removal invalidates the selection.
comment:11 Changed 9 years ago by
| Status: | review → review_failed |
|---|
First of all note that this problem occurred only using Backspace. Then I use Delete additional <div> is not added.
I checked the behavior on other browsers and and Chrome removes content completely, but IE 11 keeps <p>. These situations are handled later by the fixDom method you patched.
However, when I set the enterMode = CKEDITOR.ENTER_DIV,run IE 11, create a single paragraph in the editor <p>foo</p>, press CTRL+A;Backspace the <p> is properly removed and replaced by <div>. The <p> is not removed naively so there must be a code in the editor which do it. Moreover when I commented out fixDom method <p> is still removed. The same code should handle <div> and remove it the same way, because this is the same case. We should not create separate fix for it.
Fortunately there is no conflict with div plugin, because it automatically add <p> inside every <div>, event with autoParagraph = false; and enterMode = CKEDITOR.ENTER_DIV. Do not forget to check if your fix works fine with div-based editor. It should not be a issue, but lets make sure nothing is removed there.
One more think are tests.
Unit tests are missing. Of course you can not test browser behavior when you select all and press Backspace, but you can test if the single <div> is properly replaced with <p> or removed. Eventually other cases.
Manual test could be useful not also for this ticket but also for others. Since we are porting old sample, we used for testing, to bender manual tests, you could port this sample: http://ckeditor.dev/plugins/enterkey/samples/enterkey.html You can rethink this test to make it as simple to use as it is possible (I do not mind if it contains 9 editors). Make it useful not only for this case, but for general enter mode testing. Then use it with the changed description (same HTML, different MD) for your case.
comment:12 Changed 9 years ago by
And one more think: debugging and, in general, working with tickets on Edge, is very hard now, because Edge is too unstable. I believe handling are reviewing tickets like this at this stage is a waste of time and we should come back when Edge became more stable.
comment:13 Changed 9 years ago by
I found the piece of code that deletes that <p> in IE11 in enterMode=ENTER_DIV: https://github.com/cksource/ckeditor-dev/blob/t/13142a/plugins/wysiwygarea/plugin.js#L176
I'll modify it so that it also removes a single <div> in edge.
comment:14 Changed 9 years ago by
As always there was more to this than a single line of code that needed to be changed.
I added unit tests that create this situation regardless of the browser they are run on.
Changes pushed to branch:t/13142b.
comment:15 Changed 9 years ago by
| Status: | review_failed → review |
|---|
comment:16 Changed 9 years ago by
| Status: | review → review_failed |
|---|
After your change there is circular reference and browser crash when I press CTRL+A;Backspace. See that you only check that the enter mode is different than what browser is inserting (browser insert <div> by default, but should be <p>) not what is in fact inserted. When this element will be fixed editor will fix it again, and again, and again...
Second: enterMode is not the best name for the variable you introduced. It is what the browser insert, and has nothing with the enter mode.
Third: add more comments to your code. I have no idea why there is range.startContainer.equals( body ) || CKEDITOR.env.edge what is range.startContainer on Edge? Should not we check it somehow (it could prevent the circular reference).
comment:17 Changed 9 years ago by
Fixed that infinite loop, but there's another problem that I found: selecting all and pressing any alphanumeric key bypasses the implemented protection on Edge.
I'll fix this and add an appropriate test.
comment:18 Changed 9 years ago by
While I was fixing the problem with the infinite loop I found an edge case that we need to cover: if the user, instead of pressing backspace, starts typing the selectionchange event fires when the first character is already inserted.
As suggested by Reinmar, I went with listening on keydown events and marking divs that were already there so that on keyup I would remove those that weren't marked(keydown fires before the superfluous div appears, keyup fires after).
This works as long as the keydown and keyup events don't overlap. In this sequence of events: keydown keydown keyup keyup
the problem persists, because the second keydown marks the superfluous div as one that should be retained.
comment:19 Changed 9 years ago by
| Status: | review_failed → assigned |
|---|
comment:20 Changed 9 years ago by
| Status: | assigned → review |
|---|
I solved the problem with overlapping events by locking marking and removing appropriately.
Tests added that show how this mechanism behaves.
Changes pushed to branch:t/13142c.
comment:21 Changed 9 years ago by
| Status: | review → assigned |
|---|
comment:22 Changed 9 years ago by
| Status: | assigned → review |
|---|
I merged together the parts that deal with <p> on IE and <div> on Edge.
Changes pushed to branch:t/13142c.
comment:24 Changed 9 years ago by
Forgot to cherry-pick them from the previous iteration.
Changes pushed to branch:t/13142c.
comment:25 Changed 9 years ago by
| Status: | review → review_failed |
|---|
I rebased your branch and pushed one more commit there.
Noticed few things:
- On Edge: CTRL+A, Enter. 3 paragraphs are created.
- The above may be caused by the fact that you're listening to keydown with priority -1, so before e.g. the enter key. I think that the hack should be only executed if none other listener handles this key, so with the lowest possible priority.
- I need an explanation why simply removing the block works. I would expect that if you remove it you also remove all the content, e.g. a letter that was typed after pressing CTRL+A.
- I don't think that the MT needs "type some multiline text" in its description. I think there should be a special note that one should check various scenarios like multiple lines, single lines, created in many ways, deleted in many ways (typing, delete, enter, etc). Please write a better description which will explain complexity of this case.
comment:26 Changed 9 years ago by
I've set the priority to 999 and also tested the version without the keydown, keyup events attached. The quirk still occurs. Could this be yet another problem with Edge?
comment:27 Changed 9 years ago by
| Status: | review_failed → assigned |
|---|
comment:28 Changed 9 years ago by
The first two paragraphs are expected, because it's the enter key that's pressed. Other keys don't create this effect.
The third paragraph (or div) is created here: https://github.com/cksource/ckeditor-dev/blob/t/13142c/plugins/enterkey/plugin.js#L384
Investigating why this is occurring.
comment:29 Changed 9 years ago by
Actually, the culprit lies here: https://github.com/cksource/ckeditor-dev/blob/t/13142c/plugins/enterkey/plugin.js#L282
After this line in Chrome we have an empty paragraph, and in Edge we end up with two empty paragraphs.
comment:30 Changed 9 years ago by
I checked if this happens on latest master and it does.
The problem lies in what happens, if we select the whole content(e.g. a single paragraph) on Edge, IE and Firefox and call range.splitBlock(). Only on Chrome the range is anchored within the paragraph and on the other browsers both the start and end containers are set to <body>.
splitBlock() treats this as a situation where it should first split that paragraph and then append another to the end(not sure why - this is some old code here).
For me this is a separate issue and it should have(if it doesn't already) it's own ticket.
comment:31 Changed 9 years ago by
OK, I can see. On Firefox, if I have just one line of text, press CTRL+A and Enter, I have two lines, but the caret is in the first one. That doesn't seem right. Please report it, but I think we can ignore this issue, as it doesn't sound too serious.
So, let's finally conclude this ticket.
comment:32 Changed 9 years ago by
I'll update the MT according to comment:25 and push.
As for 3. from that comment: For backspace/delete it works, because the sequence of events is like this: keydown selectionchange selectionchange (again) keyup selectionchange selectionchange (again)
The second series of selectionchange events fires when the superfluous div is already removed by the keyup listener. That's when autoparagraphing kicks in and we get the expected result.
comment:34 Changed 9 years ago by
| Milestone: | CKEditor 4.5.2 → CKEditor 4.5.3 |
|---|
comment:36 Changed 9 years ago by
Added some commentary to the more obscure parts of the code as per request from pjasiun.
Changes pushed to branch:t/13142c.
comment:37 Changed 9 years ago by
| Milestone: | CKEditor 4.5.3 → CKEditor 4.5.4 |
|---|
Because these changes can touch more that we expected at the begging it is safer to move it to the next release.
comment:38 Changed 9 years ago by
| Status: | review → review_failed |
|---|
I rebased branch on master, pushed an additional commit, checked manual test (which is fine), checked code changes (now looks good), checked all tests in IE11 (all green), but...
3 tests in http://tests.ckeditor.dev:1030//tests/plugins/autoembed/autoembed are red on the ticket branch but green on master in Edge.
Too bad.
comment:39 Changed 9 years ago by
Also do not forget to create a ticket mentioned in https://dev.ckeditor.com/ticket/13142#comment:31.
comment:40 Changed 9 years ago by
| Status: | review_failed → review |
|---|
I had 2 failing tests, but after I ran one of them all have become green all of a sudden.
That was IE11. Have to repeat that on Edge.
comment:41 Changed 9 years ago by
| Status: | review → assigned |
|---|
comment:42 Changed 9 years ago by
| Status: | assigned → review |
|---|
Ran all the units tests on Edge and they are green (except for pastefromword, but these fail on master as well).
comment:43 Changed 9 years ago by
| Resolution: | → fixed |
|---|---|
| Status: | review → closed |
It's green indeed. Closed, finally. git:89d54d8.
It is there from at least CKE 4.0 so it looks like IE12 "interesting feature".