Opened 11 years ago
Closed 11 years ago
#10438 closed Bug (fixed)
[FF&IE] No selection put in editable on setData
Reported by: | Piotrek Koszuliński | Owned by: | Piotrek Koszuliński |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 4.2 |
Component: | Core : Selection | Version: | |
Keywords: | Cc: |
Description (last modified by )
Attached two samples.
TC:
- Open framed_focus.html sample.
- Open console.
- Execute
setData();
. - Quickly focus editor and wait.
- After data will be set verify:
- Whether caret blinks.
- Whether
EDITOR#BLUR
is logged when you click outside editor. - Whether you can type and whether after typing additional paragraph is not created (what means that selection was placed outside editable place.
Note: selection issues (3rd point) in inline editor are not a subject of this ticket. The most important part is fixing blur problem (2nd point), selection in framed editor is fixed because it was convenient.
Attachments (2)
Change History (17)
comment:1 Changed 11 years ago by
Status: | new → confirmed |
---|
comment:2 Changed 11 years ago by
Owner: | set to Piotrek Koszuliński |
---|---|
Status: | confirmed → assigned |
comment:3 Changed 11 years ago by
Description: | modified (diff) |
---|---|
Summary: | [FF] No selection put in editable on setData → [FF&IE] No selection put in editable on setData |
Changed 11 years ago by
Attachment: | framed_focus.html added |
---|
Changed 11 years ago by
Attachment: | inline_focus.html added |
---|
comment:4 Changed 11 years ago by
comment:5 Changed 11 years ago by
There will be another advantage of promoting this fix higher. We will be able to make the initial caret location configurable and I have seen such requests.
Although, since we are short on time, we may work on extracting this patch later, because that would require some research and tests.
comment:7 Changed 11 years ago by
I pushed few more commits to tests and dev making all browsers pass them.
- IE7/8 - very similar fix to that for IE9.
- Opera - it says: 'everything is fine, really! selection and active element are fine!'... yeah - right. I haven't found a way to discover that selection is broken, so I just added tests to ignored.
comment:8 Changed 11 years ago by
It turns out that we have to improve this patch and use for all setData
calls (also initial one) earlier than I thought. As #10439 proves, we need to have consistent initial selection (before first focus, after focus&setData, after switching from source mode and in all other cases) in all browsers.
In cases when we do not need selection locking (FF and Chrome with framed editor) it will be as simple as it is now - on editor load we need to set selection in the right place (range.moveToElementEditStart( this.root, true/false )
). In other cases I think that we can mock initial locked selection, as the editor was focused before. Although... brace yourself - critical core changes are coming.
comment:9 Changed 11 years ago by
Status: | assigned → review |
---|
I pushed simplified version of t/10438.
Tests:
- FF latest - 100%
- Chrome - 100%
- FF 3.6 - regression: http://ckeditor4.t/dt/core/selection/editor.html - "test selection on initial focus - ensure new doc" - but do we support 3.6?
- IE9 - 100%
- IE8 - 100% but I got alert
'Stack overflow at line: 0'
in http://ckeditor4.t/dt/core/htmldataprocessor.html which... does not test selection at all. Although, it loads data, so perhaps this is it. I'll debug this if you can confirm alert.
Note: Tests created for this ticket contains some events logging - I left this intentionally - they help while testing.
comment:10 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:11 Changed 11 years ago by
I tested FF latest for #4472, #5021, #3864, #5781 which could become regressions after git:cc72d842 (Commit: "Removed unncessary hack."), but everything is fine.
comment:12 Changed 11 years ago by
Status: | review → review_passed |
---|
comment:13 Changed 11 years ago by
I tried to debug that IE8's stack overflow alert. I reduced the TC to the absolute minimum:
<!DOCTYPE html> <html> <head> <title>CKEDITOR.htmlDataProcessor</title> <meta name="cktester-tags" content="editor,unit"> <script src="../../cktester/cktester.js"></script> <script> CKTESTER.editor = { config: { enterMode: CKEDITOR.ENTER_BR } }; CKTESTER.test( { 'ie, we will never forget what you did to us': function() { this.editor.focus(); this.editorBot.setData( '', function() { assert.isTrue( true ); } ); } } ); </script> </head> <body></body> </html>
Still, alert is opened.
Updates:
- IE8 doesn't allow me to place break point in TC file or it misses it.
- I overwrote alert function, but apparently IE calls it internally.
- It's not possible to catch that error (cause there's no error) in dev or test code.
- Surprisingly, IE9 also notifies about some problems:
SCRIPT28: Stack overflow document.js, line 220 char 3 SCRIPT2343: Stack overflow in line: 220
- There really is some kind of inf recursion. It's getting hotter here!
comment:14 Changed 11 years ago by
OK. I pushed additional fix for IEs preventing from infinitive recursion between this new selection fix and fix for #6966. It would be cool to find a better solution, but for now I'm closing this issue, because it's blocking us.
comment:15 Changed 11 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Fixed on major with git:b83e32f on dev and c84d41f on tests.
This is the commit which would be cool to enhance: git:260015a1.
Pushed t/10438 on tests and t/10438 on dev with prototype of a patch.
Tests are fixed on FF and IE9 and they pass on Webkit on master. I haven't yet tested whether IE8, IE10 and Opera also require this fix.
Manual tests on framed_focus.html also pass.
Manual tests on inline_focus.html does not fully pass - selection is not placed in editable place on FF and IE (Chrome is ok). On IE9 additionally, selection is placed at the end of editable, not at the beginning.
This makes me thing that perhaps we should promote these fixes to some editor's method called on every setData. Perhaps the original fix for Webkit also does not have to be in selection construtor. I've found that it was proposed for these issues: